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

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

[Johannes Gilger]

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
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

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

[Jeff King]

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

> 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.
> [...]
> --- 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,

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

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

[Thomas Rast]

[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.
[...]
> 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);

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

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

[Johannes Gilger]

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 <heipei@hackvalue.de>
http://heipei.net
GPG-Key: 0xD47A7FFC
GPG-Fingerprint: 5441 D425 6D4A BD33 B580  618C 3CDC C4D0 D47A 7FFC

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

[Thomas Rast]

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

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

[Jeff King]

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

> > 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

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

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

>> 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.

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

[Jeff King]

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)

[PATCH] pretty.c: Don't expand %N without --show-notes

[Johannes Gilger]

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 <heipei@hackvalue.de>
---
 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

Re: [PATCH] pretty.c: Don't expand %N without --show-notes

[Junio C Hamano]

Johannes Gilger <heipei@hackvalue.de> writes:

> 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.

I kind of like the simplicity of this approach, but this probably cuts
both ways.  I can hear some users screaming "But but but I already told
you that I am interested in notes---how else would I say %N in the format
string?" while others say "Yeah, that way I can keep using the same format
and when I don't give --show-notes it won't show extra information in the
output.  Very nice".

> 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

[PATCH] Notes: Connect the %N flag to --{show,no}-notes

[Johannes Gilger]

This improves the behaviour of the %N flag when using --pretty:

- If only --pretty is used, notes will only be initialized if %N is
  actually part of the format.
- If %N and --no-notes is used, %N will expand to the empty string.
- Behaviour for regular log with --no-notes and --show-notes remains the
  same.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
I reread a lot of code and think I've found a solution that's intuitive and
doesn't automatically initialize the trees with --pretty unless %N is actually
used. Basically what could happen is this:

- --show-notes (with eventual options) is supplied and
  init_display_notes(&rev.notes_opt) is called from log.c, %N is sucessfully
  expanded

- Only %N is used, no --no-notes was supplied. format_display_notes is called,
  notices the missing notes_trees and initializes it. this obviously happens
  only if an %N is used, otherwise no display trees will be initialized.
  init_display_notes could not have been called with any other options, since
  no --show-notes was given, so NULL is fine here.

- %N is used and --no-notes is supplied, format_display_notes is not
  called and %N is expanded to the empty string. This last case also
  applies to --pretty=full etc.

I tested a lot of cases and tried to make sure I understood all the possible
caveats. If you have any further questions feel free to ask.

 builtin/log.c |    4 ++--
 notes.c       |    4 +++-
 pretty.c      |    7 ++++---
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b706a5f..029d7b8 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -58,9 +58,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
-	if (!rev->show_notes_given && !rev->pretty_given)
+	if (!rev->show_notes_given)
 		rev->show_notes = 1;
-	if (rev->show_notes)
+	if (rev->show_notes && (!rev->pretty_given || rev->show_notes_given))
 		init_display_notes(&rev->notes_opt);
 
 	if (rev->diffopt.pickaxe || rev->diffopt.filter)
diff --git a/notes.c b/notes.c
index e425e19..ad14a8b 100644
--- a/notes.c
+++ b/notes.c
@@ -1183,7 +1183,9 @@ void format_display_notes(const unsigned char *object_sha1,
 			  struct strbuf *sb, const char *output_encoding, int flags)
 {
 	int i;
-	assert(display_notes_trees);
+	if(!display_notes_trees)
+		init_display_notes(NULL);
+
 	for (i = 0; display_notes_trees[i]; i++)
 		format_note(display_notes_trees[i], object_sha1, sb,
 			    output_encoding, flags);
diff --git a/pretty.c b/pretty.c
index 6ba3da8..8e828a1 100644
--- a/pretty.c
+++ b/pretty.c
@@ -775,9 +775,10 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		}
 		return 0;	/* unknown %g placeholder */
 	case 'N':
-		format_display_notes(commit->object.sha1, sb,
-			    git_log_output_encoding ? git_log_output_encoding
-						    : git_commit_encoding, 0);
+		if (c->pretty_ctx->show_notes)
+			format_display_notes(commit->object.sha1, sb,
+					     git_log_output_encoding ? git_log_output_encoding
+					     : git_commit_encoding, 0);
 		return 1;
 	}
 
-- 
1.7.0.2.201.g80978

Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes

[Junio C Hamano]

Johannes Gilger <heipei@hackvalue.de> writes:

