Hi Leon
On 10/05/2025 14:46, Leon Michalak via GitGitGadget wrote:
From: Leon Michalak <leonmichalak6@xxxxxxxxx>
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@xxxxxxxxx>
---
@@ -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
