Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 30, 2022, 14:14 UTC
- Message-ID
- <p11s25nn-15qr-o2q1-5700-1q581sr9331n@tzk.qr>
- In-Reply-To
- <xmqqilmbotl0.fsf@gitster.g>
Hi Junio,
On Mon, 29 Aug 2022, Junio C Hamano wrote:
Show 13 quoted lines
> 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.
Show 6 quoted lines
> 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).
Show 7 quoted lines
> 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