GNOME Bugzilla – Bug 705973
Fix up 'ostree show' and add 'ostree log'
Last modified: 2013-08-15 05:04:35 UTC
When exploring an ostree repo and trying to understand the data model having these basic commands make a big difference.
Created attachment 251589 [details] [review] Intelligible display for 'ostree show' Show something similar to git metadata display. Show raw variant data when --raw is specified
Created attachment 251590 [details] [review] If parsing an empty checksum variant return NULL This is useful when parsing the parent checksum of a commit. The first checksum has a zero length.
Created attachment 251591 [details] [review] Add 'ostree log' command Follows the parent of commits showing each in turn until it reaches the top of the commit tree.
Created attachment 251592 [details] [review] Make ostree_checksum_bytes_peek() return NULL for empty checksum This is useful when parsing the parent checksum of a commit. The first checksum has a zero length. Also prevents making assumptions that the returned data has the expected length in these cases.
Created attachment 251601 [details] [review] Intelligible display for 'ostree show' Show something similar to git metadata display. Show raw variant data when --raw is specified
Created attachment 251602 [details] [review] Add ostree_repo_parent_commit() and ostree_repo_parent_commit_v() These get the parent commit checksum of a commit, or loaded commit variant.
Created attachment 251603 [details] [review] Add 'ostree log' command Follows the parent of commits showing each in turn until it reaches the top of the commit tree.
Review of attachment 251589 [details] [review]: ::: src/libostree/ostree-core.h @@ +81,3 @@ * s - subject * s - body + * t - Timestamp in seconds since the epoch (UTC) (big endian) All integers are stored big endian; I just documented this a bit here: https://git.gnome.org/browse/ostree/commit/?id=61773f6ca4f6029be016d573fcc661c6e2bfc63e So this hunk should be unnecessary. ::: src/ostree/ot-builtin-show.c @@ +138,3 @@ + if (!ostree_repo_load_variant (repo, objtype, checksum, + &variant, error)) + return FALSE; Can you make this look like all the other functions with "gboolean ret, goto out;" style? While I know the style here is shorter, I really like the consistency of having pretty much every function flow the same, even if the code looks cleaner with early returns. I know it may feel unnatural at first, but at the moment: $ git grep 'goto out' | wc -l 783 $ ::: src/ostree/ot-dump.c @@ +102,3 @@ + * ay - Root tree contents + * ay - Root tree metadata + */ Can we just say like "see ostree-core.h" or something to avoid having multiple copies of this?
Review of attachment 251601 [details] [review]: See earlier comments.
Review of attachment 251602 [details] [review]: ::: src/libostree/ostree-repo-traverse.c @@ +232,3 @@ +gboolean +ostree_repo_parent_commit (OstreeRepo *repo, This function isn't used by this patch; I assume later ones will. But is it really worth having an API for this over just having callers inline it? Most of the time I assume they'll want to load the commit variant anyways. @@ +251,3 @@ + +gchar * +ostree_repo_parent_commit_v (GVariant *commit_variant) Since this doesn't take the repo as a parameter, it should be part of ostree-core.h probably. Something like: ostree_commit_get_parent()?
Review of attachment 251603 [details] [review]: Only one minor bit. ::: src/ostree/ot-builtin-log.c @@ +97,3 @@ + else + { + gs_unref_variant GVariant *variant = NULL; Unused?
Created attachment 251642 [details] [review] Intelligible display for 'ostree show' Show something similar to git metadata display. Show raw variant data when --raw is specified
Created attachment 251643 [details] [review] Add ostree_commit_get_parent() to get parent from variant
Created attachment 251644 [details] [review] Add 'ostree log' command Follows the parent of commits showing each in turn until it reaches the top of the commit tree.
Thanks for the review. Made those fixes
Review of attachment 251642 [details] [review]: One minor potential use of the internal test API, otherwise looks good! (Feel free to commit after adjusting) ::: tests/test-basic.sh @@ +255,3 @@ +if ! grep -q "Third commit" show-output; then + echo "show didn't contain message"; exit 1 +fi assert_file_has_content show-output "Third commit" @@ +258,3 @@ +if ! grep -q "commit $checksum" show-output; then + echo "show didn't contain checksum"; exit 1 +fi assert_file_has_content show-output "commit $checksum"
Review of attachment 251643 [details] [review]: Feel free to keep as is, or adjust for the minor comment. ::: src/libostree/ostree-core.c @@ +1641,3 @@ +{ + gs_unref_variant GVariant *bytes = NULL; + g_variant_get_child (commit_variant, 1, "@ay", &bytes); bytes = g_variant_get_child_value (commit_variant, 1); is a bit shorter, but I have no objections to this form.
Review of attachment 251644 [details] [review]: ::: src/ostree/ot-builtin-log.c @@ +97,3 @@ + { + checksum = g_strdup (rev); + } This part is unnecessary; ostree_repo_resolve_rev() does it internally. ::: tests/test-basic.sh @@ +267,3 @@ +if ! grep -q "First commit" log-output; then + echo "log didn't first message"; exit 1 +fi You can use assert_file_has_content here too.
Comment on attachment 251642 [details] [review] Intelligible display for 'ostree show' Attachment 251642 [details] pushed as 790132a - Intelligible display for 'ostree show'
Attachment 251643 [details] pushed as 5efb8e8 - Add ostree_commit_get_parent() to get parent from variant Attachment 251644 [details] pushed as 3989e0d - Add 'ostree log' command