Re: [PATCH v3 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
> ...
> something like an alias set up with it. But why spend code closing
> that door? There is no usability upside to erroring out.

This is somewhat shared with the next step, but the commit message
includes a lengthy narrative of the author's thought process ("An
off/on switch is enough for this behavior...", "But now we are faced
with a problem...", "Well, we can't. Therefore we need...").

Can we strip out the conversational journey?  The log message should
be a concise, permanent technical reference explaining the problem
(range diff notes inherit patch notes, which may contain irrelevant
iteration changelogs) and the solution (the new options and the
.override flag).

> diff --git a/Documentation/git-format-patch.adoc b/Documentation/git-format-patch.adoc
> index 191f64b77d1..5907f299a8d 100644
> --- a/Documentation/git-format-patch.adoc
> +++ b/Documentation/git-format-patch.adoc
> @@ -378,6 +378,21 @@ case is to show comparison with an older iteration of the same
>  topic and the tool should find more correspondence between the two
>  sets of patches.
>  
> +`--range-diff-notes=<ref>`::
> +`--no-range-diff-notes`::
> +	Used with `--range-diff`, tweak what notes to display in the
> +	range diff.
> ++
> +The default behavior is to display the same notes in the range diff as
> +on the patches; see `--notes`. But you can use these options to use a
> +different list of notes. For example, say you have given three notes
> +refs to `--notes`. At this point those same three notes will be
> +displayed in the range diff. But then you pass
> +`--range-diff-notes=<ref>`. Now the range diff will only display
> +_<ref>_. You can of course pass more refs to this option, just like
> +`--notes`. And you can also turn off all range diff notes with
> +`--no-range-diff-notes`.

Very chatty and colloquial.  A technical reference manual should be
concise and direct.  Here is my attempt to condense it down to make
it more readable:

  By default, '--range-diff' displays the same notes as the patches
  (see '--notes').  Use '--range-diff-notes=<ref>' to specify a
  different notes ref for the range diff. This option can be given
  multiple times to show notes from multiple refs.  Use
  '--no-range-diff-notes' to disable notes in the range diff.

> diff --git a/builtin/log.c b/builtin/log.c
> index 560af00e2fd..d70101f0755 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1327,15 +1327,56 @@ static void prepare_cover_text(struct pretty_print_context *pp,
>  	strbuf_release(&subject_sb);
>  }
>  
> +struct rdiff_notes {
> +	/*
> +	 * True if we want to override the notes behavior
> +	 * of 'format-patch'
> +	 */
> +	bool override;
> +	struct string_list notes;
> +};
> +
> +static int rdiff_notes_cb(const struct option *option,
> +		       const char *arg,
> +		       int unset)
> +{
> +	struct rdiff_notes *rdiff_notes = option->value;
> +
> +	rdiff_notes->override = 1;
> +
> +	/*
> +	 * The rest is the same as
> +	 * parse-options-cb.c:parse_opt_string_list
> +	 */

Hmph, I wonder if it is more future-proof to wrap the string-list
callback like so ...

        static int rdiff_notes_cb(const struct option *option,
                               const char *arg,
                               int unset)
        {
                struct option opt = *option;
                struct rdiff_notes *rdiff_notes = opt.value;

                rdiff_notes->override = 1;
                opt.value = &rdiff_notes->notes;
                return parse_opt_string_list(&opt, arg, unset);
        }

... than copying and letting the code drift apart.