Enhance command line utils to take a tree/file sha #1658
Conversation
|
Offhand the idea is cool, and I love to help push forward the container+OS as single tree concept and make that work better. It's not a private API if we're adding it to the symbol table 😄. We did add a special way for the CLI to call into private library bits, see In just glancing at the code: There are various leftover debug prints, and can you fix the space-between-identifer-and-parent i.e. Thanks again for working on this! Really looking forward to getting more of your patches in. |
|
☔ The latest upstream changes (presumably c7b12a8) made this pull request unmergeable. Please resolve the merge conflicts. |
wmanley
force-pushed
the
treeish
branch
2 times, most recently
from
93106ee
to
cdfb9e8
Compare
Sorry, that was sloppy. I was in a rush to finish it the week before last as I suspected that I wouldn't have time last week. I've fixed the formatting issues - at least the ones I spotted. I had a crack at configuring GNU indent and clang-format to format according to ostree style, but failed on both. I couldn't get either to align the I've changed the approach to private API as recommended. We use I've also broken up the main commit into 3 to make it easier to review. Please review. |
|
CI isn't passing, but I don't think it's related: f28-rust says:
FAH28-insttests says:
|
src/libostree/ostree-repo.c
Outdated
| } | ||
|
|
||
| if (g_strcmp0 (ostree_repo_file_tree_get_metadata_checksum (ret_out), | ||
| "00000000000000000000000000000000") == 0) |
There was a problem hiding this comment.
Hmm...we don't use the all-zero sha256 bit pattern anywhere do we? The empty tree metadata checksum looks like it's 6e340b9cffb37a989ca544e6bb780a2c78901d3fb33738768511a30617afa01d (from /usr/share/empty) which is itself different from the empty sha256:
$ sha256sum </dev/null
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 -
$
Am I misunderstanding something? How can this case be hit?
There was a problem hiding this comment.
I set it explicitly below in resolve_sha_to_tree to signify that we don't have a dirmeta SHA. On closer inspection it seems that there aren't enough 0s for a sha256. This is git sha length :). I can fix this but maybe you have a better idea.
| } | ||
|
|
||
| GFileType type = g_file_query_file_type ( | ||
| (GFile*)ret_out, G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS, cancellable); |
There was a problem hiding this comment.
I think it'd be easier to use g_file_query_info() here - that way we just can just throw (return error) if we hit ENOENT etc.
| if (!ostree_repo_read_commit (self, sha, &root_gfile, NULL, cancellable, | ||
| error)) | ||
| return FALSE; | ||
| g_autoptr(OstreeRepoFile) root = OSTREE_REPO_FILE (root_gfile); |
There was a problem hiding this comment.
This is relatively minor and it's mostly OK as is, I think I'd prefer though:
g_assert (OSTREE_IS_REPO_FILE (root_gfile));
OstreeRepoFile *root = (OstreeRepoFile*) root_gfile;
src/libostree/ostree-repo.c
Outdated
| * REMOTE:REF | ||
| * :REF | ||
| * REF | ||
| * COMMIT_SHA |
src/libostree/ostree-repo.c
Outdated
| * don't have one. We're getting a subpath here anyway we're not going to | ||
| * return this invalid file. */ | ||
| g_autoptr(OstreeRepoFile) root = _ostree_repo_file_new_root ( | ||
| self, sha, "00000000000000000000000000000000"); |
There was a problem hiding this comment.
This is where I set the "00000000000000000000000000000000" SHA. I guess the reference above deserves a comment.
|
There's a bug in ostree diff introduced here. Running: fails with presumably because ostree diff by default adds |
|
☔ The latest upstream changes (presumably 6869bad) made this pull request unmergeable. Please resolve the merge conflicts. |
So the local refspec "abcd" can be referred to as ":abcd" too. This is in preparation for allowing a second : to refer to a path.
`_ostree_repo_open_file` is intended to be used by our command line
utilities to convert from the text description of a commit/tree/ref, etc.
into an `OstreeRepoFile` representing it. It takes care to give fairly
good error messages.
This commit introduces the concept of a treeish. This is intended to make
the command line interface more flexible. We now have a way of referring
to a directory or a file within a commit. Once we migrate the command
line tools over to it they will become more general.
So with this we'll be able to write:
:myref:a/path
to refer to the tree a/path under commit pointed to by myref.
`ostree_repo_read_file` is a more general version of
`ostree_repo_read_commit` and can be used in its place for greater
flexibility when you're just after a tree anyway and don't need access to
the commit object itself.
I'm implementing this because I want greater flexibility when composing
trees with `ostree commit`.
Command-line users of `_ostree_repo_open_file` can now take a DIRTREE
SHA if they are expecting to deal with a tree object. This is a
generalisation intended to make the command-line interface more flexible.
There is one limitation: If you pass a dirtree sha you must also pass a
subdir to use. We need this because otherwise the tree won't have an
associated DIRMETA sha.
e.g.
ostree ls [DIRTREE SHA] bin
would now be valid whereas
ostree ls [DIRTREE SHA]
is not.
`_ostree_repo_open_file` is intended to be used by our command line
utilities to convert from the text description of a commit/tree/ref, etc.
into an `OstreeRepoFile` representing it.
This commit extends the treeish syntax so we can refer to a subtree. This
is intended to make the command line interface more flexible. Once we
migrate the command line tools over to `_ostree_repo_open_file` they will
become more general.
So with this we'll be able to write:
:myref:a/path
to refer to the tree a/path under commit pointed to by myref.
[TREE SHA]:a/path
is also valid.
This is a generalisation of `ostree cat` to make it more useful.
This is a generalisation of `ostree checkout` to make it more useful.
Use:
ostree commit --tree=ref=:image:usr/bin
To get a commit that contains the contents of your bin directory at the
root. This is much faster than `ostree checkout --subdir ...` followed by
`ostree commit --tree=dir=...`.
You could implement usrmove with:
mkdir links
ln -s usr/bin links bin
ln -s usr/lib links lib
ostree commit --tree=prefix=usr/bin --tree=ref=:rootfs:bin \
--tree=prefix=lib --tree=ref=:rootfs:lib
We do similar things in our rootfs build system and this change makes thing
much faster.
There is a change in the output here. Previously for files in the repo each filename in the diff would be prefixed with /. For files on the filesystem there was no / prefix. Now there is never a / prefix. This is more self-consistent and is the same behaviour as `git diff --stat`.
This is a generalisation of ostree ls to be consistent with other places
we accept a treeish. It doesn't behave quite as you might expect: the
printed paths are always relative to the commit root rather than to the
treeish passed in. So:
ostree ls :rootfs:usr/bin systemd
Prints:
-00755 0 0 12345 /usr/bin/systemd
rather than
-00755 0 0 12345 systemd
as you might expect. I think it would be nice to change this behaviour,
but it would be backwards-incompatible so it can't happen.
If there is a commit the timestamp is set to the commit time - otherwise it's set to 1970-01.01T00:00Z. In the future `ostree export` could be taught to handle blobs as well.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wmanley The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@wmanley: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@wmanley: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
...in place of a commit sha where reasonable.
TODO
Comment on "000000000000000000000000" SHA check000...000 removed. I replaced it withdirmeta_0755_0_0g_file_query_infoin place ofg_file_query_file_typeg_assert (OSTREE_IS_REPO_FILE (root_gfile));0s to dummy SHA or replace it with something else. - replacedref:filenameargument toostree diffDescription
The motivation for this is to enhance
ostree commit --tree=refto allow composing subtrees. For example: in our build system we strip away everything not under/usras the last step when building containers. This PR (in combination with #1651) allows you to do:To do this. It's fast.
As I was going along I thought it would be useful to have this ability generally in the command line tools. So now many of them accept a treeish instead of a commit.
Previously these tools accepted a COMMIT as:
And now in addition:
So the idea is that you append
:PATHto a commit and you can select a tree. This is the same as the git syntax. As a ref may already contain a colon if the REMOTE is specified we disambiguate by requiring the remote to be specified, even if it is left empty.