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

[Junio C Hamano <gitster@pobox.com>]

Johannes Gilger <heipei@hackvalue.de> writes:

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

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

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

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

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

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

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