Re: [PATCH v2] Fix notes handling in rev-list

Jeff King [off-list ref] writes:

On Mon, Jul 16, 2012 at 09:30:09PM +0300, Jukka Lehtniemi wrote:

quoted

@@ -111,6 +112,7 @@ static void show_commit(struct commit *commit, void *data)
 		ctx.date_mode = revs->date_mode;
 		ctx.date_mode_explicit = revs->date_mode_explicit;
 		ctx.fmt = revs->commit_format;
+		ctx.show_notes = revs->show_notes;
 		pretty_print_commit(&ctx, commit, &buf);
 		if (revs->graph) {
 			if (buf.len) {

Makes sense. We were just failing to propagate the show_notes flag to
the pretty-print code, as log-tree does.

quoted

@@ -159,6 +161,12 @@ static void show_commit(struct commit *commit, void *data)
 	} else {
 		if (graph_show_remainder(revs->graph))
 			putchar('\n');
+		if (revs->show_notes_given) {
+			struct strbuf buf = STRBUF_INIT;
+			format_display_notes(commit->object.sha1, &buf, 0, NOTES_SHOW_HEADER|NOTES_INDENT); 
+			fwrite(buf.buf, 1, buf.len, stdout);
+			strbuf_release(&buf);
+		}

But why are we using show_notes_given here instead of show_notes? The
former is about "did we get any kind of --notes option on the
command-line". So doing "git rev-list --no-notes" would trigger it,
which seems wrong. We should simply be checking show_notes again, no?

Also, it seems odd to me to show the notes after graph_show_remainder.
Your first hunk is about passing the notes option to the pretty-printer,
which handles graph output already, and looks like this:

  $ git rev-list --oneline --graph --notes -2 HEAD
  * f6bbb09 Fix notes handling in rev-list
  | Notes:
  |     foobar
  | 
  * 31c7954 Update draft release notes for 7th batch

Just like log, the notes are part of the commit information to the right
of the graph. But this second hunk is for when we are not using the
pretty-printer at all, and the output looks like this:

  $ git rev-list --graph --notes -2 HEAD
  * f6bbb09529a4cc73446c7c115ac1468477bd0cc6

  Notes:
      foobar
  * 31c79549b85c6393be4f40432f5b86ebc097fc7e

which doesn't make sense

I actually have quite a different feeling about this.  As I said in
the separate message, I think --graph, or anything that makes the
output unparsable or harder to parse for machines for that matter,
in rev-list are not something we have because we wanted to support
them, but that which just happen to work because the large part of
rev-list and log can share building blocks to do similar things.
The key phrase is "can share" here; it does not necessarily mean
they "should" [*1*].

First and foremost, rev-list is a tool for people who hate what our
vanilla "git log" Porcelain does enough that they want to write
their own Porcelain scripts using it.

I do not mind having an option to show the notes text, but I doubt
it is a sane thing to do to make "rev-list --notes" unconditionally
show the payload of the notes blob.  "rev-list --objects" only shows
the object names of trees and blobs, not the payload in these
objects, and this is very much on purpuse.  It allows the downstream
process that reads its output from the pipe to easily parse the
output and choose to do whatever it wants to do using them.

I wonder if we should show the blob object names that store the
notes payload if we were given --notes option in a format that is
easy for readers to mechanically parse its output.

In any case, the use of format_display_notes() that is meant for
human consumption feels very wrong to me, especially it seems to be
placed outside the "if (revs->verbose_header && commit->buffer)"
block in this patch.  I do not have any problem if the patch makes
the notes text shown in the other side of the if block that uses
pretty_print_commit(), though.


[Footnote]

*1* A simple litmus test is to ask this question: if somebody comes
    up with a "better" way to show the same output for the option,
    would we accept that update without worrying about breaking
    existing scripts?  If the answer is yes, that is a secondary
    feature in the context of "rev-list" plumbing like --graph is.

First of all, thanks for you feedback, both of you. And sorry for
wasting your time . I thought that the "In-Reply-To"-header would
serve as a reference to the original patch but obviously it wasn't
enough.

On Tue, Jul 17, 2012 at 8:42 AM, Junio C Hamano [off-list ref] wrote:

I wonder if we should show the blob object names that store the
notes payload if we were given --notes option in a format that is
easy for readers to mechanically parse its output.

Very good point indeed. I think this is how it should be. How would
you prefer the output format to be? Would e.g.

    189899d229ec Notes: 888ecad77e88

be ok?

In any case, the use of format_display_notes() that is meant for
human consumption feels very wrong to me, especially it seems to be
placed outside the "if (revs->verbose_header && commit->buffer)"
block in this patch.  I do not have any problem if the patch makes
the notes text shown in the other side of the if block that uses
pretty_print_commit(), though.

I think that another place where human readable notes should be shown
is inside the graph.

-- 
Jukka

On Mon, Jul 16, 2012 at 10:42:07PM -0700, Junio C Hamano wrote:

quoted

Just like log, the notes are part of the commit information to the right
of the graph. But this second hunk is for when we are not using the
pretty-printer at all, and the output looks like this:

  $ git rev-list --graph --notes -2 HEAD
  * f6bbb09529a4cc73446c7c115ac1468477bd0cc6

  Notes:
      foobar
  * 31c79549b85c6393be4f40432f5b86ebc097fc7e

which doesn't make sense

I actually have quite a different feeling about this.  As I said in
the separate message, I think --graph, or anything that makes the
output unparsable or harder to parse for machines for that matter,
in rev-list are not something we have because we wanted to support
them, but that which just happen to work because the large part of
rev-list and log can share building blocks to do similar things.
The key phrase is "can share" here; it does not necessarily mean
they "should" [*1*].

Somebody went to the trouble to make "rev-list --graph" work[1] (that is
what the call to graph_show_remainder in the else clause of the
conditional is about). I agree it seems kind of useless, but it does
work now, and we should at least not make it worse (and I think we both
agree that the output above is just wrong).

So whatever we show for a note, it should look like:

  * f6bbb095...
  | the notes thing to show
  * 31c79549...

Because that is how graph output is formatted. Either that, or we should
disallow --graph entirely with rev-list (which I'd also be OK with).

I do not mind having an option to show the notes text, but I doubt
it is a sane thing to do to make "rev-list --notes" unconditionally
show the payload of the notes blob.  "rev-list --objects" only shows
the object names of trees and blobs, not the payload in these
objects, and this is very much on purpuse.  It allows the downstream
process that reads its output from the pipe to easily parse the
output and choose to do whatever it wants to do using them.

I wonder if we should show the blob object names that store the
notes payload if we were given --notes option in a format that is
easy for readers to mechanically parse its output.

So leaving aside the --graph issues, we would need to decide what to
show in the non-graph case. And I think your suggestion is good; there
is no real need to dereference the blob (if you want that, you can turn
on the pretty-printer).

I'm just not sure what the output should be. I guess:

  <commit_sha1> <notes sha1s>

is probably the most sensible (it's sort of like --parents). And that
solves the --graph issue, too, since it continues to take only a single
line.

-Peff

[1] Looking at the code, I do think somebody wanted "rev-list --graph"
to work, and it is not an accident. But I think they did not do a very
thorough job, as things like "git rev-list --objects --graph" produce
nonsensical output.