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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2025, 16:55 UTC
Message-ID: <xmqqtt3x185d.fsf@gitster.g>
In-Reply-To: <1ec8a1384862765795f17e2d0fcb7b601ef2b25f.1751128487.git.gitgitgadget@gmail.com>

"Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Leon Michalak <leonmichalak6@gmail.com>
>
> Various builtins that use add-patch infrastructure do not respect
> the user's diff.context and diff.interHunkContext file configurations.

Great. "add-patch.c" invokes "diff-files", "diff-index" plumbing commands to do its thing, and these plumbing commands deliberately ignore such configuration variables, unlike "diff" Porcelain command that is meant for end-user consumption.

> This patch fixes this inconsistency.

If we were spelling it out, we would say "Fix this inconsistency" in imperative. But you never talked about "this inconsistency" so far, so it is not just confusing. It hints an incorrect conclusion that the difference between plumbing diff-{files,index,tree} and Porcelain diff is an inconsistency that needs to be "fixed", which is not true.

Follow the first paragraph with an explanation why it is a bad thing. For example:

    The user may be used to seeing their diffs with customized
    context size, but not in the patches "git add -p" shows them to
    pick from.

That would implicitly tell readers that we would want the patch shown by "add -p" generated with diff.context given by the user. So we can outline the solution next.

    Teach add-patch infrastructure to read these configuration
    variables and pass their values when spawning the underlying
    plumbing commands as their command line option.

or something.

Show 10 quoted lines

> @@ -39,8 +39,12 @@ static void init_color(struct repository *r, struct add_i_state *s,
>  void init_add_i_state(struct add_i_state *s, struct repository *r)
>  {
>  	const char *value;
> +	int context;
> +	int interhunkcontext;
>  
>  	s->r = r;
> +	s->context = -1;
> +	s->interhunkcontext = -1;

Hmph, context/interhunkcontext variables serve no purpose other than peeking into the value before assigning it to s->{context,interhunkcontext} members. In a sense, they may be confusing than they are worth.

Show 12 quoted lines

>  	if (repo_config_get_value(r, "color.interactive", &value))
>  		s->use_color = -1;
> @@ -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;
> +	}

Would the code be easier to understand if it is written more like

	if (!repo_config_get_int(r, "diff.context", &s->context)) {
		if (s->context < 0)
			die(...);
	}

with or without {braces} around the (technically) single statement block?

Show 6 quoted lines

> +	if (!repo_config_get_int(r, "diff.interHunkContext", &interhunkcontext)) {
> +		if (interhunkcontext < 0)
> +			die(_("%s cannot be negative"), "diff.interHunkContext");
> +		else
> +			s->interhunkcontext = interhunkcontext;
> +	}

Ditto.

Show 33 quoted lines

> diff --git a/add-interactive.h b/add-interactive.h
> index 693f125e8e4b..c63f35b14be8 100644
> --- a/add-interactive.h
> +++ b/add-interactive.h
> @@ -18,6 +18,7 @@ struct add_i_state {
>  
>  	int use_single_key;
>  	char *interactive_diff_filter, *interactive_diff_algorithm;
> +	int context, interhunkcontext;
>  };
>  
>  void init_add_i_state(struct add_i_state *s, struct repository *r);
> 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);

Ditto. What does it buy us to have these two local variables? We have the state object 's' available to us here, right?

Thanks.