Re: [PATCH 4/4] blame: test the -b option, use blank oid for boundary commits.

From: Philip Oakley <philipoakley@iee.email>
Date: May 27, 2020, 10:52 UTC
Message-ID: <aabb67b8-b342-aab9-6213-f7c2d5758719@iee.email>
In-Reply-To: <20200527073020.GB4006199@coredump.intra.peff.net>

On 27/05/2020 08:30, Jeff King wrote:

Show 6 quoted lines

> On Mon, May 25, 2020 at 10:57:51PM +0100, Philip Oakley wrote:
>
>> The sed script removes the last hex digit from boundary commit oids
>> '^hexx msg' -> '^hex  msg' until all leading hex's are gone, finally
>> removing the boundary commit marker.
> Thanks for documenting this, as the sed was rather hard to read:

It was hard to write as well;-) lots of dead ends in the sed language.

Show 16 quoted lines

>
>> +test_expect_success 'test -b option, blank oid for boundary commits' '
>> +	git blame -b branch1.. -- file >actual &&
>> +	git blame branch1.. -- file >full &&
>> +	sed -e "/^\^/{
>> +		:loop;
>> +		s/^\(\^[0-9a-f]*\)[0-9a-f] \(.*\)/\1  \2/g;
>> +		tloop;
>> +		s/^\^/ /;
>> +	}" full >expected &&
> I wonder if we can make it simpler.
>
> In perl I'd probably just replace the whole string with the equivalent
> number of spaces, like:
>
>   perl -pe 's/^\^\S+/" " x length($&)/e'

I'm not a perl person, so I thought sed might be an easy target but ... it wasn't.

>
> but I suppose some would consider that pretty magical, too.

I'd need to look it up!

Show 8 quoted lines

> It might be
> simpler still to just avoid testing leading whitespace:
>
>    sed 's/^\^[0-9a-f]* *//' <full >expected &&
>    sed 's/^ *//' <actual >actual.stripped &&
>    test_cmp expected actual.stripped
>
> but perhaps the indentation is a useful part of what we're testing.

The code does take care to exactly match indentation, so I felt that the indentation should be tested.

Philip