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

[Phillip Wood <phillip.wood123@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.

> 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

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