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

[Junio C Hamano <gitster@pobox.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.

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

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

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

Ditto.

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