Re: [PATCH 1/3] add-patch: respect diff.context configuration

From: Phillip Wood <phillip.wood123@gmail.com>
Date: May 6, 2025, 08:55 UTC
Message-ID: <c5591cbb-7eb9-4b69-85a1-ae1678983b14@gmail.com>
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 <leonmichalak6@gmail.com>
> 
> 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.

Show 30 quoted lines

> 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@gmail.com>
> ---
>   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

Show 9 quoted lines

> @@ -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".

Show 21 quoted lines

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

Show 7 quoted lines

>   	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

Show 16 quoted lines

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

Show 39 quoted lines

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