> diff --git a/builtin/log.c b/builtin/log.c
> index b706a5f..029d7b8 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -58,9 +58,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
>  		usage(builtin_log_usage);
>  	argc = setup_revisions(argc, argv, rev, opt);
>  
> -	if (!rev->show_notes_given && !rev->pretty_given)
> +	if (!rev->show_notes_given)
>  		rev->show_notes = 1;

I am puzzled by this change and its possible interaction with codepaths
that do not have anything to do with %N.  When there is no show-notes and
an explicit --pretty, we do not want to have rev->show_notes set.

Admittedly, the real end result we want to see in such a case is just that
notes are not shown (and rev->show_notes being false is one natural way to
achieve that), and if ...

> -	if (rev->show_notes)
> +	if (rev->show_notes && (!rev->pretty_given || rev->show_notes_given))
>  		init_display_notes(&rev->notes_opt);

... this change is about ensuring the same outcome by not initializing the
notes tree, that may work, but it somehow feels iffy.  It would leave some
codepaths (and another one you just added, I think, with the other hunk in
this patch) that say "do this only when rev->show_notes is set" and some
other codepaths that say "unconditionally try to show notes and rely on
the caller not have initialized the notes tree when it is not wanted."  Is
that what is going on?

Unfortunately I don't think of a better and cleaner solution offhand
(perhaps such a cleaner solution would involve adding a bit more state in
the rev structure, but I haven't thought things through).

> diff --git a/notes.c b/notes.c
> index e425e19..ad14a8b 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -1183,7 +1183,9 @@ void format_display_notes(const unsigned char *object_sha1,
>  			  struct strbuf *sb, const char *output_encoding, int flags)
>  {
>  	int i;
> -	assert(display_notes_trees);
> +	if(!display_notes_trees)
> +		init_display_notes(NULL);
> +
>  	for (i = 0; display_notes_trees[i]; i++)
>  		format_note(display_notes_trees[i], object_sha1, sb,
>  			    output_encoding, flags);
> diff --git a/pretty.c b/pretty.c
> index 6ba3da8..8e828a1 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -775,9 +775,10 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
>  		}
>  		return 0;	/* unknown %g placeholder */
>  	case 'N':
> -		format_display_notes(commit->object.sha1, sb,
> -			    git_log_output_encoding ? git_log_output_encoding
> -						    : git_commit_encoding, 0);
> +		if (c->pretty_ctx->show_notes)
> +			format_display_notes(commit->object.sha1, sb,
> +					     git_log_output_encoding ? git_log_output_encoding
> +					     : git_commit_encoding, 0);
>  		return 1;
>  	}
>  
> -- 
> 1.7.0.2.201.g80978

Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes

[Jeff King]

On Sat, Apr 10, 2010 at 02:51:55PM -0700, Junio C Hamano wrote:

> > +++ b/builtin/log.c
> > @@ -58,9 +58,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
> >  		usage(builtin_log_usage);
> >  	argc = setup_revisions(argc, argv, rev, opt);
> >  
> > -	if (!rev->show_notes_given && !rev->pretty_given)
> > +	if (!rev->show_notes_given)
> >  		rev->show_notes = 1;
> 
> I am puzzled by this change and its possible interaction with codepaths
> that do not have anything to do with %N.  When there is no show-notes and
> an explicit --pretty, we do not want to have rev->show_notes set.

