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

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2025, 21:36 UTC
Message-ID: <xmqqiklrvwl4.fsf@gitster.g>
In-Reply-To: <be89c3349d24e47a38f04538d3d1e3730dfc37a9.1748028010.git.code@khaugsbakk.name>

kristofferhaugsbakk@fastmail.com writes:

Show 11 quoted lines

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

Show 7 quoted lines

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