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

From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Date: Jun 2, 2023, 10:06 UTC
Message-ID: <8f434f16-de65-40c2-80d3-28d123c7be7f@app.fastmail.com>
In-Reply-To: <20230601182020.GC4165405@coredump.intra.peff.net>

Hi Peff!

On Thu, Jun 1, 2023, at 20:20, Jeff King wrote:

Show 12 quoted lines

>> 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.
>
> Yeah, I certainly agree that the behavior of range-diff is surprising,
> and that this is a bug.
>
> I'd have expected the solution here would be for range-diff to stop
> passing "--notes", and let "log" decide whether to show notes (based on
> specific --notes=foo it gets from other_arg).

I tried that (removing `--notes`) but it didn’t work out for me. I’d had hoped that `--notes` was just a holdover from a previous change that wasn’t needed any more, but it seems according to my testing that `range diff` needs to pass *some* notes-related argument to `log` in order for the test suite to pass.

Thinking about it again now it doesn’t really make sense in the isolated case of `range-diff`. But maybe it’s needed in conjunction with `format-patch`.

A relevant series is “range-diff: learn `--notes`” by Denton Liu (f3c7bfdde2 (Merge branch 'dl/range-diff-with-notes', 2019-12-05)).[1]

On the commit before that merge, `range-diff.c` does not pass `--notes` to `log`. Denton describes the behavior in the first cover letter:

> When I was using range-diff at $DAYJOB earlier, I realised that it
> includes commit notes as part of the commit message comparison.

So just like the default behavior of `log`.

However, the next version (v2) is about teaching `range-diff` to pass notes down to `log`:

> This patchset teaches range-diff and format-patch to pass `--notes`
> options down to the `git log` machinery. This should make the
> behaviour more configurable for users who either don't want notes to
> be displayed or want multiple notes refs to be displayed.

So apparently `range-diff` couldn’t pass on/down `--notes=<ref>` before that.

That series also teaches `format-patch` to handle notes in 5b583e6a09 (format-patch: pass notes configuration to range-diff, 2019-11-20).

[1] https://lore.kernel.org/git/cover.1574125554.git.liu.denton@gmail.com/

Show 16 quoted lines

>
> But...
>
>> 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 co-opting the `--standard-notes` option which has been
>> deprecated since ab18b2c0df[2] and which is currently only documented in
>> `pretty-options`.
>
> I'm not clear on whether you're actually fixing two separate bugs here,
> or if they need to be intertwined.

I guess it depends on your perspective. ;) I would say that they are so intertwined that fixing one also fixes the other.

> It seems like passing --standard-notes means that format-patch's
> range-diff will still show the standard notes by default. Maybe I'm
> misunderstanding the problem, though.

No, that should still work. See the test `format-patch --range-diff does not compare notes by default`.

That kind of thing seems to be handled by `log.c:get_notes_args`; `--no-notes` is sent to `log` if no notes should be shown.

Cheers

-- 
Kristoffer Haugsbakk