Could we perhaps just do:

  if (!rev->show_notes_given &&
      (!rev->pretty_given ||
       (rev->commit_format == CMIT_FMT_USERFORMAT && fmt_wants_notes(...))

where fmt_wants_notes is similar to what I posted earlier, or even just
strstr(fmt, "%N")? As I discussed earlier, it is not 100% accurate, but
it is extremely unlikely for it to be wrong, and when it is, we will
load notes when we don't need to. Which is an optimization failure, but
not a correctness failure.

And then just guard the '%N' placeholder by checking show_notes. That
will protect random codepaths that call format_commit_message() but
aren't log (they can't trigger an assert, but will just get '%N'
unexpanded or whatever). And doing:

  git log --no-notes --format='%N'

should also just fail to expand %N. Which is maybe a little crazy, but
what the user is asking for is crazy, and it makes the most sense to me.

-Peff

[PATCH] pretty: Initialize notes if %N is used

[Johannes Gilger]

When using git log --pretty='%N' without an explicit --show-notes, git
would segfault. This patches fixes this behaviour by loading the needed
notes datastructures if --pretty is used and the format contains %N.
When --pretty='%N' is used together with --no-notes, %N won't be
expanded.

This is an extension to a proposed patch by Jeff King.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
Hey Jeff,

something like this? I didn't see why userformat_fill_want had to have an extra
argument for the format, since the user_format variable is static in pretty.c.

Sorry for the many very different patches to the bug, as you can see I'm not
really familiar with best-practices in git.git.

Greetings,
Jojo

 builtin/log.c |    6 +++++-
 commit.h      |    5 +++++
 pretty.c      |   31 +++++++++++++++++++++++++++----
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b706a5f..f8f5d22 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -58,7 +58,11 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
-	if (!rev->show_notes_given && !rev->pretty_given)
+	struct userformat_want w;
+	if (rev->commit_format == CMIT_FMT_USERFORMAT)
+		userformat_fill_want(&w);
+
+	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
 		rev->show_notes = 1;
 	if (rev->show_notes)
 		init_display_notes(&rev->notes_opt);
diff --git a/commit.h b/commit.h
index 3cf5166..fc1c504 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(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..0e3ae98 100644
--- a/pretty.c
+++ b/pretty.c
@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		}
 		return 0;	/* unknown %g placeholder */
 	case 'N':
-		format_display_notes(commit->object.sha1, sb,
-			    git_log_output_encoding ? git_log_output_encoding
-						    : git_commit_encoding, 0);
-		return 1;
+		if (c->pretty_ctx->show_notes) {
+			format_display_notes(commit->object.sha1, sb,
+				    git_log_output_encoding ? git_log_output_encoding
+							    : git_commit_encoding, 0);
+			return 1;
+		}
+		return 0;
 	}
 
 	/* For the rest we have to parse the commit header. */
