Better support for customising context lines in --patch commands
#1915
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| `-U<n>`:: | ||
|
There was a problem hiding this comment. On the Git mailing list, Phillip Wood wrote (reply to this): Hi Leon
On 10/05/2025 14:46, Leon Michalak via GitGitGadget wrote:
> From: Leon Michalak <[email protected]>
> > This patch compliments the previous commit, where builtins that use
> add-patch infrastructure 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 commands such as diff and log, which allow for both context
> file configuration and command line overrides.
The code changes here mostly look good, I've left a few comments
below. I think the tests could be improved, I've left some suggestions
on limiting the number of tests while improving the coverage. The new
tests I'm suggesting that check invalid option combinations are the
basis for most of my code comments.
There is still the issue of what to do with -U0. As I mentioned
previously "git apply" will fail when we try to apply the patch. We
can either pass the appropriate flag when the context is zero or
possibly use -U0 to mean the default number of context lines.
> diff --git a/Documentation/diff-context-options.adoc b/Documentation/diff-context-options.adoc
> new file mode 100644
> index 000000000000..e161260358ff
> --- /dev/null
> +++ b/Documentation/diff-context-options.adoc
> @@ -0,0 +1,10 @@
> +`-U<n>`::
> +`--unified=<n>`::
> + Generate diffs with _<n>_ lines of context. Defaults to `diff.context`
> + or 3 if the config option is unset.
> +
> +`--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.
Nice - we reuse the same text for all the "-p" commands.
> diff --git a/add-interactive.c b/add-interactive.c
> [...]
> @@ -98,6 +99,17 @@ 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) {
> + if (add_p_opt->context < 0)
> + die(_("%s cannot be negative"), "--unified");
> + s->context = add_p_opt->context;
> + }
> + if (add_p_opt->interhunkcontext != -1) {
> + if (add_p_opt->interhunkcontext < 0)
> + die(_("%s cannot be negative"), "--inter-hunk-context");
> + s->interhunkcontext = add_p_opt->interhunkcontext;
> + }
Centralizing these checks like this is a good idea.
> @@ -1031,10 +1047,13 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
> if (count > 0) {
> struct child_process cmd = CHILD_PROCESS_INIT;
> > - strvec_pushl(&cmd.args, "git", "diff", "-p", "--cached",
> - oid_to_hex(!is_initial ? &oid :
> - s->r->hash_algo->empty_tree),
> - "--", NULL);
> + strvec_pushl(&cmd.args, "git", "diff", "-p", "--cached", NULL);
> + if (s->context != -1)
> + strvec_pushf(&cmd.args, "--unified=%i", s->context);
> + if (s->interhunkcontext != -1)
> + strvec_pushf(&cmd.args, "--inter-hunk-context=%i", s->interhunkcontext);
> + strvec_pushl(&cmd.args, oid_to_hex(!is_initial ? &oid :
> + s->r->hash_algo->empty_tree), "--", NULL);
This is good - we propagate the values we were given on the
command-line.
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> [...]
> @@ -564,8 +575,13 @@ static int checkout_paths(const struct checkout_opts *opts,
> else
> BUG("either flag must have been set, worktree=%d, index=%d",
> opts->checkout_worktree, opts->checkout_index);
> - return !!run_add_p(the_repository, patch_mode, rev,
> - &opts->pathspec);
> + return !!run_add_p(the_repository, patch_mode, &add_p_opt,
> + rev, &opts->pathspec);
> + } else {
> + if (opts->patch_context != -1)
> + die(_("the option '%s' requires '%s'"), "--unified", "--patch");
> + if (opts->patch_interhunk_context != -1)
> + die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
> }
This does not catch "git checkout -U 7" because this code is only run
if we're checking out paths. I think you need to check this is
checkout_main() instead.
> diff --git a/builtin/stash.c b/builtin/stash.c
> [...]
> @@ -1826,8 +1831,15 @@ static int push_stash(int argc, const char **argv, const char *prefix,
> die(_("the option '%s' requires '%s'"), "--pathspec-file-nul", "--pathspec-from-file");
> }
> > + if (!patch_mode) {
> + if (add_p_opt.context != -1)
> + die(_("the option '%s' requires '%s'"), "--unified", "--patch");
> + if (add_p_opt.interhunkcontext != -1)
> + die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
> + }
> +
This needs to die on invalid context values as "git stash" seems to
ignore the exit code of the subprocess that checks for negative values.
> @@ -1877,8 +1892,17 @@ static int save_stash(int argc, const char **argv, const char *prefix,
> stash_msg = strbuf_join_argv(&stash_msg_buf, argc, argv, ' ');
> > memset(&ps, 0, sizeof(ps));
> +
> + if (!patch_mode) {
> + if (add_p_opt.context != -1)
> + die(_("the option '%s' requires '%s'"), "--unified", "--patch");
> + if (add_p_opt.interhunkcontext != -1)
> + die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
This needs to die on invalid context values as "git stash" seems to
ignore the exit code of the subprocess that checks for negative values.
> diff --git a/commit.h b/commit.h
> [...]
> #include "object.h"
> +#include "add-interactive.h"
> struct signature_check;
> struct strbuf;
Lets not add this. Instead lets just add a declaration for "struct
add_p_opt" like the ones in the context line so that we don't end up
including everything from add-interactive.h when we only need a single
struct declaration.
> diff --git a/parse-options.h b/parse-options.h
> [...]
> +#define OPT_DIFF_UNIFIED(v) OPT_INTEGER_F('U', "unified", v,
> N_("generate diffs with <n> lines context"), PARSE_OPT_NONEG)
This looks good
> +#define OPT_DIFF_INTERHUNK_CONTEXT(v) OPT_INTEGER_F(0, "inter-hunk-context", v, N_("show context between diff hunks up to the specified number of lines"), PARSE_OPT_NONEG)
This is a bit verbose but it matches what is in diff.c.
> diff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh
> index bada0cbd32f7..d5aad6e143a7 100755
> --- a/t/t4032-diff-inter-hunk-context.sh
> +++ b/t/t4032-diff-inter-hunk-context.sh
> @@ -47,6 +47,31 @@ t() {
> "
> }
> > +t_patch() {
> + use_config=
> + git config --unset diff.interHunkContext
> +
> + case $# in
> + 4) hunks=$4; cmd="add -p -U$3";;
> + 5) hunks=$5; cmd="add -p -U$3 --inter-hunk-context=$4";;
> + 6) hunks=$5; cmd="add -p -U$3"; git config diff.interHunkContext $4; use_config="(diff.interHunkContext=$4) ";;
> + esac
> + label="$use_config$cmd, $1 common $2"
> + file=f$1
> +
> + if ! test -f $file
> + then
> + f A $1 B >$file
> + git add $file
> + git commit -q -m. $file
> + f X $1 Y >$file
> + fi
> +
> + test_expect_success "$label: count hunks ($hunks)" "
> + test $(test_write_lines q | git $cmd $file | sed -n 's/^([0-9]*\/\([0-9]*\)) Stage this hunk.*/\1/p') = $hunks
> + "
> +}
> +
> cat <<EOF >expected.f1.0.1 || exit 1
> diff --git a/f1 b/f1
> --- a/f1
> @@ -107,6 +132,42 @@ t 3 lines 1 2 1 config
> t 9 lines 3 2 2 config
> t 9 lines 3 3 1 config
> > +# common lines ctx intrctx hunks
> +t_patch 1 line 0 2
> +t_patch 1 line 0 0 2
> +t_patch 1 line 0 1 1
> +t_patch 1 line 0 2 1
> +t_patch 1 line 1 1
> +
> +t_patch 2 lines 0 2
> +t_patch 2 lines 0 0 2
> +t_patch 2 lines 0 1 2
> +t_patch 2 lines 0 2 1
> +t_patch 2 lines 1 1
> +
> +t_patch 3 lines 1 2
> +t_patch 3 lines 1 0 2
> +t_patch 3 lines 1 1 1
> +t_patch 3 lines 1 2 1
> +
> +t_patch 9 lines 3 2
> +t_patch 9 lines 3 2 2
> +t_patch 9 lines 3 3 1
> +
> +# use diff.interHunkContext?
> +t_patch 1 line 0 0 2 config
> +t_patch 1 line 0 1 1 config
> +t_patch 1 line 0 2 1 config
> +t_patch 9 lines 3 3 1 config
> +t_patch 2 lines 0 0 2 config
> +t_patch 2 lines 0 1 2 config
> +t_patch 2 lines 0 2 1 config
> +t_patch 3 lines 1 0 2 config
> +t_patch 3 lines 1 1 1 config
> +t_patch 3 lines 1 2 1 config
> +t_patch 9 lines 3 2 2 config
> +t_patch 9 lines 3 3 1 config
> +
There are 29 tests here and yet more below. I think we can
get the test coverage we need much more efficiently with the following
added to t3701
for cmd in add checkout restore 'commit -m file'
do
test_expect_success "${cmd%% *} accepts -U and --inter-hunk-context" "
test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
git add file &&
test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
$cmd -p -U 4 --inter-hunk-context 2 >actual &&
test_grep \"@@ -2,20 +2,20 @@\" actual
"
done
test_expect_success 'reset accepts -U and --inter-hunk-context' '
test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
git commit -m file file &&
test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
git add file &&
echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
reset -p -U 4 --inter-hunk-context 2 >actual &&
test_grep "@@ -2,20 +2,20 @@" actual
'
test_expect_success 'stash accepts -U and --inter-hunk-context' '
test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
git commit -m file file &&
test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
stash -p -U 4 --inter-hunk-context 2 >actual &&
test_grep "@@ -2,20 +2,20 @@" actual
'
Those tests will fail if any of the commands that accept "-p" do not
accept "-U" or "--inter-hunk-context" or if command-line arguments do
not override the config settings. We should also add tests in t3701 to
check that invalid option combinations and values are rejected like so
for cmd in add checkout commit reset restore 'stash save' 'stash push'
do
test_expect_success "$cmd rejects invalid context options" "
test_must_fail git $cmd -p -U -3 2>actual &&
test_grep -e \"--unified cannot be negative\" actual &&
test_must_fail git $cmd -p --inter-hunk-context -3 2>actual &&
test_grep -e \"--inter-hunk-context cannot be negative\" actual &&
test_must_fail git $cmd -U 7 2>actual &&
test_grep -E \".--unified. requires .(--interactive/)?--patch.\" actual &&
test_must_fail git $cmd --inter-hunk-context 2 2>actual &&
test_grep -E \".--inter-hunk-context. requires .(--interactive/)?--patch.\" actual
"
done
The "checkout", "stash save" and "stash push" tests above currently
fail because the implementation does not implement those checks
properly.
With a few tweaks this series will be looking very good
Best Wishes
PhillipThere was a problem hiding this comment. On the Git mailing list, Phillip Wood wrote (reply to this): On 13/05/2025 14:52, Phillip Wood wrote:
>> diff --git a/builtin/stash.c b/builtin/stash.c
>> [...]
>> @@ -1826,8 +1831,15 @@ static int push_stash(int argc, const char >> **argv, const char *prefix,
>> die(_("the option '%s' requires '%s'"), "--pathspec-file- >> nul", "--pathspec-from-file");
>> }
>> + if (!patch_mode) {
>> + if (add_p_opt.context != -1)
>> + die(_("the option '%s' requires '%s'"), "--unified", "-- >> patch");
>> + if (add_p_opt.interhunkcontext != -1)
>> + die(_("the option '%s' requires '%s'"), "--inter-hunk- >> context", "--patch");
>> + }
>> +
> > This needs to die on invalid context values as "git stash" seems to
> ignore the exit code of the subprocess that checks for negative values.
Looking more closely the problem is that it quits if there are no changes to stash before validating -U or --inter-hunk-context. I think it should validate the options before checking if there is anything to stash.
Best Wishes
Phillip
There was a problem hiding this comment. On the Git mailing list, Leon Michalak wrote (reply to this): Hey, thanks for the thorough review Philip. I will properly digest
this when I get some free time, but I just wanted to say (I probably
should have mentioned this so my bad) that the reason I didn't change
to test just the singular command (yet, anyway) is that someone else
thought this was a good idea testing all of them, so I wasn't sure
whether to touch it or not in the end, and thought I'd just submit
this v2 and gather more opinions. Was this perhaps the wrong approach
though?There was a problem hiding this comment. On the Git mailing list, [email protected] wrote (reply to this): Hi Leon
On 13/05/2025 16:05, Leon Michalak wrote:
> Hey, thanks for the thorough review Philip. I will properly digest
> this when I get some free time, but I just wanted to say (I probably
> should have mentioned this so my bad) that the reason I didn't change
> to test just the singular command (yet, anyway) is that someone else
> thought this was a good idea testing all of them,
I'd missed that message - have you got a link to it please
> so I wasn't sure
> whether to touch it or not in the end, and thought I'd just submit
> this v2 and gather more opinions. Was this perhaps the wrong approach
> though?
If you get conflicting advise then it is a good idea to mention that in the cover letter and explain which option you went with and why.
Best Wishes
Phillip
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): "Leon Michalak via GitGitGadget" <[email protected]> writes:
> From: Leon Michalak <[email protected]>
>
> This patch compliments the previous commit, where builtins that use
> add-patch infrastructure 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 commands such as diff and log, which allow for both context
> file configuration and command line overrides.
I skimmed the patch briefly. I am not sure if it is a good idea to
* add OPT_DIFF_*() macros to parse-options API, as its utility is
very narrow, and forces those who are learning parse-options API
to learn one more thing.
* validation of the value range to be duplicated for each and every
users of the new OPT_DIFF_*() macros.
but other than that, looked reasonable to me.
Thanks.There was a problem hiding this comment. On the Git mailing list, Phillip Wood wrote (reply to this): On 30/06/2025 18:03, Junio C Hamano wrote:
> "Leon Michalak via GitGitGadget" <[email protected]> writes:
> >> From: Leon Michalak <[email protected]>
>>
>> This patch compliments the previous commit, where builtins that use
>> add-patch infrastructure 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 commands such as diff and log, which allow for both context
>> file configuration and command line overrides.
> > I skimmed the patch briefly. I am not sure if it is a good idea to
> > * add OPT_DIFF_*() macros to parse-options API, as its utility is
> very narrow, and forces those who are learning parse-options API
> to learn one more thing.
It means that we have consistent help for all the commands with these options which I think is valuable. We have a number of other macros that define options that are shared between commands and I think that works quite well.
> > * validation of the value range to be duplicated for each and every
> users of the new OPT_DIFF_*() macros.
Yes the validation is awkward. If we changed the OPT_DIFF_* to use a callback that rejected negative values that would reduce the duplication.
> but other than that, looked reasonable to me.
I've left a couple of comments on the tests but the code changes look reasonable to me too
Thanks
Phillip
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <[email protected]> writes:
>> * add OPT_DIFF_*() macros to parse-options API, as its utility is
>> very narrow, and forces those who are learning parse-options API
>> to learn one more thing.
>
> It means that we have consistent help for all the commands with these
> options which I think is valuable. We have a number of other macros
> that define options that are shared between commands and I think that
> works quite well.
I understand that principe. What I was wondering was if there are
enough places to use these particular ones to make it worthwhile to
enlarge the set of OPT_* macros.
>> * validation of the value range to be duplicated for each and
>> every
>> users of the new OPT_DIFF_*() macros.
>
> Yes the validation is awkward. If we changed the OPT_DIFF_* to use a
> callback that rejected negative values that would reduce the
> duplication.
Yeah, I was wondering about that approach, too. Another benefit
with the "validate just after we parse the value before we assign
the result to a variable or a struct member" approach is that we can
also complain about -1 that is given from the command line (which
the current code ignores, if I am not mistaken, because it needs to
be silent if that -1 is there merely because it is the "not set yet"
sentinel value).
Or perhaps the valid value range Réne has been workingon canbe used
here?There was a problem hiding this comment. On the Git mailing list, Phillip Wood wrote (reply to this): On 01/07/2025 16:54, Junio C Hamano wrote:
> Phillip Wood <[email protected]> writes:
> >>> * add OPT_DIFF_*() macros to parse-options API, as its utility is
>>> very narrow, and forces those who are learning parse-options API
>>> to learn one more thing.
>>
>> It means that we have consistent help for all the commands with these
>> options which I think is valuable. We have a number of other macros
>> that define options that are shared between commands and I think that
>> works quite well.
> > I understand that principe. What I was wondering was if there are
> enough places to use these particular ones to make it worthwhile to
> enlarge the set of OPT_* macros.
There are six users of each of these macros so I think it is worthwhile. That's two more users than there are for OPT_RERERE_AUTOUPDATE() and twice as many users as OPT_CONTAINS().
>>> * validation of the value range to be duplicated for each and
>>> every
>>> users of the new OPT_DIFF_*() macros.
>>
>> Yes the validation is awkward. If we changed the OPT_DIFF_* to use a
>> callback that rejected negative values that would reduce the
>> duplication.
> > Yeah, I was wondering about that approach, too. Another benefit
> with the "validate just after we parse the value before we assign
> the result to a variable or a struct member" approach is that we can
> also complain about -1 that is given from the command line (which
> the current code ignores, if I am not mistaken, because it needs to
> be silent if that -1 is there merely because it is the "not set yet"
> sentinel value).
> > Or perhaps the valid value range Réne has been workingon canbe used
> here?
That would be nice
Thanks
PhillipThere was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <[email protected]> writes:
> On 01/07/2025 16:54, Junio C Hamano wrote:
>> Phillip Wood <[email protected]> writes:
>>
>>>> * add OPT_DIFF_*() macros to parse-options API, as its utility is
>>>> very narrow, and forces those who are learning parse-options API
>>>> to learn one more thing.
>>>
>>> It means that we have consistent help for all the commands with these
>>> options which I think is valuable. We have a number of other macros
>>> that define options that are shared between commands and I think that
>>> works quite well.
>> I understand that principe. What I was wondering was if there are
>> enough places to use these particular ones to make it worthwhile to
>> enlarge the set of OPT_* macros.
>
> There are six users of each of these macros so I think it is
> worthwhile. That's two more users than there are for
> OPT_RERERE_AUTOUPDATE() and twice as many users as OPT_CONTAINS().
OK. Thanks.There was a problem hiding this comment. On the Git mailing list, Phillip Wood wrote (reply to this): Hi Leon
On 28/06/2025 17:34, Leon Michalak via GitGitGadget wrote:
> From: Leon Michalak <[email protected]>
> > +for cmd in add checkout restore 'commit -m file'
> +do
> + test_expect_success "${cmd%% *} accepts -U and --inter-hunk-context" "
Looking at this again, I think the test bodies here and below should be wrapped in single quotes because they are passed to eval and we want to expand $cmd when the body is evaluated, not before. That would also simplify the quoting inside the tests as we don't need to escape double quotes. That's not your fault - you've just copied what I suggested before.
> +test_expect_success 'The -U option overrides diff.context for "add"' '
> + test_config diff.context 8 &&
> + git add -U4 -p >output &&
> + test_grep ! "^ firstline" output
> +'
Don't the tests above check this as they set diff.context and diff.interhunkcontext and pass different values to -U and --inter-hunk-context?
Thanks
PhillipThere was a problem hiding this comment. On the Git mailing list, Phillip Wood wrote (reply to this): Hi Leon
On 19/07/2025 13:28, Leon Michalak via GitGitGadget wrote:
> From: Leon Michalak <[email protected]>
> > This patch compliments the previous commit, where builtins that use
> add-patch infrastructure 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 commands such as diff and log, which allow for both context
> file configuration and command line overrides.
Thanks for updating the quoting in the tests. Unfortunately this patch now deletes the tests added in the last commit which I don't think is correct.
> > -test_expect_success 'add -p respects diff.context' '
> - test_write_lines a b c d e f g h i j k l m >file &&
I think there is some confusion here - why are we deleting the tests added in the last commit? This removes the test coverage for diff.context and diff.interHunkContext
> +for cmd in add checkout restore 'commit -m file'
> +do
> + test_expect_success "${cmd%% *} accepts -U and --inter-hunk-context" '
> + test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
> + git add file &&
> + test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
> + echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
> + $cmd -p -U 4 --inter-hunk-context 2 >actual &&
> + test_grep "@@ -2,20 +2,20 @@" actual
> + '
> +done
> +
> +test_expect_success 'reset accepts -U and --inter-hunk-context' '
> + test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
> + git commit -m file file &&
> + test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
> git add file &&
> - test_write_lines a b c d e f G h i j k l m >file &&
> - echo y | git -c diff.context=5 add -p >actual &&
> - test_grep "@@ -2,11 +2,11 @@" actual
> + echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
> + reset -p -U 4 --inter-hunk-context 2 >actual &&
> + test_grep "@@ -2,20 +2,20 @@" actual
> '
> > -test_expect_success 'add -p respects diff.interHunkContext' '
> - test_write_lines a b c d e f g h i j k l m n o p q r s >file &&
> - git add file &&
> - test_write_lines a b c d E f g i i j k l m N o p q r s >file &&
> - echo y | git -c diff.interhunkcontext=2 add -p >actual &&
> - test_grep "@@ -2,16 +2,16 @@" actual
This is also deleting a test added in the last patch
> +test_expect_success 'stash accepts -U and --inter-hunk-context' '
> + test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
> + git commit -m file file &&
> + test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
> + echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
> + stash -p -U 4 --inter-hunk-context 2 >actual &&
> + test_grep "@@ -2,20 +2,20 @@" actual
> '
> > -test_expect_success 'add -p rejects negative diff.context' '
> - test_config diff.context -1 &&
> - test_must_fail git add -p 2>output &&
> - test_grep "diff.context cannot be negative" output
> -'
and so is this. The tests you're adding look good but we shouldn't be deleting the existing ones.
> +for cmd in add checkout commit reset restore "stash save" "stash push"
> +do
> + test_expect_success "$cmd rejects invalid context options" '
> + test_must_fail git $cmd -p -U -3 2>actual &&
> + cat actual | echo &&
> + test_grep -e ".--unified. cannot be negative" actual &&
> +
> + test_must_fail git $cmd -p --inter-hunk-context -3 2>actual &&
> + test_grep -e ".--inter-hunk-context. cannot be negative" actual &&
> +
> + test_must_fail git $cmd -U 7 2>actual &&
> + test_grep -E ".--unified. requires .(--interactive/)?--patch." actual &&
> +
> + test_must_fail git $cmd --inter-hunk-context 2 2>actual &&
> + test_grep -E ".--inter-hunk-context. requires .(--interactive/)?--patch." actual
> + '
> +done
This looks good as well
> test_done
As I said last time I do not think the tests below add any value. They also do not compensate for the removal of the tests for diff.context that are deleted above as they all pass -U on the commandline.
> diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
> index 1384a8195705..0158fe6568cb 100755
> --- a/t/t4055-diff-context.sh
> +++ b/t/t4055-diff-context.sh
> @@ -58,6 +58,36 @@ test_expect_success 'The -U option overrides diff.context' '
> test_grep ! "^ firstline" output
> '
> > +test_expect_success 'The -U option overrides diff.context for "add"' '
> + test_config diff.context 8 &&
> + git add -U4 -p >output &&
> + test_grep ! "^ firstline" output
> +'
> +
> +test_expect_success 'The -U option overrides diff.context for "commit"' '
> + test_config diff.context 8 &&
> + ! git commit -U4 -p >output &&
> + test_grep ! "^ firstline" output
> +'
> +
> +test_expect_success 'The -U option overrides diff.context for "checkout"' '
> + test_config diff.context 8 &&
> + git checkout -U4 -p >output &&
> + test_grep ! "^ firstline" output
> +'
> +
> +test_expect_success 'The -U option overrides diff.context for "stash"' '
> + test_config diff.context 8 &&
> + ! git stash -U4 -p >output &&
> + test_grep ! "^ firstline" output
> +'
> +
> +test_expect_success 'The -U option overrides diff.context for "restore"' '
> + test_config diff.context 8 &&
> + git restore -U4 -p >output &&
> + test_grep ! "^ firstline" output
> +'
> +
> test_expect_success 'diff.context honored by "diff"' '
> test_config diff.context 8 &&
> git diff >output &&
Thanks
Phillip
There was a problem hiding this comment. On the Git mailing list, Leon Michalak wrote (reply to this): Hey Philip, looking again for a second time and re-reading previous
replies on this thread, I think I misunderstood a previous reply.
I will add these back as soon as I can get back to the PC, thanks for
spotting that!
On Tue, 22 Jul 2025 at 17:02, Phillip Wood <[email protected]> wrote:
>
> Hi Leon
>
> On 19/07/2025 13:28, Leon Michalak via GitGitGadget wrote:
> > From: Leon Michalak <[email protected]>
> >
> > This patch compliments the previous commit, where builtins that use
> > add-patch infrastructure 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 commands such as diff and log, which allow for both context
> > file configuration and command line overrides.
>
> Thanks for updating the quoting in the tests. Unfortunately this patch
> now deletes the tests added in the last commit which I don't think is
> correct.
>
> >
> > -test_expect_success 'add -p respects diff.context' '
> > - test_write_lines a b c d e f g h i j k l m >file &&
> I think there is some confusion here - why are we deleting the tests
> added in the last commit? This removes the test coverage for
> diff.context and diff.interHunkContext
>
> > +for cmd in add checkout restore 'commit -m file'
> > +do
> > + test_expect_success "${cmd%% *} accepts -U and --inter-hunk-context" '
> > + test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
> > + git add file &&
> > + test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
> > + echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
> > + $cmd -p -U 4 --inter-hunk-context 2 >actual &&
> > + test_grep "@@ -2,20 +2,20 @@" actual
> > + '
> > +done
> > +
> > +test_expect_success 'reset accepts -U and --inter-hunk-context' '
> > + test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
> > + git commit -m file file &&
> > + test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
> > git add file &&
> > - test_write_lines a b c d e f G h i j k l m >file &&
> > - echo y | git -c diff.context=5 add -p >actual &&
> > - test_grep "@@ -2,11 +2,11 @@" actual
> > + echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
> > + reset -p -U 4 --inter-hunk-context 2 >actual &&
> > + test_grep "@@ -2,20 +2,20 @@" actual
> > '
> >
> > -test_expect_success 'add -p respects diff.interHunkContext' '
> > - test_write_lines a b c d e f g h i j k l m n o p q r s >file &&
> > - git add file &&
> > - test_write_lines a b c d E f g i i j k l m N o p q r s >file &&
> > - echo y | git -c diff.interhunkcontext=2 add -p >actual &&
> > - test_grep "@@ -2,16 +2,16 @@" actual
>
> This is also deleting a test added in the last patch
>
> > +test_expect_success 'stash accepts -U and --inter-hunk-context' '
> > + test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
> > + git commit -m file file &&
> > + test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
> > + echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
> > + stash -p -U 4 --inter-hunk-context 2 >actual &&
> > + test_grep "@@ -2,20 +2,20 @@" actual
> > '
> >
> > -test_expect_success 'add -p rejects negative diff.context' '
> > - test_config diff.context -1 &&
> > - test_must_fail git add -p 2>output &&
> > - test_grep "diff.context cannot be negative" output
> > -'
>
> and so is this. The tests you're adding look good but we shouldn't be
> deleting the existing ones.
>
> > +for cmd in add checkout commit reset restore "stash save" "stash push"
> > +do
> > + test_expect_success "$cmd rejects invalid context options" '
> > + test_must_fail git $cmd -p -U -3 2>actual &&
> > + cat actual | echo &&
> > + test_grep -e ".--unified. cannot be negative" actual &&
> > +
> > + test_must_fail git $cmd -p --inter-hunk-context -3 2>actual &&
> > + test_grep -e ".--inter-hunk-context. cannot be negative" actual &&
> > +
> > + test_must_fail git $cmd -U 7 2>actual &&
> > + test_grep -E ".--unified. requires .(--interactive/)?--patch." actual &&
> > +
> > + test_must_fail git $cmd --inter-hunk-context 2 2>actual &&
> > + test_grep -E ".--inter-hunk-context. requires .(--interactive/)?--patch." actual
> > + '
> > +done
>
> This looks good as well
> > test_done
> As I said last time I do not think the tests below add any value. They
> also do not compensate for the removal of the tests for diff.context
> that are deleted above as they all pass -U on the commandline.
>
> > diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
> > index 1384a8195705..0158fe6568cb 100755
> > --- a/t/t4055-diff-context.sh
> > +++ b/t/t4055-diff-context.sh
> > @@ -58,6 +58,36 @@ test_expect_success 'The -U option overrides diff.context' '
> > test_grep ! "^ firstline" output
> > '
> >
> > +test_expect_success 'The -U option overrides diff.context for "add"' '
> > + test_config diff.context 8 &&
> > + git add -U4 -p >output &&
> > + test_grep ! "^ firstline" output
> > +'
> > +
> > +test_expect_success 'The -U option overrides diff.context for "commit"' '
> > + test_config diff.context 8 &&
> > + ! git commit -U4 -p >output &&
> > + test_grep ! "^ firstline" output
> > +'
> > +
> > +test_expect_success 'The -U option overrides diff.context for "checkout"' '
> > + test_config diff.context 8 &&
> > + git checkout -U4 -p >output &&
> > + test_grep ! "^ firstline" output
> > +'
> > +
> > +test_expect_success 'The -U option overrides diff.context for "stash"' '
> > + test_config diff.context 8 &&
> > + ! git stash -U4 -p >output &&
> > + test_grep ! "^ firstline" output
> > +'
> > +
> > +test_expect_success 'The -U option overrides diff.context for "restore"' '
> > + test_config diff.context 8 &&
> > + git restore -U4 -p >output &&
> > + test_grep ! "^ firstline" output
> > +'
> > +
> > test_expect_success 'diff.context honored by "diff"' '
> > test_config diff.context 8 &&
> > git diff >output &&
>
> Thanks
>
> Phillip
>There was a problem hiding this comment. On the Git mailing list, Leon Michalak wrote (reply to this): Apologies for the top-posting above, this is not my regular workflow!There was a problem hiding this comment. On the Git mailing list, Phillip Wood wrote (reply to this): Hi Leon
Here is a fixup commit for the tests which can be squashed into patch 4
Best Wishes
Phillip
---- 8< ----
From: Phillip Wood <[email protected]>
Subject: [PATCH] fixup! add-patch: add diff.context command line overrides
Restore the test coverage for diff.context and diff.interHunkContext
added in f08d4ae6e56 (add-patch: respect diff.context configuration,
2025-07-19) and remove the redunant tests in t4055 added by bd6d6ba1321
(add-patch: add diff.context command line overrides, 2025-07-19) which
duplicate the coverage of the tests added to t3071 in the same commit.
Signed-off-by: Phillip Wood <[email protected]>
---
t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++
t/t4055-diff-context.sh | 30 ------------------------------
2 files changed, 22 insertions(+), 30 deletions(-)
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 7fd9a23c998..04d2a198352 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1230,6 +1230,28 @@ test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '
test_cmp expect actual
'
+test_expect_success 'add -p respects diff.context' '
+ test_write_lines a b c d e f g h i j k l m >file &&
+ git add file &&
+ test_write_lines a b c d e f G h i j k l m >file &&
+ echo y | git -c diff.context=5 add -p >actual &&
+ test_grep "@@ -2,11 +2,11 @@" actual
+'
+
+test_expect_success 'add -p respects diff.interHunkContext' '
+ test_write_lines a b c d e f g h i j k l m n o p q r s >file &&
+ git add file &&
+ test_write_lines a b c d E f g i i j k l m N o p q r s >file &&
+ echo y | git -c diff.interhunkcontext=2 add -p >actual &&
+ test_grep "@@ -2,16 +2,16 @@" actual
+'
+
+test_expect_success 'add -p rejects negative diff.context' '
+ test_config diff.context -1 &&
+ test_must_fail git add -p 2>output &&
+ test_grep "diff.context cannot be negative" output
+'
+
for cmd in add checkout restore 'commit -m file'
do
test_expect_success "${cmd%% *} accepts -U and --inter-hunk-context" '
diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
index 0158fe6568c..1384a819570 100755
--- a/t/t4055-diff-context.sh
+++ b/t/t4055-diff-context.sh
@@ -58,36 +58,6 @@ test_expect_success 'The -U option overrides diff.context' '
test_grep ! "^ firstline" output
'
-test_expect_success 'The -U option overrides diff.context for "add"' '
- test_config diff.context 8 &&
- git add -U4 -p >output &&
- test_grep ! "^ firstline" output
-'
-
-test_expect_success 'The -U option overrides diff.context for "commit"' '
- test_config diff.context 8 &&
- ! git commit -U4 -p >output &&
- test_grep ! "^ firstline" output
-'
-
-test_expect_success 'The -U option overrides diff.context for "checkout"' '
- test_config diff.context 8 &&
- git checkout -U4 -p >output &&
- test_grep ! "^ firstline" output
-'
-
-test_expect_success 'The -U option overrides diff.context for "stash"' '
- test_config diff.context 8 &&
- ! git stash -U4 -p >output &&
- test_grep ! "^ firstline" output
-'
-
-test_expect_success 'The -U option overrides diff.context for "restore"' '
- test_config diff.context 8 &&
- git restore -U4 -p >output &&
- test_grep ! "^ firstline" output
-'
-
test_expect_success 'diff.context honored by "diff"' '
test_config diff.context 8 &&
git diff >output &&
--
2.49.0.897.gfad3eb7d210
|
||
| `--unified=<n>`:: | ||
| Generate diffs with _<n>_ lines of context. Defaults to `diff.context` | ||
| or 3 if the config option is unset. | ||
|
|
||
| `--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. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Leon Michalak wrote (reply to this):