From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 13 May 2025 13:52:25 GMT
Subject: Re: [PATCH v2 3/4] add-patch: respect diff.context configuration
Message-ID: <f00cdfa1-343a-4fa0-bce5-e06d1ed62c22@gmail.com>
In-Reply-To: <f16d3de86110ee61599459a25764248b52883b52.1746884789.git.gitgitgadget@gmail.com>

Hi Leon

On 10/05/2025 14:46, Leon Michalak via GitGitGadget wrote:
> From: Leon Michalak <leonmichalak6@gmail.com>
> 
> Various builtins that use add-patch infrastructure do not respect
> the user's diff.context and diff.interHunkContext file configurations.

We could expand this slightly by adding

This is because the plumbing commands used by "git add -p" to generate
the diff do not read those config settings. Fix this by reading the
config before generating the patch and passing it along to the diff
command with the "-U" and "--inter-hunk-context" command-line options.

> This patch fixes this inconsistency.
> 
> Signed-off-by: Leon Michalak <leonmichalak6@gmail.com>
> ---

> @@ -78,6 +82,19 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
>   	repo_config_get_string(r, "diff.algorithm",
>   			       &s->interactive_diff_algorithm);
>   
> +	if (!repo_config_get_int(r, "diff.context", &context)) {
> +		if (context < 0)
> +			die(_("%s cannot be negative"), "diff.context");
> +		else
> +			s->context = context;
> +	};
> +	if (!repo_config_get_int(r, "diff.interHunkContext", &interhunkcontext)) {
> +		if (interhunkcontext < 0)
> +			die(_("%s cannot be negative"), "diff.interHunkContext");
> +		else
> +			s->interhunkcontext = interhunkcontext;
> +	};

Thanks for changing this. This iteration of the code changes looks good

> diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
> index 1384a8195705..c4b861c360cc 100755
> --- a/t/t4055-diff-context.sh
> +++ b/t/t4055-diff-context.sh
> @@ -52,6 +52,46 @@ test_expect_success 'diff.context honored by "log"' '
>   	test_grep "^ firstline" output
>   '

It's great that you have written tests for this patch but as I said
last time I think the new tests should be in t3701-add-interactive.sh
as we're interested in testing whether "git add -p" passes on
diff.context to "git diff" , not whether "git diff" respects
diff.context. I still think there are too many tests here as we know
that all the different "-p" commands share a single code path. Our
test suite is slow enough already so we do not want to add new tests
that do not increase our code coverage. I would suggest removing
these tests and instead add the following in t3701

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 &&
	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
'

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
'

> +test_expect_success 'negative integer config parsing by "add"' '

Perhaps "add -p rejects negative diff.context" would be clearer?

> +	test_config diff.context -1 &&
> +	test_must_fail git add -p 2>output &&
> +	test_grep "diff.context cannot be negative" output
> +'

This is great but again we only need to test a single command and we
should do so in t3701. We should also check that negative values of
diff.interHunkContext are also rejected.

Best Wishes

Phillip