{"thread":{"id":"23335","subject":"[PATCH] Initialize notes trees if %N is used and no --show-notes given","startedAt":"2010-04-05T11:55:48Z","lastAt":"2010-04-13T20:31:12Z","messageCount":26,"participants":["Johannes Gilger","Jeff King","Thomas Rast","Junio C Hamano","y@vger.kernel.org"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"138588","messageId":"20100405115548.GA19971@macbook.lan.lan","threadId":"23335","inReplyTo":null,"subject":"[PATCH] Initialize notes trees if %N is used and no --show-notes given","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2010-04-05T11:55:48Z","receivedAt":"2010-04-05T11:55:48Z","isPatch":true,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"Signed-off-by: Johannes Gilger \n---\nHi list,\n\nthis bug bit me when I used 'git log --format=\"%N\"' without adding\n--show-notes, which caused git to fail an assertion:\n Assertion failed: (display_notes_trees), function format_display_notes, file notes.c, line 1186.\n\nWhile this patch fixes this behaviour, I'm not sure it's at the right\nplace or doesn't impact performance. So this is meant more as a\nbug-report.\n\nGreetings,\nJojo\n\n notes.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex e425e19..83f39ae 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1183,6 +1183,8 @@ void format_display_notes(const unsigned char *object_sha1,\n \t\t\t struct strbuf *sb, const char *output_encoding, int flags)\n {\n \tint i;\n+\tif (!display_notes_trees)\n+\t\tinit_display_notes(NULL);\n \tassert(display_notes_trees);\n \tfor (i = 0; display_notes_trees[i]; i++)\n \t\tformat_note(display_notes_trees[i], object_sha1, sb,\n-- \n1.7.0.4.360.g11766c\n"},{"id":"138707","messageId":"20100406053234.GC3901@coredump.intra.peff.net","threadId":"23335","inReplyTo":"20100405115548.GA19971@macbook.lan.lan","subject":"Re: [PATCH] Initialize notes trees if %N is used and no --show-notes given","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-06T05:32:35Z","receivedAt":"2010-04-06T05:32:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 05, 2010 at 01:55:48PM +0200, Johannes Gilger wrote:\n\n> While this patch fixes this behaviour, I'm not sure it's at the right\n> place or doesn't impact performance. So this is meant more as a\n> bug-report.\n> [...]\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -1183,6 +1183,8 @@ void format_display_notes(const unsigned char *object_sha1,\n> \t\t\t struct strbuf *sb, const char *output_encoding, int flags)\n> {\n> \tint i;\n> +\tif (!display_notes_trees)\n> +\t\tinit_display_notes(NULL);\n> \tassert(display_notes_trees);\n> \tfor (i = 0; display_notes_trees[i]; i++)\n> \t\tformat_note(display_notes_trees[i], object_sha1, sb,\n\nI'm not sure if it is right to just pass NULL. We shouldn't have any\nextra_refs in our display_notes_opt, because we would have had to\npass --show-notes to do so (at least from my brief reading of the code).\n\nBut shouldn't \"git show --no-standard-notes --format=%N\" pass a\ndisplay_notes_opt with suppress_default_notes set?\n\nI don't see it as all that likely (since without --show-notes, you\nwouldn't have _any_ notes, so why are you using %N?), but it seems to be\nthe correct behavior, and might be useful for a script that uses '%N' in\ncombination with user-provided options.\n\n-Peff\n"},{"id":"138731","messageId":"201004061127.01471.trast@student.ethz.ch","threadId":"23335","inReplyTo":"20100405115548.GA19971@macbook.lan.lan","subject":"Re: [PATCH] Initialize notes trees if %N is used and no --show-notes given","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-04-06T09:27:01Z","receivedAt":"2010-04-06T09:27:01Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"[A Cc would have been nice, I nearly missed this but it's clearly my\nbug.]\n\nJohannes Gilger wrote:\n> this bug bit me when I used 'git log --format=\"%N\"' without adding\n> --show-notes, which caused git to fail an assertion:\n> Assertion failed: (display_notes_trees), function format_display_notes, file notes.c, line 1186.\n[...]\n> diff --git a/notes.c b/notes.c\n> index e425e19..83f39ae 100644\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -1183,6 +1183,8 @@ void format_display_notes(const unsigned char *object_sha1,\n> \t\t\t struct strbuf *sb, const char *output_encoding, int flags)\n> {\n> \tint i;\n> +\tif (!display_notes_trees)\n> +\t\tinit_display_notes(NULL);\n> \tassert(display_notes_trees);\n\nThanks for the report. Unfortunately this returns to the\nsilently-initialize-with-NULL case that was I explicitly asked to\navoid.\n\nI see three options:\n- %N could simply expand to nothing if notes are disabled\n- %N could silently initialize as above\n- your patch\n\nthough for your patch, I'd also remove the assert() since it's\nbasically there to enforce the requirement of initializing them; the\ntrees list can never be NULL after init_display_notes().\n\nCurrently I think the first option would be the best, since\n(notionally; we still don't have all the bits AFAIK) the built-in\nformats can then be written with a %N at the right place, without\nhaving to worry about the other command line options. I haven't had\nenough coffee to think about any possible ill side effects, though.\n\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"138737","messageId":"20100406111904.GA46425@macbook.lan.lan","threadId":"23335","inReplyTo":"201004061127.01471.trast@student.ethz.ch","subject":"Re: [PATCH] Initialize notes trees if %N is used and no --show-notes given","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2010-04-06T11:19:04Z","receivedAt":"2010-04-06T11:19:04Z","isPatch":true,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"On 06/04/10 11:27, Thomas Rast wrote:\n> [A Cc would have been nice, I nearly missed this but it's clearly my\n> bug.]\nSorry for that, haven't mailed to git-ml for quite a while ;)\n\n> I see three options:\n> - %N could simply expand to nothing if notes are disabled\n> - %N could silently initialize as above\n> - your patch\nThe first option would be confusing. I, for one, would simply put %N in\nmy log and never really know that existing notes aren't displayed. I\nwasn't even sure my git.git checkout had notes, so I created one myself.\nA better behaviour would be to not expand %N if notes are disabled, so a\nuser gets some kind of feedback that %N isn't working.\n\nI'd really like %N to do the initialization. There is no other\nplaceholder which requires an extra option to work, if I see it\ncorrectly.\n\nAs for the builtin formats I was under the impressions that they worked\ncompletely outside the parser for placeholders, so one would not use\n'%N' in a builtin format, and %N initializing the notes would not\nconflict with --no-notes and builtin formats.\n\n> though for your patch, I'd also remove the assert() since it's\n> basically there to enforce the requirement of initializing them; the\n> trees list can never be NULL after init_display_notes().\nSure.\n\nGreetings,\nJojo\n\n-- \nJohannes Gilger \nhttp://heipei.net\nGPG-Key: 0xD47A7FFC\nGPG-Fingerprint: 5441 D425 6D4A BD33 B580 618C 3CDC C4D0 D47A 7FFC\n"},{"id":"138739","messageId":"201004061352.21945.trast@student.ethz.ch","threadId":"23335","inReplyTo":"20100406111904.GA46425@macbook.lan.lan","subject":"Re: [PATCH] Initialize notes trees if %N is used and no --show-notes given","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-04-06T11:52:21Z","receivedAt":"2010-04-06T11:52:21Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Johannes Gilger wrote:\n> \n> The first option would be confusing. I, for one, would simply put %N in\n> my log and never really know that existing notes aren't displayed. I\n> wasn't even sure my git.git checkout had notes, so I created one myself.\n> A better behaviour would be to not expand %N if notes are disabled, so a\n> user gets some kind of feedback that %N isn't working.\n> \n> I'd really like %N to do the initialization. There is no other\n> placeholder which requires an extra option to work, if I see it\n> correctly.\n\n%g[dDs] expand to nothing unless the log command walks reflogs, so\nthere is some precedent.\n\nOne thing I didn't consider in my other mail was that --pretty\nautomatically disables notes. I think in my plan (%N expands to\nnothing with --no-notes) this would have to change to the effect that\n--pretty only disables the *normal* note-showing code, but still\ninitializes according to the same rules.\n\nI'll have to check whether that amounts to the same as \"silent\ninitialization\".\n\n> As for the builtin formats I was under the impressions that they worked\n> completely outside the parser for placeholders, so one would not use\n> '%N' in a builtin format, and %N initializing the notes would not\n> conflict with --no-notes and builtin formats.\n\nThat's true, which is why I said \"notionally\".\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"138762","messageId":"20100406162240.GA5294@coredump.intra.peff.net","threadId":"23335","inReplyTo":"201004061352.21945.trast@student.ethz.ch","subject":"Re: [PATCH] Initialize notes trees if %N is used and no --show-notes given","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-06T16:22:40Z","receivedAt":"2010-04-06T16:22:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 06, 2010 at 01:52:21PM +0200, Thomas Rast wrote:\n\n> > I'd really like %N to do the initialization. There is no other\n> > placeholder which requires an extra option to work, if I see it\n> > correctly.\n> \n> %g[dDs] expand to nothing unless the log command walks reflogs, so\n> there is some precedent.\n\n%d loads decorations on demand, so there is some precedent the other\n%way, too. I don't personally have a preference, though.\n\n-Peff\n"},{"id":"138809","messageId":"7v39z7g4zp.fsf@alter.siamese.dyndns.org","threadId":"23335","inReplyTo":"201004061352.21945.trast@student.ethz.ch","subject":"Re: [PATCH] Initialize notes trees if %N is used and no --show-notes given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-07T06:18:34Z","receivedAt":"2010-04-07T06:18:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n>> I'd really like %N to do the initialization. There is no other\n>> placeholder which requires an extra option to work, if I see it\n>> correctly.\n>\n> %g[dDs] expand to nothing unless the log command walks reflogs, so\n> there is some precedent.\n\nAs Peff pointed out, %d does things lazily, but I suspect it might be hard\nto do a similar initialization for %N.\n\nI wonder if we can inspect-but-not-use format string before we even start\nwalking, to see if we need notes (when we see %N).\n\nNone of the abouve applies to %g because making it cause reflog walking\nwill change not only the output, but the fundamental behaviour of the\ncommand.\n"},{"id":"138810","messageId":"20100407063642.GA5399@coredump.intra.peff.net","threadId":"23335","inReplyTo":"7v39z7g4zp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Initialize notes trees if %N is used and no --show-notes given","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-07T06:36:42Z","receivedAt":"2010-04-07T06:36:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 06, 2010 at 11:18:34PM -0700, Junio C Hamano wrote:\n\n> As Peff pointed out, %d does things lazily, but I suspect it might be hard\n> to do a similar initialization for %N.\n\nI think you would just have to stuff the notes-related options from the\nrev-list options into the pretty-print context, which would then make\nthem available to the user-format callback.\n\n> I wonder if we can inspect-but-not-use format string before we even start\n> walking, to see if we need notes (when we see %N).\n\nI have considered something like the patch below before, but it is not\n100% accurate. Part of the parsing happens in strbuf_expand, but parsing\nof things like %w(...) happens ad-hoc inside the formatting callback (so\nwe would see \"%w(%N)\" as wanting notes, when it doesn't really. In\ntheory it would be nicer if we separated syntax and semantics, so I\ncould parse %X(...) as \"the %X placeholder with ... as arguments\"\nwithout having to actually understand what %X does. In practice, it\ndoesn't matter here because we don't have very many placeholders that\ntake arbitrary arguments.\n\nThe patch below is totally untested and just meant to illustrate the\napproach. Use caution.\n\ndiff --git a/commit.h b/commit.h\nindex 2b7fd89..5081389 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -74,11 +74,16 @@ struct pretty_print_context\n \tstruct reflog_walk_info *reflog_info;\n };\n \n+struct userformat_want {\n+\tunsigned notes:1;\n+};\n+\n extern int has_non_ascii(const char *text);\n struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern char *reencode_commit_message(const struct commit *commit,\n \t\t\t\t const char **encoding_p);\n extern void get_commit_format(const char *arg, struct rev_info *);\n+extern void userformat_fill_want(const char *format, struct userformat_want *w);\n extern void format_commit_message(const struct commit *commit,\n \t\t\t\t const char *format, struct strbuf *sb,\n \t\t\t\t const struct pretty_print_context *context);\ndiff --git a/pretty.c b/pretty.c\nindex 6ba3da8..8ed3d36 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -855,6 +855,24 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n \treturn consumed + 1;\n }\n \n+static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n+\t\t\t\t void *context)\n+{\n+\tstruct userformat_want *w = context;\n+\tswitch (*placeholder) {\n+\t\tcase 'N': w->notes = 1;\n+\t}\n+\treturn 0;\n+}\n+\n+void userformat_fill_want(const char *format, struct userformat_want *w)\n+{\n+\tstruct strbuf dummy = STRBUF_INIT;\n+\tmemset(w, 0, sizeof(*w));\n+\tstrbuf_expand(&dummy, format, userformat_want_item, w);\n+\tstrbuf_release(&dummy);\n+}\n+\n void format_commit_message(const struct commit *commit,\n \t\t\t const char *format, struct strbuf *sb,\n \t\t\t const struct pretty_print_context *pretty_ctx)\n"},{"id":"139126","messageId":"1270883127-11488-1-git-send-email-heipei@hackvalue.de","threadId":"23335","inReplyTo":"201004061127.01471.trast@student.ethz.ch","subject":"[PATCH] pretty.c: Don't expand %N without --show-notes","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2010-04-10T07:05:27Z","receivedAt":"2010-04-10T07:05:27Z","isPatch":true,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"The %N placeholder will only work if --show-notes was provided to log.\nBy not expanding the user is given feedback that he won't be shown any\nnotes.\n\nSigned-off-by: Johannes Gilger \n---\n Ok, this is another stab. I don't really know whether we want %N to expand to\n an empty string or not expand at all in case of no --show-notes. Obviously\n using 'return 1;' would implement the former behaviour, while I chose the\n latter because it prevents people like me from building useless log aliases.\n\n Documentation/pretty-formats.txt | 3 ++-\n pretty.c | 3 +++\n 2 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 1686a54..bf7813f 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -143,7 +143,8 @@ NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\n insert an empty string unless we are traversing reflog entries (e.g., by\n `git log -g`). The `%d` placeholder will use the \"short\" decoration\n-format if `--decorate` was not already provided on the command line.\n+format if `--decorate` was not already provided on the command line. The %N\n+placeholder won't be expanded unless `--show-notes` was provided.\n \n If you add a `{plus}` (plus sign) after '%' of a placeholder, a line-feed\n is inserted immediately before the expansion if and only if the\ndiff --git a/pretty.c b/pretty.c\nindex 6ba3da8..b39e2d5 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -775,6 +775,9 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,\n \t\t}\n \t\treturn 0;\t/* unknown %g placeholder */\n \tcase 'N':\n+\t\tif (!c->pretty_ctx->show_notes)\n+\t\t\treturn 0;\n+\n \t\tformat_display_notes(commit->object.sha1, sb,\n \t\t\t git_log_output_encoding ? git_log_output_encoding\n \t\t\t\t\t\t : git_commit_encoding, 0);\n-- \n1.7.0.2.201.g80978\n"},{"id":"139192","messageId":"7vaatbw00u.fsf@alter.siamese.dyndns.org","threadId":"23335","inReplyTo":"1270883127-11488-1-git-send-email-heipei@hackvalue.de","subject":"Re: [PATCH] pretty.c: Don't expand %N without --show-notes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-10T20:00:33Z","receivedAt":"2010-04-10T20:00:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Gilger writes:\n\n> The %N placeholder will only work if --show-notes was provided to log.\n> By not expanding the user is given feedback that he won't be shown any\n> notes.\n\nI kind of like the simplicity of this approach, but this probably cuts\nboth ways. I can hear some users screaming \"But but but I already told\nyou that I am interested in notes---how else would I say %N in the format\nstring?\" while others say \"Yeah, that way I can keep using the same format\nand when I don't give --show-notes it won't show extra information in the\noutput. Very nice\".\n\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index 1686a54..bf7813f 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -143,7 +143,8 @@ NOTE: Some placeholders may depend on other options given to the\n> revision traversal engine. For example, the `%g*` reflog options will\n> insert an empty string unless we are traversing reflog entries (e.g., by\n> `git log -g`). The `%d` placeholder will use the \"short\" decoration\n> -format if `--decorate` was not already provided on the command line.\n> +format if `--decorate` was not already provided on the command line. The %N\n> +placeholder won't be expanded unless `--show-notes` was provided.\n> \n> If you add a `{plus}` (plus sign) after '%' of a placeholder, a line-feed\n> is inserted immediately before the expansion if and only if the\n> diff --git a/pretty.c b/pretty.c\n> index 6ba3da8..b39e2d5 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -775,6 +775,9 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,\n> \t\t}\n> \t\treturn 0;\t/* unknown %g placeholder */\n> \tcase 'N':\n> +\t\tif (!c->pretty_ctx->show_notes)\n> +\t\t\treturn 0;\n> +\n> \t\tformat_display_notes(commit->object.sha1, sb,\n> \t\t\t git_log_output_encoding ? git_log_output_encoding\n> \t\t\t\t\t\t : git_commit_encoding, 0);\n> -- \n> 1.7.0.2.201.g80978\n"},{"id":"139201","messageId":"1270935032-10536-1-git-send-email-heipei@hackvalue.de","threadId":"23335","inReplyTo":"7vaatbw00u.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Notes: Connect the %N flag to --{show,no}-notes","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2010-04-10T21:30:32Z","receivedAt":"2010-04-10T21:30:32Z","isPatch":true,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"This improves the behaviour of the %N flag when using --pretty:\n\n- If only --pretty is used, notes will only be initialized if %N is\n actually part of the format.\n- If %N and --no-notes is used, %N will expand to the empty string.\n- Behaviour for regular log with --no-notes and --show-notes remains the\n same.\n\nSigned-off-by: Johannes Gilger \n---\nI reread a lot of code and think I've found a solution that's intuitive and\ndoesn't automatically initialize the trees with --pretty unless %N is actually\nused. Basically what could happen is this:\n\n- --show-notes (with eventual options) is supplied and\n init_display_notes(&rev.notes_opt) is called from log.c, %N is sucessfully\n expanded\n\n- Only %N is used, no --no-notes was supplied. format_display_notes is called,\n notices the missing notes_trees and initializes it. this obviously happens\n only if an %N is used, otherwise no display trees will be initialized.\n init_display_notes could not have been called with any other options, since\n no --show-notes was given, so NULL is fine here.\n\n- %N is used and --no-notes is supplied, format_display_notes is not\n called and %N is expanded to the empty string. This last case also\n applies to --pretty=full etc.\n\nI tested a lot of cases and tried to make sure I understood all the possible\ncaveats. If you have any further questions feel free to ask.\n\n builtin/log.c | 4 ++--\n notes.c | 4 +++-\n pretty.c | 7 ++++---\n 3 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b706a5f..029d7b8 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -58,9 +58,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\tusage(builtin_log_usage);\n \targc = setup_revisions(argc, argv, rev, opt);\n \n-\tif (!rev->show_notes_given && !rev->pretty_given)\n+\tif (!rev->show_notes_given)\n \t\trev->show_notes = 1;\n-\tif (rev->show_notes)\n+\tif (rev->show_notes && (!rev->pretty_given || rev->show_notes_given))\n \t\tinit_display_notes(&rev->notes_opt);\n \n \tif (rev->diffopt.pickaxe || rev->diffopt.filter)\ndiff --git a/notes.c b/notes.c\nindex e425e19..ad14a8b 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1183,7 +1183,9 @@ void format_display_notes(const unsigned char *object_sha1,\n \t\t\t struct strbuf *sb, const char *output_encoding, int flags)\n {\n \tint i;\n-\tassert(display_notes_trees);\n+\tif(!display_notes_trees)\n+\t\tinit_display_notes(NULL);\n+\n \tfor (i = 0; display_notes_trees[i]; i++)\n \t\tformat_note(display_notes_trees[i], object_sha1, sb,\n \t\t\t output_encoding, flags);\ndiff --git a/pretty.c b/pretty.c\nindex 6ba3da8..8e828a1 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -775,9 +775,10 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,\n \t\t}\n \t\treturn 0;\t/* unknown %g placeholder */\n \tcase 'N':\n-\t\tformat_display_notes(commit->object.sha1, sb,\n-\t\t\t git_log_output_encoding ? git_log_output_encoding\n-\t\t\t\t\t\t : git_commit_encoding, 0);\n+\t\tif (c->pretty_ctx->show_notes)\n+\t\t\tformat_display_notes(commit->object.sha1, sb,\n+\t\t\t\t\t git_log_output_encoding ? git_log_output_encoding\n+\t\t\t\t\t : git_commit_encoding, 0);\n \t\treturn 1;\n \t}\n \n-- \n1.7.0.2.201.g80978\n"},{"id":"139202","messageId":"7v1venvuv8.fsf@alter.siamese.dyndns.org","threadId":"23335","inReplyTo":"1270935032-10536-1-git-send-email-heipei@hackvalue.de","subject":"Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-10T21:51:55Z","receivedAt":"2010-04-10T21:51:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Gilger writes:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index b706a5f..029d7b8 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -58,9 +58,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n> \t\tusage(builtin_log_usage);\n> \targc = setup_revisions(argc, argv, rev, opt);\n> \n> -\tif (!rev->show_notes_given && !rev->pretty_given)\n> +\tif (!rev->show_notes_given)\n> \t\trev->show_notes = 1;\n\nI am puzzled by this change and its possible interaction with codepaths\nthat do not have anything to do with %N. When there is no show-notes and\nan explicit --pretty, we do not want to have rev->show_notes set.\n\nAdmittedly, the real end result we want to see in such a case is just that\nnotes are not shown (and rev->show_notes being false is one natural way to\nachieve that), and if ...\n\n> -\tif (rev->show_notes)\n> +\tif (rev->show_notes && (!rev->pretty_given || rev->show_notes_given))\n> \t\tinit_display_notes(&rev->notes_opt);\n\n... this change is about ensuring the same outcome by not initializing the\nnotes tree, that may work, but it somehow feels iffy. It would leave some\ncodepaths (and another one you just added, I think, with the other hunk in\nthis patch) that say \"do this only when rev->show_notes is set\" and some\nother codepaths that say \"unconditionally try to show notes and rely on\nthe caller not have initialized the notes tree when it is not wanted.\" Is\nthat what is going on?\n\nUnfortunately I don't think of a better and cleaner solution offhand\n(perhaps such a cleaner solution would involve adding a bit more state in\nthe rev structure, but I haven't thought things through).\n\n> diff --git a/notes.c b/notes.c\n> index e425e19..ad14a8b 100644\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -1183,7 +1183,9 @@ void format_display_notes(const unsigned char *object_sha1,\n> \t\t\t struct strbuf *sb, const char *output_encoding, int flags)\n> {\n> \tint i;\n> -\tassert(display_notes_trees);\n> +\tif(!display_notes_trees)\n> +\t\tinit_display_notes(NULL);\n> +\n> \tfor (i = 0; display_notes_trees[i]; i++)\n> \t\tformat_note(display_notes_trees[i], object_sha1, sb,\n> \t\t\t output_encoding, flags);\n> diff --git a/pretty.c b/pretty.c\n> index 6ba3da8..8e828a1 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -775,9 +775,10 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,\n> \t\t}\n> \t\treturn 0;\t/* unknown %g placeholder */\n> \tcase 'N':\n> -\t\tformat_display_notes(commit->object.sha1, sb,\n> -\t\t\t git_log_output_encoding ? git_log_output_encoding\n> -\t\t\t\t\t\t : git_commit_encoding, 0);\n> +\t\tif (c->pretty_ctx->show_notes)\n> +\t\t\tformat_display_notes(commit->object.sha1, sb,\n> +\t\t\t\t\t git_log_output_encoding ? git_log_output_encoding\n> +\t\t\t\t\t : git_commit_encoding, 0);\n> \t\treturn 1;\n> \t}\n> \n> -- \n> 1.7.0.2.201.g80978\n"},{"id":"139203","messageId":"20100410220843.GA29987@coredump.intra.peff.net","threadId":"23335","inReplyTo":"7v1venvuv8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-10T22:08:43Z","receivedAt":"2010-04-10T22:08:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 10, 2010 at 02:51:55PM -0700, Junio C Hamano wrote:\n\n> > +++ b/builtin/log.c\n> > @@ -58,9 +58,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n> > \t\tusage(builtin_log_usage);\n> > \targc = setup_revisions(argc, argv, rev, opt);\n> > \n> > -\tif (!rev->show_notes_given && !rev->pretty_given)\n> > +\tif (!rev->show_notes_given)\n> > \t\trev->show_notes = 1;\n> \n> I am puzzled by this change and its possible interaction with codepaths\n> that do not have anything to do with %N. When there is no show-notes and\n> an explicit --pretty, we do not want to have rev->show_notes set.\n\nCould we perhaps just do:\n\n if (!rev->show_notes_given &&\n (!rev->pretty_given ||\n (rev->commit_format == CMIT_FMT_USERFORMAT && fmt_wants_notes(...))\n\nwhere fmt_wants_notes is similar to what I posted earlier, or even just\nstrstr(fmt, \"%N\")? As I discussed earlier, it is not 100% accurate, but\nit is extremely unlikely for it to be wrong, and when it is, we will\nload notes when we don't need to. Which is an optimization failure, but\nnot a correctness failure.\n\nAnd then just guard the '%N' placeholder by checking show_notes. That\nwill protect random codepaths that call format_commit_message() but\naren't log (they can't trigger an assert, but will just get '%N'\nunexpanded or whatever). And doing:\n\n git log --no-notes --format='%N'\n\nshould also just fail to expand %N. Which is maybe a little crazy, but\nwhat the user is asking for is crazy, and it makes the most sense to me.\n\n-Peff\n"},{"id":"139206","messageId":"20100410222031.GA12507@dualtron.lan","threadId":"23335","inReplyTo":"7v1venvuv8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2010-04-10T22:20:31Z","receivedAt":"2010-04-10T22:20:31Z","isPatch":true,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"On 10/04/10 14:51, Junio C Hamano wrote:\n> > -\tif (!rev->show_notes_given && !rev->pretty_given)\n> > +\tif (!rev->show_notes_given)\n> > \t\trev->show_notes = 1;\n> \n> I am puzzled by this change and its possible interaction with codepaths\n> that do not have anything to do with %N. When there is no show-notes and\n> an explicit --pretty, we do not want to have rev->show_notes set.\n> \n> Admittedly, the real end result we want to see in such a case is just that\n> notes are not shown (and rev->show_notes being false is one natural way to\n> achieve that), and if ...\nYes, it might seem a little dirty looking at the name of the flags. If\nno --show-notes was given and --pretty was supplied, rev->show_notes\nshould have a value of 'maybe' ;)\n\nI was aiming for minimally invasive changes while keeping the former\nbehaviour and only dealing with the \"only %N\" case, which is what this\npatch does.\n\n> > -\tif (rev->show_notes)\n> > +\tif (rev->show_notes && (!rev->pretty_given || rev->show_notes_given))\n> > \t\tinit_display_notes(&rev->notes_opt);\n> \n> ... this change is about ensuring the same outcome by not initializing the\n> notes tree, that may work, but it somehow feels iffy. It would leave some\n> codepaths (and another one you just added, I think, with the other hunk in\n> this patch) that say \"do this only when rev->show_notes is set\" and some\n> other codepaths that say \"unconditionally try to show notes and rely on\n> the caller not have initialized the notes tree when it is not wanted.\" Is\n> that what is going on?\nThe implicit initialization of the notes_trees only happens if --pretty\nis used alone, and in no other case. I was under the impression that not\ninitializing the notes_trees if one isn't sure of it's use was meant to\nbe a performance criterion. While --show-notes will always use the notes\nwhen using plain log/show, it won't necessarily use the notes for\ncertain --pretty/--format formats. Granted, right now I can use --pretty\nand --show-notes although I don't use %N and intentionally waste memory\nby initializing the trees.\n\n> Unfortunately I don't think of a better and cleaner solution offhand\n> (perhaps such a cleaner solution would involve adding a bit more state in\n> the rev structure, but I haven't thought things through).\nYes, I came across that structure too but was happy enough my patch\nworks as it is. I'll leave design decisions up to more involved\ncontributors, my main agenda is simply to not have git segfault with\nsomething as harmless as \"git log '%N'\" ;)\n\nGreetings,\nJojo\n\n-- \nJohannes Gilger \nhttp://heipei.net\nGPG-Key: 0xD47A7FFC\nGPG-Fingerprint: 5441 D425 6D4A BD33 B580 618C 3CDC C4D0 D47A 7FFC\n"},{"id":"139253","messageId":"1270997662-25430-1-git-send-email-heipei@hackvalue.de","threadId":"23335","inReplyTo":"20100410220843.GA29987@coredump.intra.peff.net","subject":"[PATCH] pretty: Initialize notes if %N is used","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2010-04-11T14:54:22Z","receivedAt":"2010-04-11T14:54:22Z","isPatch":true,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"When using git log --pretty='%N' without an explicit --show-notes, git\nwould segfault. This patches fixes this behaviour by loading the needed\nnotes datastructures if --pretty is used and the format contains %N.\nWhen --pretty='%N' is used together with --no-notes, %N won't be\nexpanded.\n\nThis is an extension to a proposed patch by Jeff King.\n\nSigned-off-by: Johannes Gilger \n---\nHey Jeff,\n\nsomething like this? I didn't see why userformat_fill_want had to have an extra\nargument for the format, since the user_format variable is static in pretty.c.\n\nSorry for the many very different patches to the bug, as you can see I'm not\nreally familiar with best-practices in git.git.\n\nGreetings,\nJojo\n\n builtin/log.c | 6 +++++-\n commit.h | 5 +++++\n pretty.c | 31 +++++++++++++++++++++++++++----\n 3 files changed, 37 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b706a5f..f8f5d22 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -58,7 +58,11 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\tusage(builtin_log_usage);\n \targc = setup_revisions(argc, argv, rev, opt);\n \n-\tif (!rev->show_notes_given && !rev->pretty_given)\n+\tstruct userformat_want w;\n+\tif (rev->commit_format == CMIT_FMT_USERFORMAT)\n+\t\tuserformat_fill_want(&w);\n+\n+\tif (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n \t\trev->show_notes = 1;\n \tif (rev->show_notes)\n \t\tinit_display_notes(&rev->notes_opt);\ndiff --git a/commit.h b/commit.h\nindex 3cf5166..fc1c504 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -74,11 +74,16 @@ struct pretty_print_context\n \tstruct reflog_walk_info *reflog_info;\n };\n \n+struct userformat_want {\n+\tunsigned notes:1;\n+};\n+\n extern int has_non_ascii(const char *text);\n struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern char *reencode_commit_message(const struct commit *commit,\n \t\t\t\t const char **encoding_p);\n extern void get_commit_format(const char *arg, struct rev_info *);\n+extern void userformat_fill_want(struct userformat_want *w);\n extern void format_commit_message(const struct commit *commit,\n \t\t\t\t const char *format, struct strbuf *sb,\n \t\t\t\t const struct pretty_print_context *context);\ndiff --git a/pretty.c b/pretty.c\nindex 6ba3da8..0e3ae98 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,\n \t\t}\n \t\treturn 0;\t/* unknown %g placeholder */\n \tcase 'N':\n-\t\tformat_display_notes(commit->object.sha1, sb,\n-\t\t\t git_log_output_encoding ? git_log_output_encoding\n-\t\t\t\t\t\t : git_commit_encoding, 0);\n-\t\treturn 1;\n+\t\tif (c->pretty_ctx->show_notes) {\n+\t\t\tformat_display_notes(commit->object.sha1, sb,\n+\t\t\t\t git_log_output_encoding ? git_log_output_encoding\n+\t\t\t\t\t\t\t : git_commit_encoding, 0);\n+\t\t\treturn 1;\n+\t\t}\n+\t\treturn 0;\n \t}\n \n \t/* For the rest we have to parse the commit header. */\n@@ -855,6 +858,26 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n \treturn consumed + 1;\n }\n \n+static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n+\t\t\t\t void *context)\n+{\n+\tstruct userformat_want *w = context;\n+\tswitch (*placeholder) {\n+\t\tcase 'N': w->notes = 1;\n+\t}\n+\treturn 0;\n+}\n+\n+void userformat_fill_want(struct userformat_want *w)\n+{\n+\tif (!user_format)\n+\t\treturn;\n+\tstruct strbuf dummy = STRBUF_INIT;\n+\tmemset(w, 0, sizeof(*w));\n+\tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n+\tstrbuf_release(&dummy);\n+}\n+\n void format_commit_message(const struct commit *commit,\n \t\t\t const char *format, struct strbuf *sb,\n \t\t\t const struct pretty_print_context *pretty_ctx)\n-- \n1.7.0.2.201.g80978\n"},{"id":"139318","messageId":"20100412085647.GA26840@coredump.intra.peff.net","threadId":"23335","inReplyTo":"1270997662-25430-1-git-send-email-heipei@hackvalue.de","subject":"Re: [PATCH] pretty: Initialize notes if %N is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-12T08:56:47Z","receivedAt":"2010-04-12T08:56:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 11, 2010 at 04:54:22PM +0200, Johannes Gilger wrote:\n\n> something like this? I didn't see why userformat_fill_want had to have an extra\n> argument for the format, since the user_format variable is static in pretty.c.\n\nYes, this is getting closer.\n\nThe reason to take the format argument is that there are places which\ncall format_commit_message() with an arbitrary string. We would want\nthem to be able to call userformat_fill_want(), too (right now, I don't\nthink any of them should need it, though).\n\nMaybe it should take a format string, and use the user_format string if\nyou pass NULL?\n\n> Sorry for the many very different patches to the bug, as you can see I'm not\n> really familiar with best-practices in git.git.\n\nNot at all. Sometimes seemingly simple bugs end up raising a whole host\nof other issues. I am glad you are sticking around to help come up with\na good solution.\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index b706a5f..f8f5d22 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -58,7 +58,11 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n> \t\tusage(builtin_log_usage);\n> \targc = setup_revisions(argc, argv, rev, opt);\n> \n> -\tif (!rev->show_notes_given && !rev->pretty_given)\n> +\tstruct userformat_want w;\n\nDon't declare variables in the middle of a function. It's a C99-ism that\nwe avoid for older compilers.\n\n> +\tif (rev->commit_format == CMIT_FMT_USERFORMAT)\n> +\t\tuserformat_fill_want(&w);\n> +\n> +\tif (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n> \t\trev->show_notes = 1;\n\nHmm. If we didn't get a userformat, what will be in w? It will be random\ncruft from the stack, because we didn't call userformat_fill_want.\n\nYou can just call it unconditionally, and it should do the right thing\nwith a NULL user_format.\n\n> +void userformat_fill_want(struct userformat_want *w)\n> +{\n> +\tif (!user_format)\n> +\t\treturn;\n> +\tstruct strbuf dummy = STRBUF_INIT;\n> +\tmemset(w, 0, sizeof(*w));\n> +\tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n> +\tstrbuf_release(&dummy);\n> +}\n\nThis does nothing with a NULL user_format. It should probably still do\nthe memset() to indicate that nothing is wanted (otherwise the caller\nhas to be sure to initialize w themselves if they think user_format\nmight be NULL).\n\nSo combined with my above suggestion:\n\n void userformat_fill_want(const char *s, struct userformat_want *w)\n {\n struct strbuf dummy = STRBUF_INIT;\n memset(w, 0, sizeof(*w));\n if (!s) {\n if (!user_format)\n return;\n s = user_format;\n }\n strbuf_expand(&dummy, user_format, userformat_want_item, w);\n strbuf_release(&dummy);\n }\n\nand then you can just call\n\n userformat_fill_want(NULL, w);\n\nsafely from log.c (you don't even need to check rev->commit_format).\n\nAlso, even though I picked the name, userformat_fill_want is kind of a\nlousy name. It was the best I could come up with, but maybe somebody has\na better suggestion.\n\n-Peff\n"},{"id":"139400","messageId":"1271149186-30156-1-git-send-email-heipei@hackvalue.de","threadId":"23335","inReplyTo":"20100412085647.GA26840@coredump.intra.peff.net","subject":"[PATCHv2] pretty: Initialize notes if %N is used","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2010-04-13T08:59:46Z","receivedAt":"2010-04-13T08:59:46Z","isPatch":false,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"When using git log --pretty='%N' without an explicit --show-notes, git\nwould segfault. This patches fixes this behaviour by loading the needed\nnotes datastructures if --pretty is used and the format contains %N.\nWhen --pretty='%N' is used together with --no-notes, %N won't be\nexpanded.\n\nThis is an extension to a proposed patch by Jeff King.\n\nSigned-off-by: Johannes Gilger \n---\nThanks for the feedback Jeff. I've put your suggestions into my patch and tried\nto come up with a sensible name for 'userformat_fill_want'. As you can see I\ncalled it 'userformat_find_requirements', but am not really satisfied with it\nsince it's too long and not quite to the point.\n\nAnything else missing?\n\nGreetings, Jojo\n\n builtin/log.c | 5 ++++-\n commit.h | 5 +++++\n pretty.c | 36 ++++++++++++++++++++++++++++++++----\n 3 files changed, 41 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b706a5f..dd8e996 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n {\n \tint i;\n \tint decoration_style = 0;\n+\tstruct userformat_want w;\n \n \trev->abbrev = DEFAULT_ABBREV;\n \trev->commit_format = CMIT_FMT_DEFAULT;\n@@ -58,7 +59,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\tusage(builtin_log_usage);\n \targc = setup_revisions(argc, argv, rev, opt);\n \n-\tif (!rev->show_notes_given && !rev->pretty_given)\n+\tuserformat_find_requirements(NULL,&w);\n+\n+\tif (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n \t\trev->show_notes = 1;\n \tif (rev->show_notes)\n \t\tinit_display_notes(&rev->notes_opt);\ndiff --git a/commit.h b/commit.h\nindex 3cf5166..26ec8c0 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -74,11 +74,16 @@ struct pretty_print_context\n \tstruct reflog_walk_info *reflog_info;\n };\n \n+struct userformat_want {\n+\tunsigned notes:1;\n+};\n+\n extern int has_non_ascii(const char *text);\n struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern char *reencode_commit_message(const struct commit *commit,\n \t\t\t\t const char **encoding_p);\n extern void get_commit_format(const char *arg, struct rev_info *);\n+extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);\n extern void format_commit_message(const struct commit *commit,\n \t\t\t\t const char *format, struct strbuf *sb,\n \t\t\t\t const struct pretty_print_context *context);\ndiff --git a/pretty.c b/pretty.c\nindex 6ba3da8..871f4ae 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,\n \t\t}\n \t\treturn 0;\t/* unknown %g placeholder */\n \tcase 'N':\n-\t\tformat_display_notes(commit->object.sha1, sb,\n-\t\t\t git_log_output_encoding ? git_log_output_encoding\n-\t\t\t\t\t\t : git_commit_encoding, 0);\n-\t\treturn 1;\n+\t\tif (c->pretty_ctx->show_notes) {\n+\t\t\tformat_display_notes(commit->object.sha1, sb,\n+\t\t\t\t git_log_output_encoding ? git_log_output_encoding\n+\t\t\t\t\t\t\t : git_commit_encoding, 0);\n+\t\t\treturn 1;\n+\t\t}\n+\t\treturn 0;\n \t}\n \n \t/* For the rest we have to parse the commit header. */\n@@ -855,6 +858,31 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n \treturn consumed + 1;\n }\n \n+static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n+\t\t\t\t void *context)\n+{\n+\tstruct userformat_want *w = context;\n+\n+\tswitch (*placeholder) {\n+\t\tcase 'N': w->notes = 1;\n+\t}\n+\treturn 0;\n+}\n+\n+void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n+{\n+\tstruct strbuf dummy = STRBUF_INIT;\n+\n+\tmemset(w, 0, sizeof(*w));\n+\tif (!fmt) {\n+\t\tif (!user_format)\n+\t\t\treturn;\n+\t\tfmt = user_format;\n+\t}\n+\tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n+\tstrbuf_release(&dummy);\n+}\n+\n void format_commit_message(const struct commit *commit,\n \t\t\t const char *format, struct strbuf *sb,\n \t\t\t const struct pretty_print_context *pretty_ctx)\n-- \n1.7.1.rc1\n"},{"id":"139407","messageId":"20100413100304.GA29101@coredump.intra.peff.net","threadId":"23335","inReplyTo":"1271149186-30156-1-git-send-email-heipei@hackvalue.de","subject":"Re: [PATCHv2] pretty: Initialize notes if %N is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-13T10:03:04Z","receivedAt":"2010-04-13T10:03:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 13, 2010 at 10:59:46AM +0200, Johannes Gilger wrote:\n\n> Thanks for the feedback Jeff. I've put your suggestions into my patch\n> and tried to come up with a sensible name for 'userformat_fill_want'.\n> As you can see I called it 'userformat_find_requirements', but am not\n> really satisfied with it since it's too long and not quite to the\n> point.\n> \n> Anything else missing?\n\nThis version looks good to me.\n\nTwo minor comments:\n\n> -\tif (!rev->show_notes_given && !rev->pretty_given)\n> +\tuserformat_find_requirements(NULL,&w);\n\n 1. Style, no whitespace between arguments.\n\n 2. That function name also sucks. I doubt it is worth spending more\n time on, though.\n\n-Peff\n"},{"id":"139411","messageId":"20100413103611.GA4181@dualtron.lan","threadId":"23335","inReplyTo":"20100413100304.GA29101@coredump.intra.peff.net","subject":"Re: [PATCHv2] pretty: Initialize notes if %N is used","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2010-04-13T10:36:11Z","receivedAt":"2010-04-13T10:36:11Z","isPatch":false,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"On 13/04/10 06:03, Jeff King wrote:\n> This version looks good to me.\nHm, I still found one mistake: Using %+N or %-N without --show-notes\ndoesn't work. I'll have a look at how to fix this. If Junio want's to go\nahead this can also be done in a second patch.\n\nGreetings,\nJojo\n\n-- \nJohannes Gilger \nhttp://heipei.net\nGPG-Key: 0xD47A7FFC\nGPG-Fingerprint: 5441 D425 6D4A BD33 B580 618C 3CDC C4D0 D47A 7FFC\n"},{"id":"139413","messageId":"27645.7987064079$1271156264@news.gmane.org","threadId":"23335","inReplyTo":"20100413103611.GA4181@dualtron.lan","subject":"[PATCHv3] pretty: Initialize notes if %N is used","fromName":"","fromEmail":"y@vger.kernel.org","sentAt":"2010-04-13T10:57:54Z","receivedAt":"2010-04-13T10:57:54Z","isPatch":false,"sender":{"key":"y@vger.kernel.org","avatar":null},"body":"From: Johannes Gilger \n\nWhen using git log --pretty='%N' without an explicit --show-notes, git\nwould segfault. This patches fixes this behaviour by loading the needed\nnotes datastructures if --pretty is used and the format contains %N.\nWhen --pretty='%N' is used together with --no-notes, %N won't be\nexpanded.\n\nThis is an extension to a proposed patch by Jeff King.\n\nSigned-off-by: Johannes Gilger \n---\nIntroduced space when calling userformat_find_requirements and dealt with %+N\nand %-N during the strbuf_expand phase. I hope strncmp is the right way to do\nit here. strbuf is NUL-terminated so there should not be a problem.\n\nGreetings,\nJojo\n\n builtin/log.c | 5 ++++-\n commit.h | 5 +++++\n pretty.c | 40 ++++++++++++++++++++++++++++++++++++----\n 3 files changed, 45 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b706a5f..6e6bc09 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n {\n \tint i;\n \tint decoration_style = 0;\n+\tstruct userformat_want w;\n \n \trev->abbrev = DEFAULT_ABBREV;\n \trev->commit_format = CMIT_FMT_DEFAULT;\n@@ -58,7 +59,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\tusage(builtin_log_usage);\n \targc = setup_revisions(argc, argv, rev, opt);\n \n-\tif (!rev->show_notes_given && !rev->pretty_given)\n+\tuserformat_find_requirements(NULL, &w);\n+\n+\tif (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n \t\trev->show_notes = 1;\n \tif (rev->show_notes)\n \t\tinit_display_notes(&rev->notes_opt);\ndiff --git a/commit.h b/commit.h\nindex 3cf5166..26ec8c0 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -74,11 +74,16 @@ struct pretty_print_context\n \tstruct reflog_walk_info *reflog_info;\n };\n \n+struct userformat_want {\n+\tunsigned notes:1;\n+};\n+\n extern int has_non_ascii(const char *text);\n struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern char *reencode_commit_message(const struct commit *commit,\n \t\t\t\t const char **encoding_p);\n extern void get_commit_format(const char *arg, struct rev_info *);\n+extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);\n extern void format_commit_message(const struct commit *commit,\n \t\t\t\t const char *format, struct strbuf *sb,\n \t\t\t\t const struct pretty_print_context *context);\ndiff --git a/pretty.c b/pretty.c\nindex 6ba3da8..31cef5c 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,\n \t\t}\n \t\treturn 0;\t/* unknown %g placeholder */\n \tcase 'N':\n-\t\tformat_display_notes(commit->object.sha1, sb,\n-\t\t\t git_log_output_encoding ? git_log_output_encoding\n-\t\t\t\t\t\t : git_commit_encoding, 0);\n-\t\treturn 1;\n+\t\tif (c->pretty_ctx->show_notes) {\n+\t\t\tformat_display_notes(commit->object.sha1, sb,\n+\t\t\t\t git_log_output_encoding ? git_log_output_encoding\n+\t\t\t\t\t\t\t : git_commit_encoding, 0);\n+\t\t\treturn 1;\n+\t\t}\n+\t\treturn 0;\n \t}\n \n \t/* For the rest we have to parse the commit header. */\n@@ -855,6 +858,35 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n \treturn consumed + 1;\n }\n \n+static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n+\t\t\t\t void *context)\n+{\n+\tstruct userformat_want *w = context;\n+\n+\tswitch (*placeholder) {\n+\t\tcase '-':\n+\t\tcase '+':\n+\t\t\tif (!strncmp(placeholder+1, \"N\", 1))\n+\t\t\t\tw->notes = 1;\n+\t\tcase 'N': w->notes = 1;\n+\t}\n+\treturn 0;\n+}\n+\n+void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n+{\n+\tstruct strbuf dummy = STRBUF_INIT;\n+\n+\tmemset(w, 0, sizeof(*w));\n+\tif (!fmt) {\n+\t\tif (!user_format)\n+\t\t\treturn;\n+\t\tfmt = user_format;\n+\t}\n+\tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n+\tstrbuf_release(&dummy);\n+}\n+\n void format_commit_message(const struct commit *commit,\n \t\t\t const char *format, struct strbuf *sb,\n \t\t\t const struct pretty_print_context *pretty_ctx)\n-- \n1.7.1.rc1\n"},{"id":"299219","messageId":"1271156274-6984-1-git-send-email-y","threadId":"23335","inReplyTo":"20100413103611.GA4181@dualtron.lan","subject":"[PATCHv3] pretty: Initialize notes if %N is used","fromName":"","fromEmail":"y@vger.kernel.org","sentAt":"2010-04-13T10:57:54Z","receivedAt":"2010-04-13T10:57:54Z","isPatch":false,"sender":{"key":"y@vger.kernel.org","avatar":null},"body":"From: Johannes Gilger \n\nWhen using git log --pretty='%N' without an explicit --show-notes, git\nwould segfault. This patches fixes this behaviour by loading the needed\nnotes datastructures if --pretty is used and the format contains %N.\nWhen --pretty='%N' is used together with --no-notes, %N won't be\nexpanded.\n\nThis is an extension to a proposed patch by Jeff King.\n\nSigned-off-by: Johannes Gilger \n---\nIntroduced space when calling userformat_find_requirements and dealt with %+N\nand %-N during the strbuf_expand phase. I hope strncmp is the right way to do\nit here. strbuf is NUL-terminated so there should not be a problem.\n\nGreetings,\nJojo\n\n builtin/log.c | 5 ++++-\n commit.h | 5 +++++\n pretty.c | 40 ++++++++++++++++++++++++++++++++++++----\n 3 files changed, 45 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b706a5f..6e6bc09 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n {\n \tint i;\n \tint decoration_style = 0;\n+\tstruct userformat_want w;\n \n \trev->abbrev = DEFAULT_ABBREV;\n \trev->commit_format = CMIT_FMT_DEFAULT;\n@@ -58,7 +59,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\tusage(builtin_log_usage);\n \targc = setup_revisions(argc, argv, rev, opt);\n \n-\tif (!rev->show_notes_given && !rev->pretty_given)\n+\tuserformat_find_requirements(NULL, &w);\n+\n+\tif (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n \t\trev->show_notes = 1;\n \tif (rev->show_notes)\n \t\tinit_display_notes(&rev->notes_opt);\ndiff --git a/commit.h b/commit.h\nindex 3cf5166..26ec8c0 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -74,11 +74,16 @@ struct pretty_print_context\n \tstruct reflog_walk_info *reflog_info;\n };\n \n+struct userformat_want {\n+\tunsigned notes:1;\n+};\n+\n extern int has_non_ascii(const char *text);\n struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern char *reencode_commit_message(const struct commit *commit,\n \t\t\t\t const char **encoding_p);\n extern void get_commit_format(const char *arg, struct rev_info *);\n+extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);\n extern void format_commit_message(const struct commit *commit,\n \t\t\t\t const char *format, struct strbuf *sb,\n \t\t\t\t const struct pretty_print_context *context);\ndiff --git a/pretty.c b/pretty.c\nindex 6ba3da8..31cef5c 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,\n \t\t}\n \t\treturn 0;\t/* unknown %g placeholder */\n \tcase 'N':\n-\t\tformat_display_notes(commit->object.sha1, sb,\n-\t\t\t git_log_output_encoding ? git_log_output_encoding\n-\t\t\t\t\t\t : git_commit_encoding, 0);\n-\t\treturn 1;\n+\t\tif (c->pretty_ctx->show_notes) {\n+\t\t\tformat_display_notes(commit->object.sha1, sb,\n+\t\t\t\t git_log_output_encoding ? git_log_output_encoding\n+\t\t\t\t\t\t\t : git_commit_encoding, 0);\n+\t\t\treturn 1;\n+\t\t}\n+\t\treturn 0;\n \t}\n \n \t/* For the rest we have to parse the commit header. */\n@@ -855,6 +858,35 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n \treturn consumed + 1;\n }\n \n+static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n+\t\t\t\t void *context)\n+{\n+\tstruct userformat_want *w = context;\n+\n+\tswitch (*placeholder) {\n+\t\tcase '-':\n+\t\tcase '+':\n+\t\t\tif (!strncmp(placeholder+1, \"N\", 1))\n+\t\t\t\tw->notes = 1;\n+\t\tcase 'N': w->notes = 1;\n+\t}\n+\treturn 0;\n+}\n+\n+void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n+{\n+\tstruct strbuf dummy = STRBUF_INIT;\n+\n+\tmemset(w, 0, sizeof(*w));\n+\tif (!fmt) {\n+\t\tif (!user_format)\n+\t\t\treturn;\n+\t\tfmt = user_format;\n+\t}\n+\tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n+\tstrbuf_release(&dummy);\n+}\n+\n void format_commit_message(const struct commit *commit,\n \t\t\t const char *format, struct strbuf *sb,\n \t\t\t const struct pretty_print_context *pretty_ctx)\n-- \n1.7.1.rc1\n\n"},{"id":"139414","messageId":"1271156465-7302-1-git-send-email-heipei@hackvalue.de","threadId":"23335","inReplyTo":"20100413103611.GA4181@dualtron.lan","subject":"[PATCHv3] pretty: Initialize notes if %N is used","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2010-04-13T11:01:05Z","receivedAt":"2010-04-13T11:01:05Z","isPatch":false,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"When using git log --pretty='%N' without an explicit --show-notes, git\nwould segfault. This patches fixes this behaviour by loading the needed\nnotes datastructures if --pretty is used and the format contains %N.\nWhen --pretty='%N' is used together with --no-notes, %N won't be\nexpanded.\n\nThis is an extension to a proposed patch by Jeff King.\n\nSigned-off-by: Johannes Gilger \n---\nIntroduced space when calling userformat_find_requirements and dealt with %+N\nand %-N during the strbuf_expand phase. I hope strncmp is the right way to do\nit here. strbuf is NUL-terminated so there should not be a problem.\n\nGreetings,\nJojo\n\n builtin/log.c | 5 ++++-\n commit.h | 5 +++++\n pretty.c | 40 ++++++++++++++++++++++++++++++++++++----\n 3 files changed, 45 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b706a5f..6e6bc09 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n {\n \tint i;\n \tint decoration_style = 0;\n+\tstruct userformat_want w;\n \n \trev->abbrev = DEFAULT_ABBREV;\n \trev->commit_format = CMIT_FMT_DEFAULT;\n@@ -58,7 +59,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\tusage(builtin_log_usage);\n \targc = setup_revisions(argc, argv, rev, opt);\n \n-\tif (!rev->show_notes_given && !rev->pretty_given)\n+\tuserformat_find_requirements(NULL, &w);\n+\n+\tif (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n \t\trev->show_notes = 1;\n \tif (rev->show_notes)\n \t\tinit_display_notes(&rev->notes_opt);\ndiff --git a/commit.h b/commit.h\nindex 3cf5166..26ec8c0 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -74,11 +74,16 @@ struct pretty_print_context\n \tstruct reflog_walk_info *reflog_info;\n };\n \n+struct userformat_want {\n+\tunsigned notes:1;\n+};\n+\n extern int has_non_ascii(const char *text);\n struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern char *reencode_commit_message(const struct commit *commit,\n \t\t\t\t const char **encoding_p);\n extern void get_commit_format(const char *arg, struct rev_info *);\n+extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);\n extern void format_commit_message(const struct commit *commit,\n \t\t\t\t const char *format, struct strbuf *sb,\n \t\t\t\t const struct pretty_print_context *context);\ndiff --git a/pretty.c b/pretty.c\nindex 6ba3da8..31cef5c 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,\n \t\t}\n \t\treturn 0;\t/* unknown %g placeholder */\n \tcase 'N':\n-\t\tformat_display_notes(commit->object.sha1, sb,\n-\t\t\t git_log_output_encoding ? git_log_output_encoding\n-\t\t\t\t\t\t : git_commit_encoding, 0);\n-\t\treturn 1;\n+\t\tif (c->pretty_ctx->show_notes) {\n+\t\t\tformat_display_notes(commit->object.sha1, sb,\n+\t\t\t\t git_log_output_encoding ? git_log_output_encoding\n+\t\t\t\t\t\t\t : git_commit_encoding, 0);\n+\t\t\treturn 1;\n+\t\t}\n+\t\treturn 0;\n \t}\n \n \t/* For the rest we have to parse the commit header. */\n@@ -855,6 +858,35 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n \treturn consumed + 1;\n }\n \n+static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n+\t\t\t\t void *context)\n+{\n+\tstruct userformat_want *w = context;\n+\n+\tswitch (*placeholder) {\n+\t\tcase '-':\n+\t\tcase '+':\n+\t\t\tif (!strncmp(placeholder+1, \"N\", 1))\n+\t\t\t\tw->notes = 1;\n+\t\tcase 'N': w->notes = 1;\n+\t}\n+\treturn 0;\n+}\n+\n+void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n+{\n+\tstruct strbuf dummy = STRBUF_INIT;\n+\n+\tmemset(w, 0, sizeof(*w));\n+\tif (!fmt) {\n+\t\tif (!user_format)\n+\t\t\treturn;\n+\t\tfmt = user_format;\n+\t}\n+\tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n+\tstrbuf_release(&dummy);\n+}\n+\n void format_commit_message(const struct commit *commit,\n \t\t\t const char *format, struct strbuf *sb,\n \t\t\t const struct pretty_print_context *pretty_ctx)\n-- \n1.7.1.rc1\n"},{"id":"139415","messageId":"20100413110723.GA2910@coredump.intra.peff.net","threadId":"23335","inReplyTo":"1271156465-7302-1-git-send-email-heipei@hackvalue.de","subject":"Re: [PATCHv3] pretty: Initialize notes if %N is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-13T11:07:24Z","receivedAt":"2010-04-13T11:07:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 13, 2010 at 01:01:05PM +0200, Johannes Gilger wrote:\n\n> Introduced space when calling userformat_find_requirements and dealt\n> with %+N and %-N during the strbuf_expand phase. I hope strncmp is the\n> right way to do it here. strbuf is NUL-terminated so there should not\n> be a problem.\n\nUgh. I didn't even know we had such a thing. Those look like the only\nones that should be a problem, though. I'm glad to have factored out the\n\"want\" code, now. At least it will all be in one spot.\n\n> +static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n> +\t\t\t\t void *context)\n> +{\n> +\tstruct userformat_want *w = context;\n> +\n> +\tswitch (*placeholder) {\n> +\t\tcase '-':\n> +\t\tcase '+':\n> +\t\t\tif (!strncmp(placeholder+1, \"N\", 1))\n> +\t\t\t\tw->notes = 1;\n> +\t\tcase 'N': w->notes = 1;\n> +\t}\n> +\treturn 0;\n> +}\n\nShould this perhaps be:\n\n if (*placeholder == '+' || *placeholder == '-')\n placeholder++;\n\n switch (*placeholder) {\n case 'N': w->notes = 1; break;\n }\n\nso that it will extend naturally if other placeholder lookups are needed\n(since those ones also could have + or - markers).\n\nAlso, I just noticed that your case is missing a 'break'. Not a bug yet,\nbut it will be if somebody adds a new case. This is almost certainly my\nfault from the original version I posted. :)\n\n-Peff\n"},{"id":"139416","messageId":"1271157981-9767-1-git-send-email-heipei@hackvalue.de","threadId":"23335","inReplyTo":"20100413110723.GA2910@coredump.intra.peff.net","subject":"[PATCHv4] pretty: Initialize notes if %N is used","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2010-04-13T11:26:21Z","receivedAt":"2010-04-13T11:26:21Z","isPatch":false,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"When using git log --pretty='%N' without an explicit --show-notes, git\nwould segfault. This patches fixes this behaviour by loading the needed\nnotes datastructures if --pretty is used and the format contains %N.\nWhen --pretty='%N' is used together with --no-notes, %N won't be\nexpanded.\n\nThis is an extension to a proposed patch by Jeff King.\n\nSigned-off-by: Johannes Gilger \n---\nSimpler checking of character after %+ and %- for 'N'. Reindentation of 'case'\nbelow 'switch' to conform with the rest of the code.\n\n builtin/log.c | 5 ++++-\n commit.h | 5 +++++\n pretty.c | 41 +++++++++++++++++++++++++++++++++++++----\n 3 files changed, 46 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b706a5f..6e6bc09 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n {\n \tint i;\n \tint decoration_style = 0;\n+\tstruct userformat_want w;\n \n \trev->abbrev = DEFAULT_ABBREV;\n \trev->commit_format = CMIT_FMT_DEFAULT;\n@@ -58,7 +59,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\tusage(builtin_log_usage);\n \targc = setup_revisions(argc, argv, rev, opt);\n \n-\tif (!rev->show_notes_given && !rev->pretty_given)\n+\tuserformat_find_requirements(NULL, &w);\n+\n+\tif (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n \t\trev->show_notes = 1;\n \tif (rev->show_notes)\n \t\tinit_display_notes(&rev->notes_opt);\ndiff --git a/commit.h b/commit.h\nindex 3cf5166..26ec8c0 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -74,11 +74,16 @@ struct pretty_print_context\n \tstruct reflog_walk_info *reflog_info;\n };\n \n+struct userformat_want {\n+\tunsigned notes:1;\n+};\n+\n extern int has_non_ascii(const char *text);\n struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern char *reencode_commit_message(const struct commit *commit,\n \t\t\t\t const char **encoding_p);\n extern void get_commit_format(const char *arg, struct rev_info *);\n+extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);\n extern void format_commit_message(const struct commit *commit,\n \t\t\t\t const char *format, struct strbuf *sb,\n \t\t\t\t const struct pretty_print_context *context);\ndiff --git a/pretty.c b/pretty.c\nindex 6ba3da8..e79ac6f 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,\n \t\t}\n \t\treturn 0;\t/* unknown %g placeholder */\n \tcase 'N':\n-\t\tformat_display_notes(commit->object.sha1, sb,\n-\t\t\t git_log_output_encoding ? git_log_output_encoding\n-\t\t\t\t\t\t : git_commit_encoding, 0);\n-\t\treturn 1;\n+\t\tif (c->pretty_ctx->show_notes) {\n+\t\t\tformat_display_notes(commit->object.sha1, sb,\n+\t\t\t\t git_log_output_encoding ? git_log_output_encoding\n+\t\t\t\t\t\t\t : git_commit_encoding, 0);\n+\t\t\treturn 1;\n+\t\t}\n+\t\treturn 0;\n \t}\n \n \t/* For the rest we have to parse the commit header. */\n@@ -855,6 +858,36 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n \treturn consumed + 1;\n }\n \n+static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n+\t\t\t\t void *context)\n+{\n+\tstruct userformat_want *w = context;\n+\n+\tif (*placeholder == '+' || *placeholder == '-')\n+\t\tplaceholder++;\n+\n+\tswitch (*placeholder) {\n+\tcase 'N':\n+\t\tw->notes = 1;\n+\t\tbreak;\n+\t}\n+\treturn 0;\n+}\n+\n+void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n+{\n+\tstruct strbuf dummy = STRBUF_INIT;\n+\n+\tmemset(w, 0, sizeof(*w));\n+\tif (!fmt) {\n+\t\tif (!user_format)\n+\t\t\treturn;\n+\t\tfmt = user_format;\n+\t}\n+\tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n+\tstrbuf_release(&dummy);\n+}\n+\n void format_commit_message(const struct commit *commit,\n \t\t\t const char *format, struct strbuf *sb,\n \t\t\t const struct pretty_print_context *pretty_ctx)\n-- \n1.7.1.rc1\n"},{"id":"139432","messageId":"7vzl17t944.fsf@alter.siamese.dyndns.org","threadId":"23335","inReplyTo":"1271157981-9767-1-git-send-email-heipei@hackvalue.de","subject":"Re: [PATCHv4] pretty: Initialize notes if %N is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-13T20:01:31Z","receivedAt":"2010-04-13T20:01:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Gilger writes:\n\n> +void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n> +{\n> +\tstruct strbuf dummy = STRBUF_INIT;\n> +\n> +\tmemset(w, 0, sizeof(*w));\n> +\tif (!fmt) {\n> +\t\tif (!user_format)\n> +\t\t\treturn;\n> +\t\tfmt = user_format;\n> +\t}\n> +\tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n> +\tstrbuf_release(&dummy);\n> +}\n\nIt does not matter for the current set of callers, but it might make sense\nto make it the responsibility of the caller to clear *w instead of\nunconditionally clearing what have been accumulated in there by previous\ncalls to this function. It is not entirely implausible for a new caller\nto have more than one user formats, it uses one or more on the same commit\ndepending on the context, and wants to find all the requirements by\nfeeding the possible formats upfront to this function to fill a single *w\nstructure.\n"},{"id":"139435","messageId":"1271190672-23506-1-git-send-email-heipei@hackvalue.de","threadId":"23335","inReplyTo":"7vzl17t944.fsf@alter.siamese.dyndns.org","subject":"[PATCHv5] pretty: Initialize notes if %N is used","fromName":"Johannes Gilger","fromEmail":"heipei@hackvalue.de","sentAt":"2010-04-13T20:31:12Z","receivedAt":"2010-04-13T20:31:12Z","isPatch":false,"sender":{"key":"heipei@hackvalue.de","avatar":"https://avatars.githubusercontent.com/u/6072?v=4"},"body":"When using git log --pretty='%N' without an explicit --show-notes, git\nwould segfault. This patches fixes this behaviour by loading the needed\nnotes datastructures if --pretty is used and the format contains %N.\nWhen --pretty='%N' is used together with --no-notes, %N won't be\nexpanded.\n\nThis is an extension to a proposed patch by Jeff King.\n\nSigned-off-by: Johannes Gilger \n---\nShifted responsibility for zeroing the userformat_want struct to the caller\nafter Junio recommended it.\n\n builtin/log.c | 6 +++++-\n commit.h | 5 +++++\n pretty.c | 40 ++++++++++++++++++++++++++++++++++++----\n 3 files changed, 46 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b706a5f..6208703 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -36,6 +36,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n {\n \tint i;\n \tint decoration_style = 0;\n+\tstruct userformat_want w;\n \n \trev->abbrev = DEFAULT_ABBREV;\n \trev->commit_format = CMIT_FMT_DEFAULT;\n@@ -58,7 +59,10 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\tusage(builtin_log_usage);\n \targc = setup_revisions(argc, argv, rev, opt);\n \n-\tif (!rev->show_notes_given && !rev->pretty_given)\n+\tmemset(&w, 0, sizeof(w));\n+\tuserformat_find_requirements(NULL, &w);\n+\n+\tif (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n \t\trev->show_notes = 1;\n \tif (rev->show_notes)\n \t\tinit_display_notes(&rev->notes_opt);\ndiff --git a/commit.h b/commit.h\nindex 3cf5166..26ec8c0 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -74,11 +74,16 @@ struct pretty_print_context\n \tstruct reflog_walk_info *reflog_info;\n };\n \n+struct userformat_want {\n+\tunsigned notes:1;\n+};\n+\n extern int has_non_ascii(const char *text);\n struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern char *reencode_commit_message(const struct commit *commit,\n \t\t\t\t const char **encoding_p);\n extern void get_commit_format(const char *arg, struct rev_info *);\n+extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);\n extern void format_commit_message(const struct commit *commit,\n \t\t\t\t const char *format, struct strbuf *sb,\n \t\t\t\t const struct pretty_print_context *context);\ndiff --git a/pretty.c b/pretty.c\nindex 6ba3da8..7cb3a2a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -775,10 +775,13 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,\n \t\t}\n \t\treturn 0;\t/* unknown %g placeholder */\n \tcase 'N':\n-\t\tformat_display_notes(commit->object.sha1, sb,\n-\t\t\t git_log_output_encoding ? git_log_output_encoding\n-\t\t\t\t\t\t : git_commit_encoding, 0);\n-\t\treturn 1;\n+\t\tif (c->pretty_ctx->show_notes) {\n+\t\t\tformat_display_notes(commit->object.sha1, sb,\n+\t\t\t\t git_log_output_encoding ? git_log_output_encoding\n+\t\t\t\t\t\t\t : git_commit_encoding, 0);\n+\t\t\treturn 1;\n+\t\t}\n+\t\treturn 0;\n \t}\n \n \t/* For the rest we have to parse the commit header. */\n@@ -855,6 +858,35 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n \treturn consumed + 1;\n }\n \n+static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n+\t\t\t\t void *context)\n+{\n+\tstruct userformat_want *w = context;\n+\n+\tif (*placeholder == '+' || *placeholder == '-')\n+\t\tplaceholder++;\n+\n+\tswitch (*placeholder) {\n+\tcase 'N':\n+\t\tw->notes = 1;\n+\t\tbreak;\n+\t}\n+\treturn 0;\n+}\n+\n+void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n+{\n+\tstruct strbuf dummy = STRBUF_INIT;\n+\n+\tif (!fmt) {\n+\t\tif (!user_format)\n+\t\t\treturn;\n+\t\tfmt = user_format;\n+\t}\n+\tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n+\tstrbuf_release(&dummy);\n+}\n+\n void format_commit_message(const struct commit *commit,\n \t\t\t const char *format, struct strbuf *sb,\n \t\t\t const struct pretty_print_context *pretty_ctx)\n-- \n1.7.1.rc1\n"}]}