@@ -855,6 +858,26 @@ 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(struct userformat_want *w)
+{
+	if (!user_format)
+		return;
+	struct strbuf dummy = STRBUF_INIT;
+	memset(w, 0, sizeof(*w));
+	strbuf_expand(&dummy, user_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)
-- 
1.7.0.2.201.g80978

Re: [PATCH] pretty: Initialize notes if %N is used

[Jeff King]

On Sun, Apr 11, 2010 at 04:54:22PM +0200, Johannes Gilger wrote:

> something like this? I didn't see why userformat_fill_want had to have an extra
> argument for the format, since the user_format variable is static in pretty.c.

Yes, this is getting closer.

The reason to take the format argument is that there are places which
call format_commit_message() with an arbitrary string. We would want
them to be able to call userformat_fill_want(), too (right now, I don't
think any of them should need it, though).

Maybe it should take a format string, and use the user_format string if
you pass NULL?

> Sorry for the many very different patches to the bug, as you can see I'm not
> really familiar with best-practices in git.git.

Not at all. Sometimes seemingly simple bugs end up raising a whole host
of other issues. I am glad you are sticking around to help come up with
a good solution.

> diff --git a/builtin/log.c b/builtin/log.c
> index b706a5f..f8f5d22 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -58,7 +58,11 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
>  		usage(builtin_log_usage);
>  	argc = setup_revisions(argc, argv, rev, opt);
>  
> -	if (!rev->show_notes_given && !rev->pretty_given)
> +	struct userformat_want w;

Don't declare variables in the middle of a function. It's a C99-ism that
we avoid for older compilers.

> +	if (rev->commit_format == CMIT_FMT_USERFORMAT)
> +		userformat_fill_want(&w);
> +
> +	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
>  		rev->show_notes = 1;

Hmm. If we didn't get a userformat, what will be in w? It will be random
cruft from the stack, because we didn't call userformat_fill_want.

You can just call it unconditionally, and it should do the right thing
with a NULL user_format.

> +void userformat_fill_want(struct userformat_want *w)
> +{
> +	if (!user_format)
> +		return;
> +	struct strbuf dummy = STRBUF_INIT;
> +	memset(w, 0, sizeof(*w));
> +	strbuf_expand(&dummy, user_format, userformat_want_item, w);
> +	strbuf_release(&dummy);
> +}

This does nothing with a NULL user_format. It should probably still do
the memset() to indicate that nothing is wanted (otherwise the caller
has to be sure to initialize w themselves if they think user_format
might be NULL).

So combined with my above suggestion:

  void userformat_fill_want(const char *s, struct userformat_want *w)
  {
          struct strbuf dummy = STRBUF_INIT;
          memset(w, 0, sizeof(*w));
          if (!s) {
                  if (!user_format)
                          return;
                  s = user_format;
          }
          strbuf_expand(&dummy, user_format, userformat_want_item, w);
          strbuf_release(&dummy);
  }

and then you can just call

  userformat_fill_want(NULL, w);

safely from log.c (you don't even need to check rev->commit_format).

Also, even though I picked the name, userformat_fill_want is kind of a
lousy name. It was the best I could come up with, but maybe somebody has
a better suggestion.

-Peff

[PATCHv2] pretty: Initialize notes if %N is used

[Johannes Gilger]

When using git log --pretty='%N' without an explicit --show-notes, git
would segfault. This patches fixes this behaviour by loading the needed
notes datastructures if --pretty is used and the format contains %N.
When --pretty='%N' is used together with --no-notes, %N won't be
expanded.

This is an extension to a proposed patch by Jeff King.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
Thanks for the feedback Jeff. I've put your suggestions into my patch and tried
to come up with a sensible name for 'userformat_fill_want'. As you can see I
called it 'userformat_find_requirements', but am not really satisfied with it
since it's too long and not quite to the point.

Anything else missing?

Greetings, Jojo

 builtin/log.c |    5 ++++-
 commit.h      |    5 +++++
 pretty.c      |   36 ++++++++++++++++++++++++++++++++----
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b706a5f..dd8e996 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 {
 	int i;
 	int decoration_style = 0;
+	struct userformat_want w;
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
@@ -58,7 +59,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
-	if (!rev->show_notes_given && !rev->pretty_given)
+	userformat_find_requirements(NULL,&w);
+
+	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
 		rev->show_notes = 1;
 	if (rev->show_notes)
 		init_display_notes(&rev->notes_opt);
diff --git a/commit.h b/commit.h
index 3cf5166..26ec8c0 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_find_requirements(const char *fmt, 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..871f4ae 100644
--- a/pretty.c
+++ b/pretty.c
@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		}
 		return 0;	/* unknown %g placeholder */
 	case 'N':
-		format_display_notes(commit->object.sha1, sb,
-			    git_log_output_encoding ? git_log_output_encoding
-						    : git_commit_encoding, 0);
-		return 1;
+		if (c->pretty_ctx->show_notes) {
+			format_display_notes(commit->object.sha1, sb,
+				    git_log_output_encoding ? git_log_output_encoding
+							    : git_commit_encoding, 0);
+			return 1;
+		}
+		return 0;
 	}
 
 	/* For the rest we have to parse the commit header. */
@@ -855,6 +858,31 @@ 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_find_requirements(const char *fmt, struct userformat_want *w)
+{
+	struct strbuf dummy = STRBUF_INIT;
+
+	memset(w, 0, sizeof(*w));
+	if (!fmt) {
+		if (!user_format)
+			return;
+		fmt = user_format;
+	}
+	strbuf_expand(&dummy, user_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)
-- 
1.7.1.rc1

Re: [PATCHv2] pretty: Initialize notes if %N is used

[Jeff King]

On Tue, Apr 13, 2010 at 10:59:46AM +0200, Johannes Gilger wrote:

> Thanks for the feedback Jeff. I've put your suggestions into my patch
> and tried to come up with a sensible name for 'userformat_fill_want'.
> As you can see I called it 'userformat_find_requirements', but am not
> really satisfied with it since it's too long and not quite to the
> point.
> 
> Anything else missing?

This version looks good to me.

Two minor comments:

> -	if (!rev->show_notes_given && !rev->pretty_given)
> +	userformat_find_requirements(NULL,&w);

 1. Style, no whitespace between arguments.

 2. That function name also sucks. I doubt it is worth spending more
    time on, though.

-Peff

Re: [PATCHv2] pretty: Initialize notes if %N is used

[Johannes Gilger]

On 13/04/10 06:03, Jeff King wrote:
> This version looks good to me.
Hm, I still found one mistake: Using %+N or %-N without --show-notes
doesn't work. I'll have a look at how to fix this. If Junio want's to go
ahead this can also be done in a second patch.

Greetings,
Jojo

-- 
Johannes Gilger <heipei@hackvalue.de>
http://heipei.net
GPG-Key: 0xD47A7FFC
GPG-Fingerprint: 5441 D425 6D4A BD33 B580  618C 3CDC C4D0 D47A 7FFC

[PATCHv3] pretty: Initialize notes if %N is used

[y]

From: Johannes Gilger <heipei@hackvalue.de>

When using git log --pretty='%N' without an explicit --show-notes, git
would segfault. This patches fixes this behaviour by loading the needed
notes datastructures if --pretty is used and the format contains %N.
When --pretty='%N' is used together with --no-notes, %N won't be
expanded.

This is an extension to a proposed patch by Jeff King.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
Introduced space when calling userformat_find_requirements and dealt with %+N
and %-N during the strbuf_expand phase. I hope strncmp is the right way to do
it here. strbuf is NUL-terminated so there should not be a problem.

Greetings,
Jojo

 builtin/log.c |    5 ++++-
 commit.h      |    5 +++++
 pretty.c      |   40 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b706a5f..6e6bc09 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 {
 	int i;
 	int decoration_style = 0;
+	struct userformat_want w;
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
@@ -58,7 +59,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
-	if (!rev->show_notes_given && !rev->pretty_given)
+	userformat_find_requirements(NULL, &w);
+
+	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
 		rev->show_notes = 1;
 	if (rev->show_notes)
 		init_display_notes(&rev->notes_opt);
diff --git a/commit.h b/commit.h
index 3cf5166..26ec8c0 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_find_requirements(const char *fmt, 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..31cef5c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		}
 		return 0;	/* unknown %g placeholder */
 	case 'N':
-		format_display_notes(commit->object.sha1, sb,
-			    git_log_output_encoding ? git_log_output_encoding
-						    : git_commit_encoding, 0);
-		return 1;
+		if (c->pretty_ctx->show_notes) {
+			format_display_notes(commit->object.sha1, sb,
+				    git_log_output_encoding ? git_log_output_encoding
+							    : git_commit_encoding, 0);
+			return 1;
+		}
+		return 0;
 	}
 
 	/* For the rest we have to parse the commit header. */
@@ -855,6 +858,35 @@ 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 '-':
+		case '+':
+			if (!strncmp(placeholder+1, "N", 1))
+				w->notes = 1;
+		case 'N': w->notes = 1;
+	}
+	return 0;
+}
+
+void userformat_find_requirements(const char *fmt, struct userformat_want *w)
+{
+	struct strbuf dummy = STRBUF_INIT;
+
+	memset(w, 0, sizeof(*w));
+	if (!fmt) {
+		if (!user_format)
+			return;
+		fmt = user_format;
+	}
+	strbuf_expand(&dummy, user_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)
-- 
1.7.1.rc1

[PATCHv3] pretty: Initialize notes if %N is used

[y]

From: Johannes Gilger <heipei@hackvalue.de>

When using git log --pretty='%N' without an explicit --show-notes, git
would segfault. This patches fixes this behaviour by loading the needed
notes datastructures if --pretty is used and the format contains %N.
When --pretty='%N' is used together with --no-notes, %N won't be
expanded.

This is an extension to a proposed patch by Jeff King.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
Introduced space when calling userformat_find_requirements and dealt with %+N
and %-N during the strbuf_expand phase. I hope strncmp is the right way to do
it here. strbuf is NUL-terminated so there should not be a problem.

Greetings,
Jojo

 builtin/log.c |    5 ++++-
 commit.h      |    5 +++++
 pretty.c      |   40 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b706a5f..6e6bc09 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 {
 	int i;
 	int decoration_style = 0;
+	struct userformat_want w;
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
@@ -58,7 +59,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
-	if (!rev->show_notes_given && !rev->pretty_given)
+	userformat_find_requirements(NULL, &w);
+
+	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
 		rev->show_notes = 1;
 	if (rev->show_notes)
 		init_display_notes(&rev->notes_opt);
diff --git a/commit.h b/commit.h
index 3cf5166..26ec8c0 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_find_requirements(const char *fmt, 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..31cef5c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		}
 		return 0;	/* unknown %g placeholder */
 	case 'N':
-		format_display_notes(commit->object.sha1, sb,
-			    git_log_output_encoding ? git_log_output_encoding
-						    : git_commit_encoding, 0);
-		return 1;
+		if (c->pretty_ctx->show_notes) {
+			format_display_notes(commit->object.sha1, sb,
+				    git_log_output_encoding ? git_log_output_encoding
+							    : git_commit_encoding, 0);
+			return 1;
+		}
+		return 0;
 	}
 
 	/* For the rest we have to parse the commit header. */
@@ -855,6 +858,35 @@ 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 '-':
+		case '+':
+			if (!strncmp(placeholder+1, "N", 1))
+				w->notes = 1;
+		case 'N': w->notes = 1;
+	}
+	return 0;
+}
+
+void userformat_find_requirements(const char *fmt, struct userformat_want *w)
+{
+	struct strbuf dummy = STRBUF_INIT;
+
+	memset(w, 0, sizeof(*w));
+	if (!fmt) {
+		if (!user_format)
+			return;
+		fmt = user_format;
+	}
+	strbuf_expand(&dummy, user_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)
-- 
1.7.1.rc1

[PATCHv3] pretty: Initialize notes if %N is used

[Johannes Gilger]

When using git log --pretty='%N' without an explicit --show-notes, git
would segfault. This patches fixes this behaviour by loading the needed
notes datastructures if --pretty is used and the format contains %N.
When --pretty='%N' is used together with --no-notes, %N won't be
expanded.

This is an extension to a proposed patch by Jeff King.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
Introduced space when calling userformat_find_requirements and dealt with %+N
and %-N during the strbuf_expand phase. I hope strncmp is the right way to do
it here. strbuf is NUL-terminated so there should not be a problem.

Greetings,
Jojo

 builtin/log.c |    5 ++++-
 commit.h      |    5 +++++
 pretty.c      |   40 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b706a5f..6e6bc09 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 {
 	int i;
 	int decoration_style = 0;
+	struct userformat_want w;
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
@@ -58,7 +59,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
-	if (!rev->show_notes_given && !rev->pretty_given)
+	userformat_find_requirements(NULL, &w);
+
+	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
 		rev->show_notes = 1;
 	if (rev->show_notes)
 		init_display_notes(&rev->notes_opt);
diff --git a/commit.h b/commit.h
index 3cf5166..26ec8c0 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_find_requirements(const char *fmt, 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..31cef5c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		}
 		return 0;	/* unknown %g placeholder */
 	case 'N':
-		format_display_notes(commit->object.sha1, sb,
-			    git_log_output_encoding ? git_log_output_encoding
-						    : git_commit_encoding, 0);
-		return 1;
+		if (c->pretty_ctx->show_notes) {
+			format_display_notes(commit->object.sha1, sb,
+				    git_log_output_encoding ? git_log_output_encoding
+							    : git_commit_encoding, 0);
+			return 1;
+		}
+		return 0;
 	}
 
 	/* For the rest we have to parse the commit header. */
@@ -855,6 +858,35 @@ 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 '-':
+		case '+':
+			if (!strncmp(placeholder+1, "N", 1))
+				w->notes = 1;
+		case 'N': w->notes = 1;
+	}
+	return 0;
+}
+
+void userformat_find_requirements(const char *fmt, struct userformat_want *w)
+{
+	struct strbuf dummy = STRBUF_INIT;
+
+	memset(w, 0, sizeof(*w));
+	if (!fmt) {
+		if (!user_format)
+			return;
+		fmt = user_format;
+	}
+	strbuf_expand(&dummy, user_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)
-- 
1.7.1.rc1

Re: [PATCHv3] pretty: Initialize notes if %N is used

[Jeff King]

On Tue, Apr 13, 2010 at 01:01:05PM +0200, Johannes Gilger wrote:

> Introduced space when calling userformat_find_requirements and dealt
> with %+N and %-N during the strbuf_expand phase. I hope strncmp is the
> right way to do it here. strbuf is NUL-terminated so there should not
> be a problem.

Ugh. I didn't even know we had such a thing. Those look like the only
ones that should be a problem, though. I'm glad to have factored out the
"want" code, now. At least it will all be in one spot.

> +static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
> +				   void *context)
> +{
> +	struct userformat_want *w = context;
> +
> +	switch (*placeholder) {
> +		case '-':
> +		case '+':
> +			if (!strncmp(placeholder+1, "N", 1))
> +				w->notes = 1;
> +		case 'N': w->notes = 1;
> +	}
> +	return 0;
> +}

