Re: [PATCH v4 4/4] add-patch: add diff.context command line overrides

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 22, 2025, 16:01 UTC
Message-ID: <076bb5e9-e8c4-466d-b8dd-bc84bba708b2@gmail.com>
In-Reply-To: <2774b930406819ea60f522e0e0741e4046ee01ee.1752928113.git.gitgitgadget@gmail.com>

Hi Leon

On 19/07/2025 13:28, Leon Michalak via GitGitGadget wrote:

Show 13 quoted lines

> From: Leon Michalak <leonmichalak6@gmail.com>
> 
> This patch compliments the previous commit, where builtins that use
> add-patch infrastructure now respect diff.context and
> diff.interHunkContext file configurations.
> 
> In particular, this patch helps users who don't want to set persistent
> context configurations or just want a way to override them on a one-time
> basis, by allowing the relevant builtins to accept corresponding command
> line options that override the file configurations.
> 
> This mimics commands such as diff and log, which allow for both context
> file configuration and command line overrides.

Thanks for updating the quoting in the tests. Unfortunately this patch now deletes the tests added in the last commit which I don't think is correct.

>   
> -test_expect_success 'add -p respects diff.context' '
> -	test_write_lines a b c d e f g h i j k l m >file &&

I think there is some confusion here - why are we deleting the tests added in the last commit? This removes the test coverage for diff.context and diff.interHunkContext

Show 31 quoted lines

> +for cmd in add checkout restore 'commit -m file'
> +do
> +	test_expect_success "${cmd%% *} accepts -U and --inter-hunk-context" '
> +		test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
> +		git add file &&
> +		test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
> +		echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
> +			$cmd -p -U 4 --inter-hunk-context 2 >actual &&
> +		test_grep "@@ -2,20 +2,20 @@" actual
> +	'
> +done
> +
> +test_expect_success 'reset accepts -U and --inter-hunk-context' '
> +	test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
> +	git commit -m file file &&
> +	test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
>   	git add file &&
> -	test_write_lines a b c d e f G h i j k l m >file &&
> -	echo y | git -c diff.context=5 add -p >actual &&
> -	test_grep "@@ -2,11 +2,11 @@" actual
> +	echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
> +		reset -p -U 4 --inter-hunk-context 2 >actual &&
> +	test_grep "@@ -2,20 +2,20 @@" actual
>   '
>   
> -test_expect_success 'add -p respects diff.interHunkContext' '
> -	test_write_lines a b c d e f g h i j k l m n o p q r s >file &&
> -	git add file &&
> -	test_write_lines a b c d E f g i i j k l m N o p q r s >file &&
> -	echo y | git -c diff.interhunkcontext=2 add -p >actual &&
> -	test_grep "@@ -2,16 +2,16 @@" actual

This is also deleting a test added in the last patch

Show 14 quoted lines

> +test_expect_success 'stash accepts -U and --inter-hunk-context' '
> +	test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
> +	git commit -m file file &&
> +	test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
> +	echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
> +		stash -p -U 4 --inter-hunk-context 2 >actual &&
> +	test_grep "@@ -2,20 +2,20 @@" actual
>   '
>   
> -test_expect_success 'add -p rejects negative diff.context' '
> -	test_config diff.context -1 &&
> -	test_must_fail git add -p 2>output &&
> -	test_grep "diff.context cannot be negative" output
> -'

and so is this. The tests you're adding look good but we shouldn't be deleting the existing ones.

Show 17 quoted lines

> +for cmd in add checkout commit reset restore "stash save" "stash push"
> +do
> +	test_expect_success "$cmd rejects invalid context options" '
> +		test_must_fail git $cmd -p -U -3 2>actual &&
> +		cat actual | echo &&
> +		test_grep -e ".--unified. cannot be negative" actual &&
> +
> +		test_must_fail git $cmd -p --inter-hunk-context -3 2>actual &&
> +		test_grep -e ".--inter-hunk-context. cannot be negative" actual &&
> +
> +		test_must_fail git $cmd -U 7 2>actual &&
> +		test_grep -E ".--unified. requires .(--interactive/)?--patch." actual &&
> +
> +		test_must_fail git $cmd --inter-hunk-context 2 2>actual &&
> +		test_grep -E ".--inter-hunk-context. requires .(--interactive/)?--patch." actual
> +	'
> +done

This looks good as well

>   test_done

As I said last time I do not think the tests below add any value. They also do not compensate for the removal of the tests for diff.context that are deleted above as they all pass -U on the commandline.

Show 41 quoted lines

> diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
> index 1384a8195705..0158fe6568cb 100755
> --- a/t/t4055-diff-context.sh
> +++ b/t/t4055-diff-context.sh
> @@ -58,6 +58,36 @@ test_expect_success 'The -U option overrides diff.context' '
>   	test_grep ! "^ firstline" output
>   '
>   
> +test_expect_success 'The -U option overrides diff.context for "add"' '
> +	test_config diff.context 8 &&
> +	git add -U4 -p >output &&
> +	test_grep ! "^ firstline" output
> +'
> +
> +test_expect_success 'The -U option overrides diff.context for "commit"' '
> +	test_config diff.context 8 &&
> +	! git commit -U4 -p >output &&
> +	test_grep ! "^ firstline" output
> +'
> +
> +test_expect_success 'The -U option overrides diff.context for "checkout"' '
> +	test_config diff.context 8 &&
> +	git checkout -U4 -p >output &&
> +	test_grep ! "^ firstline" output
> +'
> +
> +test_expect_success 'The -U option overrides diff.context for "stash"' '
> +	test_config diff.context 8 &&
> +	! git stash -U4 -p >output &&
> +	test_grep ! "^ firstline" output
> +'
> +
> +test_expect_success 'The -U option overrides diff.context for "restore"' '
> +	test_config diff.context 8 &&
> +	git restore -U4 -p >output &&
> +	test_grep ! "^ firstline" output
> +'
> +
>   test_expect_success 'diff.context honored by "diff"' '
>   	test_config diff.context 8 &&
>   	git diff >output &&

Thanks

Phillip