Re: git notes: notes

Jeff King [off-list ref] writes:

Is splitting on blank lines an error? I don't think so. The original
format was never strictly defined, but given the --pretty=raw format, it
seems like a fairly obvious thing to do.

I am inclined to cut the notes output from --pretty=raw, and let callers
ask for them explicitly with --show-notes or something similar. We can
leave them on by default in the "normal" output. This will still break
scripts doing "git log | ./script", but I don't think we have ever
condoned that practice.

Sounds like a plan.

On Wed, Jan 20, 2010 at 12:59:38PM -0800, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

Is splitting on blank lines an error? I don't think so. The original
format was never strictly defined, but given the --pretty=raw format, it
seems like a fairly obvious thing to do.

I am inclined to cut the notes output from --pretty=raw, and let callers
ask for them explicitly with --show-notes or something similar. We can
leave them on by default in the "normal" output. This will still break
scripts doing "git log | ./script", but I don't think we have ever
condoned that practice.

Sounds like a plan.

We can start with this patch, which clears up Joey's problem.

-- >8 --
Subject: [PATCH] don't show notes for --pretty=raw

The --pretty=raw format of the log family is likely to be
used by scripts. Such scripts may parse the output into
records on blank lines, since doing so in the past has
always worked. However, with the recently added notes
output, such parsers will see an extra stanza for any
commits that have notes.

This patch turns off the notes output for the raw format to
avoid breaking such scripts.

Signed-off-by: Jeff King <redacted>
---
 pretty.c         |    2 +-
 t/t3301-notes.sh |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/pretty.c b/pretty.c
index 9001379..0674027 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1094,7 +1094,7 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 	if (fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
 		strbuf_addch(sb, '\n');
 
-	if (fmt != CMIT_FMT_ONELINE)
+	if (fmt != CMIT_FMT_ONELINE && fmt != CMIT_FMT_RAW)
 		get_commit_notes(commit, sb, encoding,
 				 NOTES_SHOW_HEADER | NOTES_INDENT);
 

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 1e34f48..4c3de9d 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh

@@ -147,4 +147,18 @@ test_expect_success 'show -m and -F notes' '
 	test_cmp expect-m-and-F output
 '
 
+cat >expect << EOF
+commit 15023535574ded8b1a89052b32673f84cf9582b8
+tree e070e3af51011e47b183c33adf9736736a525709
+parent 1584215f1d29c65e99c6c6848626553fdd07fd75
+author A U Thor <author@example.com> 1112912173 -0700
+committer C O Mitter <committer@example.com> 1112912173 -0700
+
+    4th
+EOF
+test_expect_success 'git log --pretty=raw does not show notes' '
+	git log -1 --pretty=raw >output &&
+	test_cmp expect output
+'
+
 test_done

-- 
1.6.6.510.g159cf

On Wed, Jan 20, 2010 at 04:31:38PM -0500, Jeff King wrote:

We can start with this patch, which clears up Joey's problem.

-- >8 --
Subject: [PATCH] don't show notes for --pretty=raw

The --pretty=raw format of the log family is likely to be
used by scripts. Such scripts may parse the output into
records on blank lines, since doing so in the past has
always worked. However, with the recently added notes
output, such parsers will see an extra stanza for any
commits that have notes.

This patch turns off the notes output for the raw format to
avoid breaking such scripts.

And the second half would be something like the patch below implementing
--show-notes, so that things like "gitk" which can handle the new
feature can turn it on.

I'm not that happy with this patch, though. If we have --show-notes, we
should probably have --no-show-notes, which this doesn't do, since a
lack of --show-notes simply means "do whatever the format dictates".

Also, passing everything through a pretty_print_context seems kind of
hack-ish, as we just end up copying items from the rev-list context into
the pp_context. I did so here only in the log_tree_commit case. I don't
think there are other places which would want to propagate it, but I
might have missed one. I wonder if we would do better to simply past the
rev-list options into pretty_print_commit and let it pick what it wants
out of the struct.

Anyway, I am out of time to work on git for now, so maybe somebody who
cares more about notes can pick this up. I think the first patch (the
one I am replying to) should definitely go in to un-break Joey's case
(or an alternative patch that turns it off in even more cases), and
people who care about it can fight about whether and by what mechanism
things like gitk should see the notes.

-- >8 --
Subject: [PATCH] teach log family to --show-notes

Most log formats will implicitly show commit notes, but the
raw format will not. Callers can now explicitly call
--show-notes to enable them.

Signed-off-by: Jeff King <redacted>
---
 commit.h         |    1 +
 log-tree.c       |    1 +
 pretty.c         |    3 ++-
 revision.c       |    2 ++
 revision.h       |    1 +
 t/t3301-notes.sh |   16 ++++++++++++++++
 6 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/commit.h b/commit.h
index 24128d7..4646751 100644
--- a/commit.h
+++ b/commit.h

@@ -71,6 +71,7 @@ struct pretty_print_context
 	enum date_mode date_mode;
 	int need_8bit_cte;
 	struct reflog_walk_info *reflog_info;
+	int show_notes;
 };
 
 extern int has_non_ascii(const char *text);

diff --git a/log-tree.c b/log-tree.c
index 0fdf159..9155a31 100644
--- a/log-tree.c
+++ b/log-tree.c

@@ -412,6 +412,7 @@ void show_log(struct rev_info *opt)
 	ctx.abbrev = opt->diffopt.abbrev;
 	ctx.after_subject = extra_headers;
 	ctx.reflog_info = opt->reflog_info;
+	ctx.show_notes = opt->show_notes;
 	pretty_print_commit(opt->commit_format, commit, &msgbuf, &ctx);
 
 	if (opt->add_signoff)

diff --git a/pretty.c b/pretty.c
index 0674027..95fe39a 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1094,7 +1094,8 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 	if (fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
 		strbuf_addch(sb, '\n');
 
-	if (fmt != CMIT_FMT_ONELINE && fmt != CMIT_FMT_RAW)
+	if (context->show_notes ||
+	    (fmt != CMIT_FMT_ONELINE && fmt != CMIT_FMT_RAW))
 		get_commit_notes(commit, sb, encoding,
 				 NOTES_SHOW_HEADER | NOTES_INDENT);
 

diff --git a/revision.c b/revision.c
index 7328201..3815dd3 100644
--- a/revision.c
+++ b/revision.c

@@ -1175,6 +1175,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 		get_commit_format("oneline", revs);
 		revs->abbrev_commit = 1;
+	} else if (!strcmp(arg, "--show-notes")) {
+		revs->show_notes = 1;
 	} else if (!strcmp(arg, "--graph")) {
 		revs->topo_order = 1;
 		revs->rewrite_parents = 1;

diff --git a/revision.h b/revision.h
index d368003..e51842f 100644
--- a/revision.h
+++ b/revision.h

@@ -83,6 +83,7 @@ struct rev_info {
 			abbrev_commit:1,
 			use_terminator:1,
 			missing_newline:1,
+			show_notes:1,
 			date_mode_explicit:1;
 	unsigned int	disable_stdin:1;
 

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 4c3de9d..d51e8bd 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh

@@ -161,4 +161,20 @@ test_expect_success 'git log --pretty=raw does not show notes' '
 	test_cmp expect output
 '
 
+cat >>expect <<EOF
+
+Notes:
+    spam
+$whitespace
+    xyzzy
+$whitespace
+    foo
+    bar
+    baz
+EOF
+test_expect_success 'git log --show-notes' '
+	git log -1 --pretty=raw --show-notes >output &&
+	test_cmp expect output
+'
+
 test_done

-- 
1.6.6.510.g159cf