Re: [PATCH v4 1/1] range-diff: treat notes like `log`

From: Junio C Hamano <gitster@pobox.com>
To: Kristoffer Haugsbakk <code@khaugsbakk.name>
Cc: git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Denton Liu <liu.denton@gmail.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH v4 1/1] range-diff: treat notes like `log`
Date: Tue, 19 Sep 2023 12:27:30 -0700	[thread overview]
Message-ID: <xmqqled2ezfh.fsf@gitster.g> (raw)
In-Reply-To: <244e102cc4693bb6291e03cffea6df05cdb29df3.1695144790.git.code@khaugsbakk.name> (Kristoffer Haugsbakk's message of "Tue, 19 Sep 2023 20:05:32 +0200")

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> Currently, `range-diff` shows the default notes if no notes-related
> arguments are given. This is also how `log` behaves. But unlike
> `range-diff`, `log` does *not* show the default notes if
> `--notes=<custom>` are given. In other words, this:
>
>     git log --notes=custom
>
> is equivalent to this:
>
>     git log --no-notes --notes=custom
>
> While:
>
>     git range-diff --notes=custom
>
> acts like this:
>
>     git log --notes --notes-custom
>
> This can’t be how the user expects `range-diff` to behave given that the
> man page for `range-diff` under `--[no-]notes[=<ref>]` says:
>
>> This flag is passed to the `git log` program (see git-log(1)) that
>> generates the patches.
>
> This behavior also affects `format-patch` since it uses `range-diff` for
> the cover letter. Unlike `log`, though, `format-patch` is not supposed
> to show the default notes if no notes-related arguments are given.[1]
> But this promise is broken when the range-diff happens to have something
> to say about the changes to the default notes, since that will be shown
> in the cover letter.
>
> Remedy this by introducing `--show-notes-by-default` that `range-diff` can
> use to tell the `log` subprocess what to do.

Very well described.  I think the rest of the proposed log message
is redundant now we have quite a good write-up above.

>  ifndef::git-rev-list[]
> +--show-notes-by-default::
> +	Show the default notes (see `--notes`) unless subsequent arguments
> +	are used to display specific notes.
> +
>  --notes[=<ref>]::
>  	Show the notes (see linkgit:git-notes[1]) that annotate the
>  	commit, when showing the commit log message.  This is the default

I think the new entry should come after the description of `--notes`,
which is the primary option around the "notes" feature.

In the description, I think "subsequent" is misphrased.  It makes it
sound as if

    $ git log --show-notes-by-default --notes=amlog

would stop showing the notes from the default notes tree (because
the notes from the .git/refs/notes/amlog is explicitly asked for),
while

    $ git log --notes=amlog --show-notes-by-default

would show both the default and the custom notes, which is not what
the code does, I think, in this hunk:

> @@ -3054,6 +3056,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  	if (revs->expand_tabs_in_log < 0)
>  		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
>  
> +	if (!revs->show_notes_given && revs->show_notes_by_default) {
> +		enable_default_display_notes(&revs->notes_opt, &revs->show_notes);
> +		revs->show_notes_given = 1;
> +	}
> +
>  	return left;
>  }

Other than the above minor nits, looks very good.

Thanks.