Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 10, 2010, 21:51 UTC
- Message-ID
- <7v1venvuv8.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1270935032-10536-1-git-send-email-heipei@hackvalue.de>
Johannes Gilger <heipei@hackvalue.de> writes:
Show 11 quoted lines
> diff --git a/builtin/log.c b/builtin/log.c > index b706a5f..029d7b8 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -58,9 +58,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix, > usage(builtin_log_usage); > argc = setup_revisions(argc, argv, rev, opt); > > - if (!rev->show_notes_given && !rev->pretty_given) > + if (!rev->show_notes_given) > rev->show_notes = 1;
I am puzzled by this change and its possible interaction with codepaths that do not have anything to do with %N. When there is no show-notes and an explicit --pretty, we do not want to have rev->show_notes set.
Admittedly, the real end result we want to see in such a case is just that notes are not shown (and rev->show_notes being false is one natural way to achieve that), and if ...
> - if (rev->show_notes) > + if (rev->show_notes && (!rev->pretty_given || rev->show_notes_given)) > init_display_notes(&rev->notes_opt);
... this change is about ensuring the same outcome by not initializing the notes tree, that may work, but it somehow feels iffy. It would leave some codepaths (and another one you just added, I think, with the other hunk in this patch) that say "do this only when rev->show_notes is set" and some other codepaths that say "unconditionally try to show notes and rely on the caller not have initialized the notes tree when it is not wanted." Is that what is going on?
Unfortunately I don't think of a better and cleaner solution offhand (perhaps such a cleaner solution would involve adding a bit more state in the rev structure, but I haven't thought things through).
Show 35 quoted lines
> diff --git a/notes.c b/notes.c
> index e425e19..ad14a8b 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -1183,7 +1183,9 @@ void format_display_notes(const unsigned char *object_sha1,
> struct strbuf *sb, const char *output_encoding, int flags)
> {
> int i;
> - assert(display_notes_trees);
> + if(!display_notes_trees)
> + init_display_notes(NULL);
> +
> for (i = 0; display_notes_trees[i]; i++)
> format_note(display_notes_trees[i], object_sha1, sb,
> output_encoding, flags);
> diff --git a/pretty.c b/pretty.c
> index 6ba3da8..8e828a1 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -775,9 +775,10 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
> }
> return 0; /* unknown %g placeholder */
> case 'N':
> - format_display_notes(commit->object.sha1, sb,
> - git_log_output_encoding ? git_log_output_encoding
> - : git_commit_encoding, 0);
> + if (c->pretty_ctx->show_notes)
> + format_display_notes(commit->object.sha1, sb,
> + git_log_output_encoding ? git_log_output_encoding
> + : git_commit_encoding, 0);
> return 1;
> }
>
> --
> 1.7.0.2.201.g80978