Re: [PATCH] squash! diff: add --diff-line-prefix option for passing in a prefix

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 15, 2016, 16:20 UTC
Message-ID: <xmqqfuq6humf.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1608151519480.4924@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 9 quoted lines

> On Sun, 14 Aug 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> 
>> > -	test=$(echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g')
>> > +	test=$(echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g' -e 'y/>/_/')
>> ...
> I know that this is so because my first iteration of the patch did exactly
> what you suggested.

You probably should have stepped back and taken a deep breath before writing the second round. Doing so after writing it before sending would also have been OK.

Among three characters that are special cased here, the problem "if we squish a run of problematic characters into one underscore, we risk making the result ambiguous" is NOT limited to '>'; it is not a new problem with '>', either. I can think of two other possible solutions offhand:

 (1) drop "squishing a run", i.e. [/ ]*, from the regexp, and rename
     existing test vectors that would be affected;

 (2) change the string used in the offending test so that squishing
     will not make the result ambiguous.

before special casing "y/>/_/"; as there is nothing in ">" that inherently causes the ambiguity that won't be caused by "/" or " ", adding second clause to the sed expression that does things differently only for ">" is simply wrong. After all, you only wanted to affect the "prefix=-->" test and not the whole set of the tests in the script.

Obviously (1) is a lot of impact with little gain, and as Jacob already offered to do, I think (2) is a lot more sensible solution and it also is more in line with your "If it isn't broken, do not fix it", I would say.