Re: [PATCH v4 2/2] format-patch: learn --[no-]range-diff-notes

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

kristofferhaugsbakk@fastmail.com writes:

> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> git-format-patch(1) passes on the notes behavior that it is using for
> the patches to git-range-diff(1). In turn you get the same Git notes
> displayed in the range diff as the ones you used to generate the
> patches. And that makes sense in most cases.
>
> However, I often make notes between series versions that mostly prepend
> to the original. They end up looking like this:
>
>     v3:
>     [desc.]
>     v2:
>     [descr.]
>     v1:
>     [descr.]
>
> These notes are meant for the git-format-patch(1) output since they
> document the iterations. But including them also includes them in the
> range diff. And they have nothing useful to say there.
>
> Let’s teach git-format-patch(1) `--[no-]range-diff-notes` so that we
> can pass in different notes refs to the range diff, or just turn them
> off entirely.
>
> In addition to storing the list of notes, we also need a boolean
> `override` to distinguish these two cases:
>
> 1. No such options were given and empty list (use `--notes`)
> 2. Options were given and empty list (`--no-...` given; don’t use notes)

Nicely described.

> ***
> Note that using `--creation-factor` without `--range-diff` will cause
> the command to die. But this is not the case for `--[no-]range-diff-
> notes`; we would have to check `rdiff_notes.override`, which is a sticky
> value (cannot be turned off). The reason is that it is potentially
> inconvenient to error out since it would not let you turn off
> `--range-diff` in, say, some alias that uses `--no-range-diff-
> notes`. Granted, it is difficult for me to come up with a concrete use
> case since `--range-diff` requires a value, specifically a value which
> is probably not that reusable (revision range), and yet you have
> something like an alias set up with it. But why spend code closing
> that door? There is no usability upside to erroring out.

In short, do you mean something like this?

  Unlike `--creation-factor`, `--[no-]range-diff-notes` does not
  error out when used without `--range-diff`.  This flexibility
  accommodates workflows where users might configure default options
  in aliases or wrapper scripts, allowing `--range-diff` to be
  toggled independently.

I suspect that erroring out when only creation-factor is given,
perhaps via an alias, was a design mistake.  A user who wants to use
a setting customized for their workflow must resort to an alias
because there is no configuration variable to control its default.
In that light, the same argument for --[no-]range-diff-notes applies
here.  On the other hand, perhaps if we had a configuration variable
to control which notes are compared in range-diff and shown in the
output, we would not have to worry about these things.  I do not
know.

Other than that (no, not the "shall we also add a configuration?",
which I consider is outside the topic, but the overly verbose log
message that gives wandering thought process that does not help the
readers with crisp reasoning that leads to the decision which they
may or may not agree with), it looks good.