From: Phillip Wood Date: Tue, 06 May 2025 08:55:52 GMT Subject: Re: [PATCH 1/3] add-patch: respect diff.context configuration Message-ID: In-Reply-To: <8b91eef8120b8f92db953ec983fddce8a442abcc.1746436719.git.gitgitgadget@gmail.com> 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 > > 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 > --- > 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' '