Re: [PATCH] diff: allow --color-moved with --no-ext-diff

From: René Scharfe <l.s.r@web.de>
Date: Jun 24, 2024, 19:15 UTC
Message-ID: <48948980-ac98-452a-b2df-11cd81de56da@web.de>
In-Reply-To: <xmqqsex2cnwk.fsf@gitster.g>

Am 24.06.24 um 18:21 schrieb Junio C Hamano:

Show 17 quoted lines

> René Scharfe <l.s.r@web.de> writes:
>
>> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
>> index b443626afd..a1478680b6 100755
>> --- a/t/t4015-diff-whitespace.sh
>> +++ b/t/t4015-diff-whitespace.sh
>> @@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' '
>>  	test_cmp expected actual
>>  '
>>
>> +test_expect_success '--color-moved with --no-ext-diff' '
>> +	test_config color.diff.oldMoved "normal red" &&
>> +	test_config color.diff.newMoved "normal green" &&
>
> We are making sure we won't be affected by previous tests.  We
> assume that we did not set color.diff.{old,new} to these two colors,
> but that would be an OK assumption to make.

The previous test also uses test_config to set these values, which means they get cleared at its end. We need to set them again to reuse the actual.raw file.

Show 9 quoted lines

>> +	cp actual.raw expect &&
>
> But then this introduces a dependence to an earlier _specific_ test,
> the one that created this version (among three) of actual.raw;>
> If we did this instead
>
> 	git diff --color --color-moved=zebra --no-renames HEAD >expect &&
>
> it would make this a lot more self-contained.

Oh, yes, good idea! We'd still rely on there being staged differences that include moved lines,

Show 10 quoted lines

>> +	git -c diff.external=false diff HEAD --no-ext-diff \
>> +		--color-moved=zebra --color --no-renames >actual &&
>
> Also, please do stick to the normal CLI ocnvention, dashed options
> come before the revs, i.e.
>
> 	git -c diff.external=false diff --no-ext-diff --color \
> 		--color-moved=zebra --no-renames HEAD >actual &&
>
> Our tests shouldn't be setting a wrong example.

I mimicked the style of the previous test to hint that we do the same here, just with the diff.external/--no-ext-diff noop on top. Not close enough, perhaps, but also hard to spot with 40+ lines between them.

René