Hi Leon
Thanks for working on this. I've left a few comments below but for your
first submission this is very good.
On 05/05/2025 10:18, Leon Michalak via GitGitGadget wrote:
From: Leon Michalak <leonmichalak6@xxxxxxxxx>
This aims to teach relevant builtins (that take in `--patch`) to respect
the user's diff.context and diff.interHunkContext file configurations.
This is a good summary of the intent of the patch. I'd maybe drop "This
aims" and instead say that the various commands do not respect the
config and this patch fixes that.
Since these are both UI options and `--patch` is designed for the end user,
I believe this was previously just an inconsistency, which this patch hopes
to address.
Signed-off-by: Leon Michalak <leonmichalak6@xxxxxxxxx>
---
add-interactive.c | 16 ++++++++++----
add-patch.c | 6 ++++++
t/t4055-diff-context.sh | 48 ++++++++++++++++++++++++++++++++++++++++-
3 files changed, 65 insertions(+), 5 deletions(-)
diff --git a/add-interactive.c b/add-interactive.c
index 97ff35b6f12a..ad12dc416598 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -41,6 +41,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
const char *value;
s->r = r;
+ s->context = -1;
+ s->interhunkcontext = -1;
if (repo_config_get_value(r, "color.interactive", &value))
s->use_color = -1;
@@ -78,6 +80,9 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
repo_config_get_string(r, "diff.algorithm",
&s->interactive_diff_algorithm);
+ repo_config_get_int(r, "diff.context", &s->context);
+ repo_config_get_int(r, "diff.interHunkContext", &s->interhunkcontext);
This looks good
@@ -1014,10 +1019,13 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
if (count > 0) {
struct child_process cmd = CHILD_PROCESS_INIT;
- strvec_pushl(&cmd.args, "git", "diff", "-p", "--cached",
- oid_to_hex(!is_initial ? &oid :
- s->r->hash_algo->empty_tree),
- "--", NULL);
+ strvec_pushl(&cmd.args, "git", "diff", "-p", "--cached", NULL);
As we're running "git diff" here nothing needs to be changed because it
reads all of the relevant config variables itself. This is why the
existing code does not worry about adding "--algorithm".
diff --git a/add-patch.c b/add-patch.c
index 95c67d8c80c4..b43ca1600738 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -415,6 +415,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
{
struct strvec args = STRVEC_INIT;
const char *diff_algorithm = s->s.interactive_diff_algorithm;
+ int diff_context = s->s.context;
+ int diff_interhunkcontext = s->s.interhunkcontext;
struct strbuf *plain = &s->plain, *colored = NULL;
struct child_process cp = CHILD_PROCESS_INIT;
char *p, *pend, *colored_p = NULL, *colored_pend = NULL, marker = '\0';
@@ -424,6 +426,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
int res;
strvec_pushv(&args, s->mode->diff_cmd);
+ if (diff_context != -1)
+ strvec_pushf(&args, "--unified=%i", diff_context);
+ if (diff_interhunkcontext != -1)
+ strvec_pushf(&args, "--inter-hunk-context=%i", diff_interhunkcontext);
This is good - if the user has set diff.config or diff.interhunkcontext
then we pass the appropriate values to "git diff-index" or "git
diff-files" which unlike "git diff" do not read those config values
themselves.
if (diff_algorithm)
strvec_pushf(&args, "--diff-algorithm=%s", diff_algorithm);
if (s->revision) {
diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
index ec2804eea67c..9c024200ade7 100755
--- a/t/t4055-diff-context.sh
+++ b/t/t4055-diff-context.sh
I'm not sure this is the best home for the new tests. Normally tests
related to the code in add-patch.c go into t3701-add-interactive.sh
@@ -49,7 +49,53 @@ test_expect_success 'diff.context honored by "log"' '
! grep firstline output &&
git config diff.context 8 &&
git log -1 -p >output &&
- grep "^ firstline" output
+ grep "^ firstline" output &&
+ git config --unset diff.context
+'
+
+test_expect_success 'diff.context honored by "add"' '
+ git add -p >output &&
+ ! grep firstline output &&
+ git config diff.context 8 &&
+ git add -p >output &&
+ grep "^ firstline" output &&
+ git config --unset diff.context
Eric has already mentioned using "test_config", I would go one step
further and say that as we're only running a single command we should
use "git add -c diff.context=* add -p" to avoid having to change the on
disk config at all.
I would also recommend using "test_grep ..." rather than "grep ..." and
"test_grep ! .." instead of "! grep ..." as they provide useful
debugging information if the test fails.
Given the way the code is structured with the config being read in
add-patch.c I'm not sure how much value there is in testing all the
different "-p" commands. It would however be very useful to check that
"git -c diff.interhunkcontext=8 add -p" works as expected.
I'll leave it there for now, I'll try and look at the other patches
later today or maybe tomorrow.
Best Wishes
Phillip
+'
+
+test_expect_success 'diff.context honored by "commit"' '
+ ! git commit -p >output &&
+ ! grep firstline output &&
+ git config diff.context 8 &&
+ ! git commit -p >output &&
+ grep "^ firstline" output &&
+ git config --unset diff.context
+'
+
+test_expect_success 'diff.context honored by "checkout"' '
+ git checkout -p >output &&
+ ! grep firstline output &&
+ git config diff.context 8 &&
+ git checkout -p >output &&
+ grep "^ firstline" output &&
+ git config --unset diff.context
+'
+
+test_expect_success 'diff.context honored by "stash"' '
+ ! git stash -p >output &&
+ ! grep firstline output &&
+ git config diff.context 8 &&
+ ! git stash -p >output &&
+ grep "^ firstline" output &&
+ git config --unset diff.context
+'
+
+test_expect_success 'diff.context honored by "restore"' '
+ git restore -p >output &&
+ ! grep firstline output &&
+ git config diff.context 8 &&
+ git restore -p >output &&
+ grep "^ firstline" output &&
+ git config --unset diff.context
'
test_expect_success 'The -U option overrides diff.context' '
