Hi Leon
On 05/05/2025 10:18, Leon Michalak via GitGitGadget wrote:
From: Leon Michalak <leonmichalak6@xxxxxxxxx>
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@xxxxxxxxx>
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
