Re: [PATCH v2 6/9] doc: notes: clearly state that --stripspace is the default

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

kristofferhaugsbakk@fastmail.com writes:

>       See the review on the patch:
>     
>          https://lore.kernel.org/git/xmqq4jp326oj.fsf@gitster.g/
>     
>       There was concern about the order of options:
>     
>           >     ... One more thing need to note is "the order of
>           >     the options matter", [...]
>     
>           This sounds more like a design/implementation mistake that we may
>           want to fix.

That is a comment on the 8th iteration of the series, which is
different from what you are documenting, which is 11th if I am
reading the thread correctly.

Comparing [v8 6/6] with [v11 6/7],

https://lore.kernel.org/git/f60f743203d78a489b90df81472e71391b45367d.1682429602.git.dyroneteng@gmail.com/
https://lore.kernel.org/git/19865941bb29c28a690ed3e28fc4f6870e352923.1685174012.git.dyroneteng@gmail.com/

I notice that the later version has .stripspace member in note_msg
as well as note_data.  note_data has a collection of multiple
note_msg instances.  Each of note_msg comes from various sources,
like '-m', '-C', etc., and each of these types have hardcoded
default values for note_msg.stripspace (mostly to strip but note_msg
that comes from an existing note blob has stripspace unset).  When
concat_messages() takes note_msg instances in a note_data together,
it _could_ do:

 - if note_data.stripspace is explicitly specified, use that,
   otherwise use note_msg.stripspace, to cleanse the message
   contained in note_msg.

and concatenate the results.  But even at v11 of the series, the
implementation of concat_messages() uses the stripspace determined
to instead cleanse the concatenated result.

Which I think is simply buggy.

What it should do probably is

 - Loop over d->messages[] and:
   - if d->stripspace is set or d->messages[]->stripspace is set,
     cleanse the message (i.e. d->messages[]->buf) and append to d->buf
   - otherwise append d->messages[]->buf as-is to d->buf

 - If d->stripspace is set, cleanse d->buf (may not be necessary, as
   the messages that are concatenated have _all_ lost leading and
   trailing blank lines before concatenation).

and that would truly fix the problem pointed out during the review
of v8.

> +--
> ++
> +`--stripspace` is the default except for
> +`-C`/`--reuse-message`. However, keep in mind that this depends on the
> +order of similar options. For example, for `-C <object> -m<message>`,
> +`--stripspace` will be used because the default for `-m` overrides the
> +previous `-C`.

So, I think this is an accurate description of the current
behaviour, but we might want to phrase the part that comes after
"however" as documenting a known bug, which we would want to fix
later.

Thanks.