From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 22 Jul 2025 16:01:55 GMT
Subject: Re: [PATCH v4 4/4] add-patch: add diff.context command line overrides
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:
> 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

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

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

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

> 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