Initialize notes trees if %N is used and no --show-notes given
PATCH 1/1 Initialize notes trees if %N is used and no --show-notes given
---
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(-)
notes.c +2 -0
@@ -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,
Signature · 1 line
1.7.0.4.360.g11766c
Quoted 7 lines · Johannes Gilger
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.
[...]
notes.c void format_display_notes(const unsigned char *object_sha1, · line 1190
Quoted 8 lines
notes.c +2
-0@@ -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
[A Cc would have been nice, I nearly missed this but it's clearly my bug.]
Quoted 3 lines · Johannes Gilger
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), functionformat_display_notes, filenotes.c, line 1186.
[...]
notes.c void format_display_notes(const unsigned char *object_sha1, · line 1188
Quoted 11 lines
notes.c +2
-0@@ -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.
Signature · 2 lines
Thomas Rast
trast@{inf,student}.ethz.chQuoted 2 lines · Thomas Rast
[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 ;)
Quoted 4 lines
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.
Quoted 3 lines
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 afterinit_display_notes().
Sure.
Greetings,
Jojo
Signature · 4 lines
Johannes Gilger <[email protected]> http://heipei.net GPG-Key: 0xD47A7FFC GPG-Fingerprint: 5441 D425 6D4A BD33 B580 618C 3CDC C4D0 D47A 7FFC
Quoted 10 lines · Johannes Gilger
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".
Quoted 4 lines
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".
Signature · 2 lines
Thomas Rast
trast@{inf,student}.ethz.chQuoted 6 lines · Thomas Rast
Quoted 3 lines
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
Quoted 6 lines · Thomas Rast
Quoted 3 lines
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.
Quoted 2 lines · Junio C Hamano
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.
Quoted 2 lines
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.
commit.h +5 -0
@@ -74,11 +74,16 @@ struct pretty_print_contextstruct 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);
pretty.c +18 -0
@@ -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 1/1 pretty.c: Don't expand %N without --show-notes
In reply to Thomas RastThe %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.
---
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(-)
Documentation/pretty-formats.txt +2 -1
@@ -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
pretty.c +3 -0
@@ -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 );
Signature · 1 line
1.7.0.2.201.g80978
Re: [PATCH] pretty.c: Don't expand %N without --show-notes
Quoted 3 lines · 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.
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".
Quoted 30 lines
Documentation/pretty-formats.txt +2
-1@@ -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 thepretty.c +3
-0@@ -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 );Signature · 1 line
1.7.0.2.201.g80978
PATCH 1/1 Notes: Connect the %N flag to --{show,no}-notes
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.
---
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(-)
builtin/log.c +2 -2
@@ -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)
notes.c +3 -1
@@ -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);
pretty.c +4 -3
@@ -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 ; }
Signature · 1 line
1.7.0.2.201.g80978
Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes
builtin/log.c static void cmd_log_init(int argc, const char **argv, const char *prefix, · line 62
Quoted 11 lines · Johannes Gilger
builtin/log.c +1
-1@@ -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 ...
builtin/log.c static void cmd_log_init(int argc, const char **argv, const char *prefix, · line 64
Quoted 3 lines
builtin/log.c +1
-1-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).
Quoted 35 lines
notes.c +3
-1@@ -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);pretty.c +4
-3@@ -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 ; }Signature · 1 line
1.7.0.2.201.g80978
Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes
Quoted 5 lines · Junio C Hamano
Quoted 5 lines
+++
b/builtin/log.cbuiltin/log.c +0
-0@@ -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);
Quoted 7 lines
builtin/log.c static void cmd_log_init(int argc, const char **argv, const char *prefix, · line 62
Quoted 3 lines
builtin/log.c +1
-1-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_notesset.
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 1/1 pretty: Initialize notes if %N is used
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.
---
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(-)
builtin/log.c +5 -1
@@ -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);
commit.h +5 -0
@@ -74,11 +74,16 @@ struct pretty_print_contextstruct 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);
pretty.c +27 -4
@@ -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)
Signature · 1 line
1.7.0.2.201.g80978
Re: [PATCH] pretty: Initialize notes if %N is used
Quoted 2 lines · Johannes Gilger
something like this? I didn't see why
userformat_fill_wanthad to have an extra argument for the format, since theuser_formatvariable is static inpretty.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?
Quoted 2 lines
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.
builtin/log.c static void cmd_log_init(int argc, const char **argv, const char *prefix, · line 61
Quoted 10 lines
builtin/log.c +1
-1@@ -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.
builtin/log.c static void cmd_log_init(int argc, const char **argv, const char *prefix, · line 66
Quoted 5 lines
builtin/log.c +4
-0+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.
builtin/log.c static void cmd_log_init(int argc, const char **argv, const char *prefix,
Quoted 9 lines
builtin/log.c +9
-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); +}
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
PATCH v2 1/1 pretty: Initialize notes if %N is used
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.
---
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(-)
builtin/log.c +4 -1
@@ -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);
commit.h +5 -0
@@ -74,11 +74,16 @@ struct pretty_print_contextstruct 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);
pretty.c +32 -4
@@ -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)
Signature · 1 line
1.7.1.rc1
Re: [PATCHv2] pretty: Initialize notes if %N is used
Quoted 7 lines · Johannes Gilger
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:
builtin/log.c static void cmd_log_init(int argc, const char **argv, const char *prefix, · line 62
Quoted 2 lines
builtin/log.c +1
-1-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
Quoted 1 line · Jeff King
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
Signature · 4 lines
Johannes Gilger <[email protected]> http://heipei.net GPG-Key: 0xD47A7FFC GPG-Fingerprint: 5441 D425 6D4A BD33 B580 618C 3CDC C4D0 D47A 7FFC
PATCH v3 1/1 pretty: Initialize notes if %N is used
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.
---
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(-)
builtin/log.c +4 -1
@@ -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);
commit.h +5 -0
@@ -74,11 +74,16 @@ struct pretty_print_contextstruct 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);
pretty.c +36 -4
@@ -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)
Signature · 1 line
1.7.1.rc1
Re: [PATCHv3] pretty: Initialize notes if %N is used
Quoted 4 lines · Johannes Gilger
Introduced space when calling
userformat_find_requirementsand dealt with %+N and %-N during thestrbuf_expandphase. 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.
Quoted 4 lines
pretty.c +4
-0+static size_t userformat_want_item (struct strbuf *sb,const char *placeholder, +void *context) +{ +struct userformat_want *w = context;
pretty.c static size_t format_commit_item(struct strbuf *sb, const char *placeholder, · line 874
Quoted 10 lines
pretty.c +10
-0+ +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
PATCH v4 1/1 pretty: Initialize notes if %N is used
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.
---
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(-)
builtin/log.c +4 -1
@@ -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);
commit.h +5 -0
@@ -74,11 +74,16 @@ struct pretty_print_contextstruct 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);
pretty.c +37 -4
53 lines not on this page · raw message
Signature · 1 line
1.7.1.rc1
Re: [PATCHv4] pretty: Initialize notes if %N is used
pretty.c static size_t format_commit_item(struct strbuf *sb, const char *placeholder, · line 889
Quoted 13 lines · Johannes Gilger
pretty.c +13
-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); +}
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.
PATCH v5 1/1 pretty: Initialize notes if %N is used
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.
---
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(-)
builtin/log.c +5 -1
@@ -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);
commit.h +5 -0
@@ -74,11 +74,16 @@ struct pretty_print_contextstruct 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);
pretty.c +36 -4
@@ -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)
Signature · 1 line
1.7.1.rc1
PATCH v3 1/1 pretty: Initialize notes if %N is used
In reply to Johannes GilgerFrom: Johannes Gilger <[email protected]>
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.
---
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(-)
builtin/log.c +4 -1
@@ -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);
commit.h +5 -0
@@ -74,11 +74,16 @@ struct pretty_print_contextstruct 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);
pretty.c +36 -4
@@ -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)
Signature · 1 line
1.7.1.rc1
PATCH v3 1/1 pretty: Initialize notes if %N is used
In reply to Johannes GilgerFrom: Johannes Gilger <[email protected]>
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.
---
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(-)
builtin/log.c +4 -1
@@ -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);
commit.h +5 -0
@@ -74,11 +74,16 @@ struct pretty_print_contextstruct 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);
pretty.c +36 -4
@@ -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)
Signature · 1 line
1.7.1.rc1
Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes
In reply to Junio C HamanoQuoted 11 lines · Junio C Hamano
builtin/log.c static void cmd_log_init(int argc, const char **argv, const char *prefix, · line 62
Quoted 3 lines
builtin/log.c +1
-1-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_notesset.Admittedly, the real end result we want to see in such a case is just that notes are not shown (and
rev->show_notesbeing 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.
Quoted 11 lines
builtin/log.c static void cmd_log_init(int argc, const char **argv, const char *prefix, · line 64
Quoted 3 lines
builtin/log.c +1
-1-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_notesis 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.
Quoted 3 lines
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
Signature · 4 lines
Johannes Gilger <[email protected]> http://heipei.net GPG-Key: 0xD47A7FFC GPG-Fingerprint: 5441 D425 6D4A BD33 B580 618C 3CDC C4D0 D47A 7FFC