After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 705973 - Fix up 'ostree show' and add 'ostree log'
Fix up 'ostree show' and add 'ostree log'
Status: RESOLVED FIXED
Product: ostree
Classification: Infrastructure
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: OSTree maintainer(s)
OSTree maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-08-14 09:49 UTC by Stef Walter
Modified: 2013-08-15 05:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Intelligible display for 'ostree show' (10.31 KB, patch)
2013-08-14 09:49 UTC, Stef Walter
reviewed Details | Review
If parsing an empty checksum variant return NULL (1.23 KB, patch)
2013-08-14 09:49 UTC, Stef Walter
none Details | Review
Add 'ostree log' command (5.89 KB, patch)
2013-08-14 09:49 UTC, Stef Walter
none Details | Review
Make ostree_checksum_bytes_peek() return NULL for empty checksum (1.34 KB, patch)
2013-08-14 09:51 UTC, Stef Walter
none Details | Review
Intelligible display for 'ostree show' (10.31 KB, patch)
2013-08-14 10:51 UTC, Stef Walter
reviewed Details | Review
Add ostree_repo_parent_commit() and ostree_repo_parent_commit_v() (3.24 KB, patch)
2013-08-14 10:52 UTC, Stef Walter
reviewed Details | Review
Add 'ostree log' command (5.81 KB, patch)
2013-08-14 10:52 UTC, Stef Walter
accepted-commit_now Details | Review
Intelligible display for 'ostree show' (10.35 KB, patch)
2013-08-14 19:37 UTC, Stef Walter
committed Details | Review
Add ostree_commit_get_parent() to get parent from variant (2.43 KB, patch)
2013-08-14 19:38 UTC, Stef Walter
committed Details | Review
Add 'ostree log' command (6.82 KB, patch)
2013-08-14 19:38 UTC, Stef Walter
committed Details | Review

Description Stef Walter 2013-08-14 09:49:41 UTC
When exploring an ostree repo and trying to understand the data
model having these basic commands make a big difference.
Comment 1 Stef Walter 2013-08-14 09:49:47 UTC
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
Comment 2 Stef Walter 2013-08-14 09:49:52 UTC
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.
Comment 3 Stef Walter 2013-08-14 09:49:57 UTC
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.
Comment 4 Stef Walter 2013-08-14 09:51:26 UTC
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.
Comment 5 Stef Walter 2013-08-14 10:51:05 UTC
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
Comment 6 Stef Walter 2013-08-14 10:52:00 UTC
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.
Comment 7 Stef Walter 2013-08-14 10:52:15 UTC
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.
Comment 8 Colin Walters 2013-08-14 15:49:04 UTC
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?
Comment 9 Colin Walters 2013-08-14 15:50:15 UTC
Review of attachment 251601 [details] [review]:

See earlier comments.
Comment 10 Colin Walters 2013-08-14 15:57:57 UTC
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()?
Comment 11 Colin Walters 2013-08-14 16:54:11 UTC
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?
Comment 12 Stef Walter 2013-08-14 19:37:53 UTC
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
Comment 13 Stef Walter 2013-08-14 19:38:17 UTC
Created attachment 251643 [details] [review]
Add ostree_commit_get_parent() to get parent from variant
Comment 14 Stef Walter 2013-08-14 19:38:36 UTC
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.
Comment 15 Stef Walter 2013-08-14 19:39:33 UTC
Thanks for the review. Made those fixes
Comment 16 Colin Walters 2013-08-15 00:51:23 UTC
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"
Comment 17 Colin Walters 2013-08-15 00:53:00 UTC
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.
Comment 18 Colin Walters 2013-08-15 01:15:32 UTC
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 19 Stef Walter 2013-08-15 04:51:18 UTC
Comment on attachment 251642 [details] [review]
Intelligible display for 'ostree show'

Attachment 251642 [details] pushed as 790132a - Intelligible display for 'ostree show'
Comment 20 Stef Walter 2013-08-15 05:04:27 UTC
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