Re: [PATCH 2/2] diff: teach diff to read gitattribute diff-algorithm

[John Cai <johncai86@gmail.com>]

Hi Phillip,

On 6 Feb 2023, at 11:27, Phillip Wood wrote:

> Hi John
>
> On 05/02/2023 03:46, John Cai via GitGitGadget wrote:
>> From: John Cai <johncai86@gmail.com>
>>
>> It can be useful to specify diff algorithms per file type. For example,
>> one may want to use the minimal diff algorithm for .json files, another
>> for .c files, etc.
>
> Have you got any examples of why this is useful? I find myself occasionally changing the algorithm when the default gives a sub-optimal diff but I've not really noticed any pattern with respect to file types.

At $DAYJOB, there has been a discussion and request for a feature like this [1].
One use case that came up was to be able to set a different diff algorithm for
.json files.

1. https://gitlab.com/gitlab-org/gitaly/-/issues/2591

>
>> Teach the diff machinery to check attributes for a diff algorithm.
>> Enforce precedence by favoring the command line option, then looking at
>> attributes, then finally the config.
>>
>> To enforce precedence order, set the `xdl_opts_command_line` member
>> during options pasing to indicate the diff algorithm was set via command
>> line args.
>
> I've only commented on the tests as it looks like Eric and had a careful look at the code
>
>> diff --git a/t/lib-diff-alternative.sh b/t/lib-diff-alternative.sh
>> index 8d1e408bb58..630c98ea65a 100644
>> --- a/t/lib-diff-alternative.sh
>> +++ b/t/lib-diff-alternative.sh
>> @@ -107,8 +107,27 @@ EOF
>>    	STRATEGY=$1
>>  +	test_expect_success "$STRATEGY diff from attributes" '
>> +		echo "file* diff-algorithm=$STRATEGY" >.gitattributes &&
>> +		test_must_fail git diff --no-index file1 file2 > output &&
>> +		test_cmp expect output
>> +	'
>> +
>>   	test_expect_success "$STRATEGY diff" '
>> -		test_must_fail git diff --no-index "--$STRATEGY" file1 file2 > output &&
>> +		test_must_fail git diff --no-index "--diff-algorithm=$STRATEGY" file1 file2 > output &&
>> +		test_cmp expect output
>> +	'
>> +
>> +	test_expect_success "$STRATEGY diff command line precedence before attributes" '
>> +		echo "file* diff-algorithm=meyers" >.gitattributes &&
>> +		test_must_fail git diff --no-index "--diff-algorithm=$STRATEGY" file1 file2 > output &&
>> +		test_cmp expect output
>> +	'
>> +
>> +	test_expect_success "$STRATEGY diff attributes precedence before config" '
>> +		git config diff.algorithm default &&
>> +		echo "file* diff-algorithm=$STRATEGY" >.gitattributes &&
>> +		test_must_fail git diff --no-index "--diff-algorithm=$STRATEGY" file1 file2 > output &&
>
> This test passes with or without the code changes in this patch. I think you need to drop --diff-algorithm=$STRATEGY from the diff command.

an oversight indeed, thanks for catching this.

>
>>   		test_cmp expect output
>>   	'
>>  @@ -166,5 +185,11 @@ EOF
>>   		test_must_fail git diff --no-index "--$STRATEGY" uniq1 uniq2 > output &&
>>   		test_cmp expect output
>>   	'
>> +
>> +	test_expect_success "$STRATEGY diff from attributes" '
>> +		echo "file* diff-algorithm=$STRATEGY" >.gitattributes &&
>> +		test_must_fail git diff --no-index uniq1 uniq2 > output &&
>> +		test_cmp expect output
>
> This test also passes with or without the code changes in this patch. It  is the same as the first test added above but with files that give the same diff irrespective of the algorithm chosen so I don't think it is doing anything useful. Unless I've missed something it should be dropped.

I should have been more thorough with this one as well, thanks.

>
> Best Wishes
>
> Phillip
>
>> +	'
>>   }
>>