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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 6, 2023, 16:27 UTC
Message-ID: <19f6b40b-d8e8-e86c-5a9a-b35f42909118@dunelm.org.uk>
In-Reply-To: <8e73793b0db3e84366a9c6441cc0fdc04f9614a5.1675568781.git.gitgitgadget@gmail.com>

Hi John

On 05/02/2023 03:46, John Cai via GitGitGadget wrote:

Show 5 quoted lines

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

Show 7 quoted lines

> 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

Show 30 quoted lines

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

Show 12 quoted lines

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

Best Wishes

Phillip

> +	'
>   }
>