Should this perhaps be:

  if (*placeholder == '+' || *placeholder == '-')
    placeholder++;

  switch (*placeholder) {
    case 'N': w->notes = 1; break;
  }

so that it will extend naturally if other placeholder lookups are needed
(since those ones also could have + or - markers).

Also, I just noticed that your case is missing a 'break'. Not a bug yet,
but it will be if somebody adds a new case. This is almost certainly my
fault from the original version I posted. :)

-Peff

[PATCHv4] pretty: Initialize notes if %N is used

[Johannes Gilger]

When using git log --pretty='%N' without an explicit --show-notes, git
would segfault. This patches fixes this behaviour by loading the needed
notes datastructures if --pretty is used and the format contains %N.
When --pretty='%N' is used together with --no-notes, %N won't be
expanded.

This is an extension to a proposed patch by Jeff King.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
Simpler checking of character after %+ and %- for 'N'. Reindentation of 'case'
below 'switch' to conform with the rest of the code.

 builtin/log.c |    5 ++++-
 commit.h      |    5 +++++
 pretty.c      |   41 +++++++++++++++++++++++++++++++++++++----
 3 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b706a5f..6e6bc09 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 {
 	int i;
 	int decoration_style = 0;
+	struct userformat_want w;
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
@@ -58,7 +59,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
-	if (!rev->show_notes_given && !rev->pretty_given)
+	userformat_find_requirements(NULL, &w);
+
+	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
 		rev->show_notes = 1;
 	if (rev->show_notes)
 		init_display_notes(&rev->notes_opt);
diff --git a/commit.h b/commit.h
index 3cf5166..26ec8c0 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_find_requirements(const char *fmt, 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..e79ac6f 100644
--- a/pretty.c
+++ b/pretty.c
@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		}
 		return 0;	/* unknown %g placeholder */
 	case 'N':
-		format_display_notes(commit->object.sha1, sb,
-			    git_log_output_encoding ? git_log_output_encoding
-						    : git_commit_encoding, 0);
-		return 1;
+		if (c->pretty_ctx->show_notes) {
+			format_display_notes(commit->object.sha1, sb,
+				    git_log_output_encoding ? git_log_output_encoding
+							    : git_commit_encoding, 0);
+			return 1;
+		}
+		return 0;
 	}
 
 	/* For the rest we have to parse the commit header. */
