Hi Peff! On Thu, Jun 1, 2023, at 20:20, Jeff King wrote: >> 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@xxxxxxxxx/ > > 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
