GNOME Bugzilla – Bug 707727
Change the API for immutable trees to be based on OstreeRepoFile
Last modified: 2013-09-10 16:19:29 UTC
As commits have separate contents tree and a metadata tree to save space when xattrs, etc. are the same, it can be a bit cumbersome to carry both hashes around. The idea here is that once you write an mtree to the repo, you'll get back an OstreeRepoFile which is a handle to the immutable tree stored inside the repo, instead of hashes. You can then pass this around to other things that want a pair of content and metadata trees, like write_commit.
Created attachment 254403 [details] [review] repo-file: s/content_checksum/contents_checksum/ This is what we call it everywhere else, so just be consistent. It also lines up with metadata_checksum better.
Created attachment 254404 [details] [review] builtin-commit: Don't parse the parent's GVariant by hand Instead, use OstreeRepoFile as a handle for the parent commit. We need to add an accessor for the metadata checksum, as that hasn't been exposed before.
Created attachment 254405 [details] [review] repo: Drop the branch parameter from ostree_repo_commit It's unused. Make users explicitly write a ref if they want this; high-level convenience API will be introduced later.
Created attachment 254406 [details] [review] repo: Make the body parameter to ostree_repo_commit optional
Created attachment 254407 [details] [review] repo-file: Base OstreeRepoFile on trees instead of commits We want an OstreeRepoFile to be the way to represent a filesystem tree inside an ostree repository, so change ostree_repo_file_new_root to take two tree IDs, and add a convenience method to replace the commit usages.
Created attachment 254408 [details] [review] repo: Change the pairs of checksums to instead be based on OstreeRepoFiles We want an OstreeRepoFile to be the way to reference a "filesystem tree" that's stored in the repo, which is a combination of a DIR_TREE and a DIR_META. The idea is that once you write an mtree to the repo using ostree_repo_write_mtree, it becomes serialized and you get an OstreeRepoFile in return. Change any APIs that care about DIR_TREE / DIR_META checksums to care about OstreeRepoFiles instead, which right now is mostly is ostree_repo_write_commit.
Created attachment 254409 [details] [review] repo-libarchive: Apply commit modifiers to libarchive archives as well And document the libarchive methods as well, so we can pass a NULL commit modifier.
Review of attachment 254403 [details] [review]: Yep.
Review of attachment 254404 [details] [review]: Yes! Definitely better.
Review of attachment 254405 [details] [review]: Go go gadget code deletion!a
Review of attachment 254406 [details] [review]: Sure.
Review of attachment 254407 [details] [review]: Hmm. I forget exactly why I was caching the resolve error before. The code dates wayyy back, at least before Nov 2011 when I renamed the file. So basically what we want is that say if you call g_file_get_child () on an OstreeRepoFile, and then generic code (diff/checkout) calls e.g. g_file_query_info() on it, we correctly throw an error. I think that will happen with your new code too... But since you deleted the caching of the error, what's the difference now between ostree_repo_file_ensure_resolved() and do_resolve_nonroot() It looks like we call both.
Review of attachment 254408 [details] [review]: ::: src/libostree/ostree-repo-commit.c @@ +1665,3 @@ + + contents_checksum = ostree_checksum_from_bytes (contents_csum); + ostree_mutable_tree_set_contents_checksum (mtree, (char *) contents_csum); This looks wrong; don't you want to pass contents_checksum? contents_csum is the binary version. ::: src/libostree/ostree-repo.h @@ +311,3 @@ gboolean ostree_repo_write_mtree (OstreeRepo *self, OstreeMutableTree *mtree, + GFile **out_file, This one is a bit up in the air as to what it returns; you could quite sanely pass it over to generic code. But I lean a bit towards making this an OstreeRepoFile. But on the other hand, if you don't want to do that in this commit, that's fine by me. We can do it later. @@ +319,3 @@ const char *subject, const char *body, + GFile *root, This one *has* to be an OstreeRepoFile, so let's make it so? Shouldn't be too hard to force the few callers of _commit to cast.
Review of attachment 254409 [details] [review]: This looks like a nice cleanup.
Created attachment 254422 [details] [review] repo-file: Base OstreeRepoFile on trees instead of commits We want an OstreeRepoFile to be the way to represent a filesystem tree inside an ostree repository, so change ostree_repo_file_new_root to take two tree IDs, and add a convenience method to replace the commit usages.
Created attachment 254423 [details] [review] repo: Change the pairs of checksums to instead be based on OstreeRepoFiles We want an OstreeRepoFile to be the way to reference a "filesystem tree" that's stored in the repo, which is a combination of a DIR_TREE and a DIR_META. The idea is that once you write an mtree to the repo using ostree_repo_write_mtree, it becomes serialized and you get an OstreeRepoFile in return. Change any APIs that care about DIR_TREE / DIR_META checksums to care about OstreeRepoFiles instead, which right now is mostly is ostree_repo_write_commit.
Attachment 254403 [details] pushed as 8ac0f99 - repo-file: s/content_checksum/contents_checksum/ Attachment 254404 [details] pushed as f49ed9e - builtin-commit: Don't parse the parent's GVariant by hand Attachment 254405 [details] pushed as 1f8db2a - repo: Drop the branch parameter from ostree_repo_commit Attachment 254406 [details] pushed as db148cc - repo: Make the body parameter to ostree_repo_commit optional Let's push these for now.
Review of attachment 254422 [details] [review]: So if we create an OstreeRepoFile just from a tree, how do we implement g_file_get_parent ()? I think that's why I originally had it only take commits.
Review of attachment 254423 [details] [review]: ::: src/libostree/ostree-repo-commit.c @@ +1664,3 @@ goto out; + + contents_checksum = ostree_checksum_from_bytes (contents_csum); This is leaked. Use _ostree_checksum_inplace_from_bytes instead?
Created attachment 254545 [details] [review] repo-file: Base OstreeRepoFile on trees instead of commits We want an OstreeRepoFile to be the way to represent a filesystem tree inside an ostree repository. In order to do this, we need to drop the commit from an OstreeRepoFile, and make that go to callers. Switch all current users of ostree_repo_file_new_root to ostree_repo_read_commit, and make the actual constructor private.
Created attachment 254546 [details] [review] repo: Change the pairs of checksums to instead be based on OstreeRepoFiles We want an OstreeRepoFile to be the way to reference a "filesystem tree" that's stored in the repo, which is a combination of a DIR_TREE and a DIR_META. The idea is that once you write an mtree to the repo using ostree_repo_write_mtree, it becomes serialized and you get an OstreeRepoFile in return. Change any APIs that care about DIR_TREE / DIR_META checksums to care about OstreeRepoFiles instead, which right now is mostly is ostree_repo_write_commit.
Created attachment 254547 [details] [review] repo-libarchive: Apply commit modifiers to libarchive archives as well And document the libarchive methods as well, so we can pass a NULL commit modifier.
Created attachment 254548 [details] [review] repo: Make read_commit spit out a resolved commit ref as well
Created attachment 254549 [details] [review] repo: Make the optimization for reusing checksums clearer
Created attachment 254550 [details] [review] repo: Change the pairs of checksums to instead be based on OstreeRepoFiles We want an OstreeRepoFile to be the way to reference a "filesystem tree" that's stored in the repo, which is a combination of a DIR_TREE and a DIR_META. The idea is that once you write an mtree to the repo using ostree_repo_write_mtree, it becomes serialized and you get an OstreeRepoFile in return. Change any APIs that care about DIR_TREE / DIR_META checksums to care about OstreeRepoFiles instead, which right now is mostly is ostree_repo_write_commit.
Discussed this on IRC, had some small fixes, but all looks good to me aside from those.