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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Leon

On 05/05/2025 10:18, Leon Michalak via GitGitGadget wrote:
> 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.

> +`-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[]

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

> 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

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

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

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

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

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