[PATCH] Initialize notes trees if %N is used and no --show-notes given

Signed-off-by: Johannes Gilger <redacted>
---
Hi list,

this bug bit me when I used 'git log --format="%N"' without adding
--show-notes, which caused git to fail an assertion:
 Assertion failed: (display_notes_trees), function format_display_notes, file notes.c, line 1186.

While this patch fixes this behaviour, I'm not sure it's at the right
place or doesn't impact performance. So this is meant more as a
bug-report.

Greetings,
Jojo

 notes.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/notes.c b/notes.c
index e425e19..83f39ae 100644
--- a/notes.c
+++ b/notes.c

@@ -1183,6 +1183,8 @@ void format_display_notes(const unsigned char *object_sha1,
 			  struct strbuf *sb, const char *output_encoding, int flags)
 {
 	int i;
+	if (!display_notes_trees)
+		init_display_notes(NULL);
 	assert(display_notes_trees);
 	for (i = 0; display_notes_trees[i]; i++)
 		format_note(display_notes_trees[i], object_sha1, sb,

-- 
1.7.0.4.360.g11766c

On Mon, Apr 05, 2010 at 01:55:48PM +0200, Johannes Gilger wrote:

I'm not sure if it is right to just pass NULL. We shouldn't have any
extra_refs in our display_notes_opt, because we would have had to
pass --show-notes to do so (at least from my brief reading of the code).

But shouldn't "git show --no-standard-notes --format=%N" pass a
display_notes_opt with suppress_default_notes set?

I don't see it as all that likely (since without --show-notes, you
wouldn't have _any_ notes, so why are you using %N?), but it seems to be
the correct behavior, and might be useful for a script that uses '%N' in
combination with user-provided options.

-Peff

[A Cc would have been nice, I nearly missed this but it's clearly my
bug.]

Johannes Gilger wrote:

this bug bit me when I used 'git log --format="%N"' without adding
--show-notes, which caused git to fail an assertion:
 Assertion failed: (display_notes_trees), function format_display_notes, file notes.c, line 1186.

[...]

Thanks for the report.  Unfortunately this returns to the
silently-initialize-with-NULL case that was I explicitly asked to
avoid.

I see three options:
- %N could simply expand to nothing if notes are disabled
- %N could silently initialize as above
- your patch

though for your patch, I'd also remove the assert() since it's
basically there to enforce the requirement of initializing them; the
trees list can never be NULL after init_display_notes().

Currently I think the first option would be the best, since
(notionally; we still don't have all the bits AFAIK) the built-in
formats can then be written with a %N at the right place, without
having to worry about the other command line options.  I haven't had
enough coffee to think about any possible ill side effects, though.


-- 
Thomas Rast
trast@{inf,student}.ethz.ch

On 06/04/10 11:27, Thomas Rast wrote:

[A Cc would have been nice, I nearly missed this but it's clearly my
bug.]

Sorry for that, haven't mailed to git-ml for quite a while ;)

I see three options:
- %N could simply expand to nothing if notes are disabled
- %N could silently initialize as above
- your patch

The first option would be confusing. I, for one, would simply put %N in
my log and never really know that existing notes aren't displayed. I
wasn't even sure my git.git checkout had notes, so I created one myself.
A better behaviour would be to not expand %N if notes are disabled, so a
user gets some kind of feedback that %N isn't working.

I'd really like %N to do the initialization. There is no other
placeholder which requires an extra option to work, if I see it
correctly.

As for the builtin formats I was under the impressions that they worked
completely outside the parser for placeholders, so one would not use
'%N' in a builtin format, and %N initializing the notes would not
conflict with --no-notes and builtin formats.

though for your patch, I'd also remove the assert() since it's
basically there to enforce the requirement of initializing them; the
trees list can never be NULL after init_display_notes().

Sure.

Greetings,
Jojo

-- 
Johannes Gilger [off-list ref]
http://heipei.net
GPG-Key: 0xD47A7FFC
GPG-Fingerprint: 5441 D425 6D4A BD33 B580  618C 3CDC C4D0 D47A 7FFC

Johannes Gilger wrote:

The first option would be confusing. I, for one, would simply put %N in
my log and never really know that existing notes aren't displayed. I
wasn't even sure my git.git checkout had notes, so I created one myself.
A better behaviour would be to not expand %N if notes are disabled, so a
user gets some kind of feedback that %N isn't working.

I'd really like %N to do the initialization. There is no other
placeholder which requires an extra option to work, if I see it
correctly.

%g[dDs] expand to nothing unless the log command walks reflogs, so
there is some precedent.

One thing I didn't consider in my other mail was that --pretty
automatically disables notes.  I think in my plan (%N expands to
nothing with --no-notes) this would have to change to the effect that
--pretty only disables the *normal* note-showing code, but still
initializes according to the same rules.

I'll have to check whether that amounts to the same as "silent
initialization".

As for the builtin formats I was under the impressions that they worked
completely outside the parser for placeholders, so one would not use
'%N' in a builtin format, and %N initializing the notes would not
conflict with --no-notes and builtin formats.

That's true, which is why I said "notionally".

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

On Tue, Apr 06, 2010 at 01:52:21PM +0200, Thomas Rast wrote:

quoted

I'd really like %N to do the initialization. There is no other
placeholder which requires an extra option to work, if I see it
correctly.

%g[dDs] expand to nothing unless the log command walks reflogs, so
there is some precedent.

%d loads decorations on demand, so there is some precedent the other
%way, too. I don't personally have a preference, though.

-Peff

Thomas Rast [off-list ref] writes:

quoted

I'd really like %N to do the initialization. There is no other
placeholder which requires an extra option to work, if I see it
correctly.

%g[dDs] expand to nothing unless the log command walks reflogs, so
there is some precedent.

As Peff pointed out, %d does things lazily, but I suspect it might be hard
to do a similar initialization for %N.

I wonder if we can inspect-but-not-use format string before we even start
walking, to see if we need notes (when we see %N).

None of the abouve applies to %g because making it cause reflog walking
will change not only the output, but the fundamental behaviour of the
command.

On Tue, Apr 06, 2010 at 11:18:34PM -0700, Junio C Hamano wrote:

As Peff pointed out, %d does things lazily, but I suspect it might be hard
to do a similar initialization for %N.

I think you would just have to stuff the notes-related options from the
rev-list options into the pretty-print context, which would then make
them available to the user-format callback.

I wonder if we can inspect-but-not-use format string before we even start
walking, to see if we need notes (when we see %N).

I have considered something like the patch below before, but it is not
100% accurate. Part of the parsing happens in strbuf_expand, but parsing
of things like %w(...) happens ad-hoc inside the formatting callback (so
we would see "%w(%N)" as wanting notes, when it doesn't really. In
theory it would be nicer if we separated syntax and semantics, so I
could parse %X(...) as "the %X placeholder with ... as arguments"
without having to actually understand what %X does. In practice, it
doesn't matter here because we don't have very many placeholders that
take arbitrary arguments.

The patch below is totally untested and just meant to illustrate the
approach. Use caution.

diff --git a/commit.h b/commit.h
index 2b7fd89..5081389 100644
--- a/commit.h
+++ b/commit.h

@@ -74,11 +74,16 @@ struct pretty_print_context
 	struct reflog_walk_info *reflog_info;
 };
 
+struct userformat_want {
+	unsigned notes:1;
+};
+
 extern int has_non_ascii(const char *text);
 struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
 extern char *reencode_commit_message(const struct commit *commit,
 				     const char **encoding_p);
 extern void get_commit_format(const char *arg, struct rev_info *);
+extern void userformat_fill_want(const char *format, struct userformat_want *w);
 extern void format_commit_message(const struct commit *commit,
 				  const char *format, struct strbuf *sb,
 				  const struct pretty_print_context *context);

diff --git a/pretty.c b/pretty.c
index 6ba3da8..8ed3d36 100644
--- a/pretty.c
+++ b/pretty.c

@@ -855,6 +855,24 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	return consumed + 1;
 }
 
+static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
+				 void *context)
+{
+	struct userformat_want *w = context;
+	switch (*placeholder) {
+		case 'N': w->notes = 1;
+	}
+	return 0;
+}
+
+void userformat_fill_want(const char *format, struct userformat_want *w)
+{
+	struct strbuf dummy = STRBUF_INIT;
+	memset(w, 0, sizeof(*w));
+	strbuf_expand(&dummy, format, userformat_want_item, w);
+	strbuf_release(&dummy);
+}
+
 void format_commit_message(const struct commit *commit,
 			   const char *format, struct strbuf *sb,
 			   const struct pretty_print_context *pretty_ctx)

The %N placeholder will only work if --show-notes was provided to log.
By not expanding the user is given feedback that he won't be shown any
notes.

Signed-off-by: Johannes Gilger <redacted>
---
 Ok, this is another stab. I don't really know whether we want %N to expand to
 an empty string or not expand at all in case of no --show-notes. Obviously
 using 'return 1;' would implement the former behaviour, while I chose the
 latter because it prevents people like me from building useless log aliases.

 Documentation/pretty-formats.txt |    3 ++-
 pretty.c                         |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 1686a54..bf7813f 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt

@@ -143,7 +143,8 @@ NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
 insert an empty string unless we are traversing reflog entries (e.g., by
 `git log -g`). The `%d` placeholder will use the "short" decoration
-format if `--decorate` was not already provided on the command line.
+format if `--decorate` was not already provided on the command line. The %N
+placeholder won't be expanded unless `--show-notes` was provided.
 
 If you add a `{plus}` (plus sign) after '%' of a placeholder, a line-feed
 is inserted immediately before the expansion if and only if the

diff --git a/pretty.c b/pretty.c
index 6ba3da8..b39e2d5 100644
--- a/pretty.c
+++ b/pretty.c

@@ -775,6 +775,9 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		}
 		return 0;	/* unknown %g placeholder */
 	case 'N':
+		if (!c->pretty_ctx->show_notes)
+			return 0;
+
 		format_display_notes(commit->object.sha1, sb,
 			    git_log_output_encoding ? git_log_output_encoding
 						    : git_commit_encoding, 0);

-- 
1.7.0.2.201.g80978