Re: [PATCH v3 1/2] format-patch: simplify get_notes_arg parameters

> 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.

>[snip]
>> +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.