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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 24, 2024, 16:21 UTC
Message-ID: <xmqqsex2cnwk.fsf@gitster.g>
In-Reply-To: <8a8bd51e-9ce5-4a68-bfbe-f16dcbb7e89c@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 11 quoted lines

> 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.

> +	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.

> +	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.

Show 6 quoted lines

> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'detect malicious moved code, inside file' '
>  	test_config color.diff.oldMoved "normal red" &&
>  	test_config color.diff.newMoved "normal green" &&

Other than that, looking very good.

Thanks.