Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Mon, 29 Aug 2022, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Here is why: in the _regular_ case, i.e. when we have a colored hunk
> > header like `@@ -936 +936 @@ wow()`, we manage to parse the line range,
> > and then record the offset of the extra part that starts afterwards.
> >
> > This extra part is non-empty, therefore we add an extra space.
> >
> > But that part already starts with a space, so now we end up with two
> > spaces.
>
> In other words, this breaks because the original code depended on
> having the extra whitespace before the "funcname" part.

Yes.

> Stepping back a bit, if the final goal for the UI generation out of
> this string is to append the material with a whitespace before it
> for better visual separation, then the original should probably have
> (at least conceptually) lstrip'ed the leading whitespaces from the
> string it found after " @@" and then appended the result to where it
> is showing, with its own single whitespace as a prefix.

Yes, with one twist: ANSI escape sequences can make lstrip'ing non-trivial
(granted, the line range parser totally ignores the fact that `@@<RESET> `
is a totally legitimate end of a colored line range).

> It would have prevented this bug from happening by future-proofing, and
> made the final output nicer, as any excess whitespaces between the " @@"
> and "funcname" would have been turned into a single SP.
>
> The "prepend one iff it does not already begin with a whitespace" is
> a (at least mentally to the developer) less expensive approach that
> is fine, too.

Yes, it is definitely less expensive.

Ciao,
Dscho