Re: [PATCH 2/3] add-patch: add diff.context command line overrides

From: Phillip Wood <phillip.wood123@gmail.com>
Date: May 7, 2025, 09:51 UTC
Message-ID: <61fe7690-87af-4159-be87-cd39c09475fe@gmail.com>
In-Reply-To: <7700eb173e73bb240852dc1c7ce26f3d1f95d8ca.1746436719.git.gitgitgadget@gmail.com>

Hi Leon

On 05/05/2025 10:18, Leon Michalak via GitGitGadget wrote:

Show 15 quoted lines

> From: Leon Michalak <leonmichalak6@gmail.com>
> 
> This patch compliments `8b91eef812`, where builtins that accept
> `--patch` options now respect `diff.context` and `diff.interHunkContext`
> file configurations.
> 
> In particular, this patch helps users who don't want to set persistent
> context configurations or just want a way to override them on a one-time
> basis, by allowing the relevant builtins to accept corresponding command
> line options that override the file configurations.
> 
> This mimics `diff` which allows for both context file configuration and
> command line overrides.
> 
> Signed-off-by: Leon Michalak <leonmichalak6@gmail.com>

I'm somewhat torn about the name "--unified" as I don't think it is particularly informative - it only makes sense if one knows the diff option and we only support a single diff format. Having said that it is probably better to reuse the name from "git diff" for consistency.

Show 10 quoted lines

> +`-U<n>`::
> +`--unified=<n>`::
> +	Generate diffs with _<n>_ lines of context. Defaults to `diff.context`
> +	or 3 if the config option is unset. Implies `--interactive/--patch`.
> +
> +`--inter-hunk-context=<n>`::
> +	Show the context between diff hunks, up to the specified _<number>_
> +	of lines, thereby fusing hunks that are close to each other.
> +	Defaults to `diff.interHunkContext` or 0 if the config option
> +	is unset. Implies `--interactive/--patch`.

This documentation is repeated for each command. I think it would be better to put this in separate file that is then included where it is needed. That way if we need to update the documentation in the future we only have one copy to worry about. The syntax to include a file called diff-context-options.adoc is

include::diff-context-options.adoc[]

Show 11 quoted lines

> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -86,6 +87,11 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
>   	repo_config_get_bool(r, "interactive.singlekey", &s->use_single_key);
>   	if (s->use_single_key)
>   		setbuf(stdin, NULL);
> +
> +	if (add_p_opt->context != -1)
> +		s->context = add_p_opt->context;
> +	if (add_p_opt->interhunkcontext != -1)
> +		s->interhunkcontext = add_p_opt->interhunkcontext;

This happens after we read the config so the command line option wins - good.

Show 8 quoted lines

> index 693f125e8e4b..653f07a917b8 100644
> --- a/add-interactive.h
> +++ b/add-interactive.h
> @@ -3,6 +3,13 @@
>   
>   #include "color.h"
>   
> +#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1 }

I think we normally define initializer macros after the struct they initialize so the reader can more easily check the fields are all initialized correctly

Show 13 quoted lines

> +struct add_p_opt {
> +	int context;
> +	int interhunkcontext;
> +};
> +
>   struct add_i_state {
>   	struct repository *r;
>   	int use_color;
> @@ -18,14 +25,17 @@ struct add_i_state {
>   
>   	int use_single_key;
>   	char *interactive_diff_filter, *interactive_diff_algorithm;
> +	int context, interhunkcontext;

Should this be in the previous patch? It is a good idea to run

     git rebase --keep-base --exec 'make && cd t && prove -j8 
tests-that-you-think-might-fail :: --root=/dev/shm'

before submitting a patch series so that you can be sure the patches all compile and the tests pass. Running the whole test suite can be pretty slow so just run the tests that are relevant to your changes.

Show 7 quoted lines

> @@ -169,9 +170,10 @@ int interactive_add(struct repository *repo,
>   		       prefix, argv);
>   
>   	if (patch)
> -		ret = !!run_add_p(repo, ADD_P_ADD, NULL, &pathspec);
> +		ret = !!run_add_p(repo, ADD_P_ADD, add_p_opt, NULL,
> +				  &pathspec);

This line could be unwraped and still fit within 80 columns I think.

Show 12 quoted lines

>   	else
> -		ret = !!run_add_i(repo, &pathspec);
> +		ret = !!run_add_i(repo, &pathspec, add_p_opt);
>   
>   	clear_pathspec(&pathspec);
>   	return ret;
> @@ -253,6 +255,12 @@ static struct option builtin_add_options[] = {
>   	OPT_GROUP(""),
>   	OPT_BOOL('i', "interactive", &add_interactive, N_("interactive picking")),
>   	OPT_BOOL('p', "patch", &patch_interactive, N_("select hunks interactively")),
> +	OPT_INTEGER_F('U', "unified", &add_p_opt.context,
> +		      N_("generate diffs with <n> lines context, implies --interactive/--patch"),

It cannot imply both --interactive and --patch as they are two different operations. I think it should imply --patch because that option is supported by all the commands we're adding -U to.

> +		      PARSE_OPT_NONEG),
> +	OPT_INTEGER_F(0, "inter-hunk-context", &add_p_opt.interhunkcontext,
> +		      N_("show context between diff hunks up to the specified number of lines, implies --interactive/--patch"),
> +		      PARSE_OPT_NONEG),

As these two options are duplicated in many commands we should define a preprocessor macro for them in parse-options.h. If you look at the end of that file there are a bunch of similar definitions.

Show 14 quoted lines

>   	OPT_BOOL('e', "edit", &edit_interactive, N_("edit current diff and apply")),
>   	OPT__FORCE(&ignored_too, N_("allow adding otherwise ignored files"), 0),
>   	OPT_BOOL('u', "update", &take_worktree_changes, N_("update tracked files")),
> @@ -398,7 +406,12 @@ int cmd_add(int argc,
>   			die(_("options '%s' and '%s' cannot be used together"), "--dry-run", "--interactive/--patch");
>   		if (pathspec_from_file)
>   			die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--interactive/--patch");
> -		exit(interactive_add(repo, argv + 1, prefix, patch_interactive));
> +		exit(interactive_add(repo, argv + 1, prefix, patch_interactive, &add_p_opt));
> +	} else {
> +		if (add_p_opt.context != -1)
> +			die(_("the option '%s' requires '%s'"), "--unified", "--interactive/--patch");
> +		if (add_p_opt.interhunkcontext != -1)
> +			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--interactive/--patch");

I don't understand this - I thought the help for --unified said it implied --patch but here it is erroring out if --patch is not given.

Somewhere we should check that the values given for the context and interhunk context are non-negative. git_diff_ui_config() errors out if the context is negative and we should do the same in add-patch.c otherwise we'll get some wierd error about "git diff-index" failing. We should also think about -U0 as "git apply" rejects hunks without any context unless by default and I dont think "add -p" passes the option to enable that and I'm not sure it should.

Show 5 quoted lines

> +test_expect_success 'The -U option overrides diff.context for "add"' '
> +	git config diff.context 8 &&
> +	git add -U4 -p >output &&
> +	! grep "^ firstline" output
> +'

It is great that you've added tests and I do think we should test all the commands here as unlike the previous patch there isn't a common code path. My other comments about the tests in the previous patch apply here though I think.

We should have a test to check negative context values and possibly zero are rejected.

Thanks

Phillip