@@ -855,6 +858,36 @@ 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;
+
+	if (*placeholder == '+' || *placeholder == '-')
+		placeholder++;
+
+	switch (*placeholder) {
+	case 'N':
+		w->notes = 1;
+		break;
+	}
+	return 0;
+}
+
+void userformat_find_requirements(const char *fmt, struct userformat_want *w)
+{
+	struct strbuf dummy = STRBUF_INIT;
+
+	memset(w, 0, sizeof(*w));
+	if (!fmt) {
+		if (!user_format)
+			return;
+		fmt = user_format;
+	}
+	strbuf_expand(&dummy, user_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)
-- 
1.7.1.rc1

Re: [PATCHv4] pretty: Initialize notes if %N is used

[Junio C Hamano]

Johannes Gilger <heipei@hackvalue.de> writes:

> +void userformat_find_requirements(const char *fmt, struct userformat_want *w)
> +{
> +	struct strbuf dummy = STRBUF_INIT;
> +
> +	memset(w, 0, sizeof(*w));
> +	if (!fmt) {
> +		if (!user_format)
> +			return;
> +		fmt = user_format;
> +	}
> +	strbuf_expand(&dummy, user_format, userformat_want_item, w);
> +	strbuf_release(&dummy);
> +}

It does not matter for the current set of callers, but it might make sense
to make it the responsibility of the caller to clear *w instead of
unconditionally clearing what have been accumulated in there by previous
calls to this function.  It is not entirely implausible for a new caller
to have more than one user formats, it uses one or more on the same commit
depending on the context, and wants to find all the requirements by
feeding the possible formats upfront to this function to fill a single *w
structure.

[PATCHv5] pretty: Initialize notes if %N is used

[Johannes Gilger]

When using git log --pretty='%N' without an explicit --show-notes, git
would segfault. This patches fixes this behaviour by loading the needed
notes datastructures if --pretty is used and the format contains %N.
When --pretty='%N' is used together with --no-notes, %N won't be
expanded.

This is an extension to a proposed patch by Jeff King.

Signed-off-by: Johannes Gilger <heipei@hackvalue.de>
---
Shifted responsibility for zeroing the userformat_want struct to the caller
after Junio recommended it.

 builtin/log.c |    6 +++++-
 commit.h      |    5 +++++
 pretty.c      |   40 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b706a5f..6208703 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 {
 	int i;
 	int decoration_style = 0;
+	struct userformat_want w;
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
@@ -58,7 +59,10 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
-	if (!rev->show_notes_given && !rev->pretty_given)
+	memset(&w, 0, sizeof(w));
+	userformat_find_requirements(NULL, &w);
+
+	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
 		rev->show_notes = 1;
 	if (rev->show_notes)
 		init_display_notes(&rev->notes_opt);
diff --git a/commit.h b/commit.h
index 3cf5166..26ec8c0 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_find_requirements(const char *fmt, 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..7cb3a2a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		}
 		return 0;	/* unknown %g placeholder */
 	case 'N':
-		format_display_notes(commit->object.sha1, sb,
-			    git_log_output_encoding ? git_log_output_encoding
-						    : git_commit_encoding, 0);
-		return 1;
+		if (c->pretty_ctx->show_notes) {
+			format_display_notes(commit->object.sha1, sb,
+				    git_log_output_encoding ? git_log_output_encoding
+							    : git_commit_encoding, 0);
+			return 1;
+		}
+		return 0;
 	}
 
 	/* For the rest we have to parse the commit header. */
@@ -855,6 +858,35 @@ 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;
+
+	if (*placeholder == '+' || *placeholder == '-')
+		placeholder++;
+
+	switch (*placeholder) {
+	case 'N':
+		w->notes = 1;
+		break;
+	}
+	return 0;
+}
+
+void userformat_find_requirements(const char *fmt, struct userformat_want *w)
+{
+	struct strbuf dummy = STRBUF_INIT;
+
+	if (!fmt) {
+		if (!user_format)
+			return;
+		fmt = user_format;
+	}
+	strbuf_expand(&dummy, user_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)
-- 
1.7.1.rc1

Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes

[Johannes Gilger]

On 10/04/10 14:51, Junio C Hamano wrote:
> > -	if (!rev->show_notes_given && !rev->pretty_given)
> > +	if (!rev->show_notes_given)
> >  		rev->show_notes = 1;
> 
> I am puzzled by this change and its possible interaction with codepaths
> that do not have anything to do with %N.  When there is no show-notes and
> an explicit --pretty, we do not want to have rev->show_notes set.
> 
> Admittedly, the real end result we want to see in such a case is just that
> notes are not shown (and rev->show_notes being false is one natural way to
> achieve that), and if ...
Yes, it might seem a little dirty looking at the name of the flags. If
no --show-notes was given and --pretty was supplied, rev->show_notes
should have a value of 'maybe' ;)

I was aiming for minimally invasive changes while keeping the former
behaviour and only dealing with the "only %N" case, which is what this
patch does.

> > -	if (rev->show_notes)
> > +	if (rev->show_notes && (!rev->pretty_given || rev->show_notes_given))
> >  		init_display_notes(&rev->notes_opt);
> 
> ... this change is about ensuring the same outcome by not initializing the
> notes tree, that may work, but it somehow feels iffy.  It would leave some
> codepaths (and another one you just added, I think, with the other hunk in
> this patch) that say "do this only when rev->show_notes is set" and some
> other codepaths that say "unconditionally try to show notes and rely on
> the caller not have initialized the notes tree when it is not wanted."  Is
> that what is going on?
The implicit initialization of the notes_trees only happens if --pretty
is used alone, and in no other case. I was under the impression that not
initializing the notes_trees if one isn't sure of it's use was meant to
be a performance criterion. While --show-notes will always use the notes
when using plain log/show, it won't necessarily use the notes for
certain --pretty/--format formats. Granted, right now I can use --pretty
and --show-notes although I don't use %N and intentionally waste memory
by initializing the trees.

> Unfortunately I don't think of a better and cleaner solution offhand
> (perhaps such a cleaner solution would involve adding a bit more state in
> the rev structure, but I haven't thought things through).
Yes, I came across that structure too but was happy enough my patch
works as it is. I'll leave design decisions up to more involved
contributors, my main agenda is simply to not have git segfault with
something as harmless as "git log '%N'" ;)

Greetings,
Jojo

-- 
Johannes Gilger <heipei@hackvalue.de>
http://heipei.net
GPG-Key: 0xD47A7FFC
GPG-Fingerprint: 5441 D425 6D4A BD33 B580  618C 3CDC C4D0 D47A 7FFC