Skip to content

Better support for customising context lines in --patch commands #1915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Better support for customising context lines in --patch commands #1915

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4f92a1b
test: refactor to use "test_grep"
NinjaInShade May 10, 2025
75424cb
test: refactor to use "test_config"
NinjaInShade May 10, 2025
f16d3de
add-patch: respect diff.context configuration
NinjaInShade May 4, 2025
973dfad
add-patch: add diff.context command line overrides
NinjaInShade May 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions add-interactive.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

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, Phillip Wood wrote (reply to this):

Hi Leon

On 10/05/2025 14:46, Leon Michalak via GitGitGadget wrote:
> From: Leon Michalak <[email protected]>
> > Various builtins that use add-patch infrastructure do not respect
> the user's diff.context and diff.interHunkContext file configurations.

We could expand this slightly by adding

This is because the plumbing commands used by "git add -p" to generate
the diff do not read those config settings. Fix this by reading the
config before generating the patch and passing it along to the diff
command with the "-U" and "--inter-hunk-context" command-line options.

> This patch fixes this inconsistency.
> > Signed-off-by: Leon Michalak <[email protected]>
> ---

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

Thanks for changing this. This iteration of the code changes looks good

> diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
> index 1384a8195705..c4b861c360cc 100755
> --- a/t/t4055-diff-context.sh
> +++ b/t/t4055-diff-context.sh
> @@ -52,6 +52,46 @@ test_expect_success 'diff.context honored by "log"' '
>   	test_grep "^ firstline" output
>   '

It's great that you have written tests for this patch but as I said
last time I think the new tests should be in t3701-add-interactive.sh
as we're interested in testing whether "git add -p" passes on
diff.context to "git diff" , not whether "git diff" respects
diff.context. I still think there are too many tests here as we know
that all the different "-p" commands share a single code path. Our
test suite is slow enough already so we do not want to add new tests
that do not increase our code coverage. I would suggest removing
these tests and instead add the following in t3701

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 'negative integer config parsing by "add"' '

Perhaps "add -p rejects negative diff.context" would be clearer?

> +	test_config diff.context -1 &&
> +	test_must_fail git add -p 2>output &&
> +	test_grep "diff.context cannot be negative" output
> +'

This is great but again we only need to test a single command and we
should do so in t3701. We should also check that negative values of
diff.interHunkContext are also rejected.

Best Wishes

Phillip

Copy link

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):

Phillip Wood <[email protected]> writes:

> Hi Leon
>
> On 10/05/2025 14:46, Leon Michalak via GitGitGadget wrote:
>> From: Leon Michalak <[email protected]>
>> Various builtins that use add-patch infrastructure do not respect
>> the user's diff.context and diff.interHunkContext file configurations.
>
> We could expand this slightly by adding
>
> This is because the plumbing commands used by "git add -p" to generate
> the diff do not read those config settings. Fix this by reading the
> config before generating the patch and passing it along to the diff
> command with the "-U" and "--inter-hunk-context" command-line options.
>
>> This patch fixes this inconsistency.
>> Signed-off-by: Leon Michalak <[email protected]>
>> ---
>
>> @@ -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;
>> +	};
>> +	if (!repo_config_get_int(r, "diff.interHunkContext", &interhunkcontext)) {
>> +		if (interhunkcontext < 0)
>> +			die(_("%s cannot be negative"), "diff.interHunkContext");
>> +		else
>> +			s->interhunkcontext = interhunkcontext;
>> +	};
>
> Thanks for changing this. This iteration of the code changes looks good

Lose the ';' (semicolon) after closing {brace}s.
This is C; you do not need an empty statement after a {block}.

Everything in your review I am very happy to see.  Thanks for giving
a great review.

Copy link

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, Phillip Wood wrote (reply to this):

On 13/05/2025 16:47, Junio C Hamano wrote:
> Phillip Wood <[email protected]> writes:
>> On 10/05/2025 14:46, Leon Michalak via GitGitGadget wrote:
>>
>>> @@ -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;
>>> +	};
>>> +	if (!repo_config_get_int(r, "diff.interHunkContext", &interhunkcontext)) {
>>> +		if (interhunkcontext < 0)
>>> +			die(_("%s cannot be negative"), "diff.interHunkContext");
>>> +		else
>>> +			s->interhunkcontext = interhunkcontext;
>>> +	};
>>
>> Thanks for changing this. This iteration of the code changes looks good
> > Lose the ';' (semicolon) after closing {brace}s.
> This is C; you do not need an empty statement after a {block}.

Oh well spotted, I'd missed that

Thanks

Phillip

Copy link

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):

Phillip Wood <[email protected]> writes:

> On 13/05/2025 16:47, Junio C Hamano wrote:
>> Phillip Wood <[email protected]> writes:
>>> On 10/05/2025 14:46, Leon Michalak via GitGitGadget wrote:
>>>
>>>> @@ -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;
>>>> +	};
>>>> +	if (!repo_config_get_int(r, "diff.interHunkContext", &interhunkcontext)) {
>>>> +		if (interhunkcontext < 0)
>>>> +			die(_("%s cannot be negative"), "diff.interHunkContext");
>>>> +		else
>>>> +			s->interhunkcontext = interhunkcontext;
>>>> +	};
>>>
>>> Thanks for changing this. This iteration of the code changes looks good
>> Lose the ';' (semicolon) after closing {brace}s.
>> This is C; you do not need an empty statement after a {block}.
>
> Oh well spotted, I'd missed that

Heh, with enough number of eyeballs, all the bugs are shallow.

{
const char *value;
int context;
int interhunkcontext;

s->r = r;
s->context = -1;
s->interhunkcontext = -1;

if (repo_config_get_value(r, "color.interactive", &value))
s->use_color = -1;
Expand Down Expand Up @@ -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;
};
if (!repo_config_get_int(r, "diff.interHunkContext", &interhunkcontext)) {
if (interhunkcontext < 0)
die(_("%s cannot be negative"), "diff.interHunkContext");
else
s->interhunkcontext = interhunkcontext;
};

repo_config_get_bool(r, "interactive.singlekey", &s->use_single_key);
if (s->use_single_key)
setbuf(stdin, NULL);
Expand Down
1 change: 1 addition & 0 deletions add-interactive.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 6 additions & 0 deletions add-patch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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);
if (diff_algorithm)
strvec_pushf(&args, "--diff-algorithm=%s", diff_algorithm);
if (s->revision) {
Expand Down
48 changes: 24 additions & 24 deletions t/t3701-add-interactive.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ test_expect_success 'setup (initial)' '
'
Copy link

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):

"Leon Michalak via GitGitGadget" <[email protected]> writes:

> From: Leon Michalak <[email protected]>
>
> Refactor to use the modern "test_grep" test utility instead of regular
> "grep" which provides better debug information if tests fail.
>
> This is a prerequisite to the commits that follow which add to both test
> files.
>
> Signed-off-by: Leon Michalak <[email protected]>
> ---

These mostly look sensible, but I would title & phrase the commit
description to 'use "test_grep"', not 'refactor to &'.  It's shorter
and more direct ;-)

Thanks.

Copy link

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):

On Mon, 12 May 2025 at 14:42, Junio C Hamano <[email protected]> wrote:
> These mostly look sensible, but I would title & phrase the commit
> description to 'use "test_grep"', not 'refactor to &'.  It's shorter
> and more direct ;-)

Thanks - will make sure to change that in v3 :)

test_expect_success 'status works (initial)' '
git add -i </dev/null >output &&
grep "+1/-0 *+2/-0 file" output
test_grep "+1/-0 *+2/-0 file" output
'

test_expect_success 'setup expected' '
Expand All @@ -86,7 +86,7 @@ test_expect_success 'revert works (initial)' '
git add file &&
test_write_lines r 1 | git add -i &&
git ls-files >output &&
! grep . output
test_grep ! . output
'

test_expect_success 'add untracked (multiple)' '
Expand All @@ -109,7 +109,7 @@ test_expect_success 'setup (commit)' '
'
test_expect_success 'status works (commit)' '
git add -i </dev/null >output &&
grep "+1/-0 *+2/-0 file" output
test_grep "+1/-0 *+2/-0 file" output
'

test_expect_success 'update can stage deletions' '
Expand Down Expand Up @@ -141,7 +141,7 @@ test_expect_success 'revert works (commit)' '
git add file &&
test_write_lines r 1 | git add -i &&
git add -i </dev/null >output &&
grep "unchanged *+3/-0 file" output
test_grep "unchanged *+3/-0 file" output
'

test_expect_success 'reject multi-key input' '
Expand Down Expand Up @@ -185,7 +185,7 @@ test_expect_success 'setup fake editor' '
test_expect_success 'bad edit rejected' '
git reset &&
test_write_lines e n d | git add -p >output &&
grep "hunk does not apply" output
test_grep "hunk does not apply" output
'

test_expect_success 'setup patch' '
Expand All @@ -198,7 +198,7 @@ test_expect_success 'setup patch' '
test_expect_success 'garbage edit rejected' '
git reset &&
test_write_lines e n d | git add -p >output &&
grep "hunk does not apply" output
test_grep "hunk does not apply" output
'

test_expect_success 'setup patch' '
Expand Down Expand Up @@ -313,8 +313,8 @@ test_expect_success FILEMODE 'stage mode and hunk' '
chmod +x file &&
printf "y\\ny\\n" | git add -p &&
git diff --cached file >out &&
grep "new mode" out &&
grep "+content" out &&
test_grep "new mode" out &&
test_grep "+content" out &&
git diff file >out &&
test_must_be_empty out
'
Expand Down Expand Up @@ -636,7 +636,7 @@ test_expect_success 'split hunk "add -p (edit)"' '
printf "%s\n" s e q n q q |
EDITOR=: git add -p &&
git diff >actual &&
! grep "^+15" actual
test_grep ! "^+15" actual
'

test_expect_success 'split hunk "add -p (no, yes, edit)"' '
Expand All @@ -648,7 +648,7 @@ test_expect_success 'split hunk "add -p (no, yes, edit)"' '
EDITOR=: git add -p 2>error &&
test_must_be_empty error &&
git diff >actual &&
! grep "^+31" actual
test_grep ! "^+31" actual
'

test_expect_success 'split hunk with incomplete line at end' '
Expand Down Expand Up @@ -682,7 +682,7 @@ test_expect_success 'edit, adding lines to the first hunk' '
EDITOR=./fake_editor.sh git add -p 2>error &&
test_must_be_empty error &&
git diff --cached >actual &&
grep "^+22" actual
test_grep "^+22" actual
'

test_expect_success 'patch mode ignores unmerged entries' '
Expand All @@ -696,7 +696,7 @@ test_expect_success 'patch mode ignores unmerged entries' '
test_must_fail git merge side &&
echo changed >non-conflict.t &&
echo y | git add -p >output &&
! grep a/conflict.t output &&
test_grep ! a/conflict.t output &&
cat >expected <<-\EOF &&
* Unmerged path conflict.t
diff --git a/non-conflict.t b/non-conflict.t
Expand Down Expand Up @@ -728,7 +728,7 @@ test_expect_success 'diffs can be colorized' '

# We do not want to depend on the exact coloring scheme
# git uses for diffs, so just check that we saw some kind of color.
grep "$(printf "\\033")" output
test_grep "$(printf "\\033")" output
'

test_expect_success 'colors can be overridden' '
Expand All @@ -743,7 +743,7 @@ test_expect_success 'colors can be overridden' '
-c color.interactive.error=blue \
add -i 2>err.raw <input &&
test_decode_color <err.raw >err &&
grep "<BLUE>Huh (trigger)?<RESET>" err &&
test_grep "<BLUE>Huh (trigger)?<RESET>" err &&

test_write_lines help quit >input &&
force_color git \
Expand Down Expand Up @@ -863,7 +863,7 @@ test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
printf y >y &&
force_color git -c diff.wsErrorHighlight=all add -p >output.raw 2>&1 <y &&
test_decode_color <output.raw >output &&
grep "old<" output
test_grep "old<" output
'

test_expect_success 'diffFilter filters diff' '
Expand All @@ -876,7 +876,7 @@ test_expect_success 'diffFilter filters diff' '

# avoid depending on the exact coloring or content of the prompts,
# and just make sure we saw our diff prefixed
grep foo:.*content output
test_grep foo:.*content output
'

test_expect_success 'detect bogus diffFilter output' '
Expand All @@ -886,7 +886,7 @@ test_expect_success 'detect bogus diffFilter output' '
test_config interactive.diffFilter "sed 6d" &&
printf y >y &&
force_color test_must_fail git add -p <y >output 2>&1 &&
grep "mismatched output" output
test_grep "mismatched output" output
'

test_expect_success 'handle iffy colored hunk headers' '
Expand All @@ -896,7 +896,7 @@ test_expect_success 'handle iffy colored hunk headers' '
printf n >n &&
force_color git -c interactive.diffFilter="sed s/.*@@.*/XX/" \
add -p >output 2>&1 <n &&
grep "^XX$" output
test_grep "^XX$" output
'

test_expect_success 'handle very large filtered diff' '
Expand Down Expand Up @@ -1002,7 +1002,7 @@ test_expect_success 'add -p does not expand argument lists' '
# update it, but we want to be sure that our "." pathspec
# was not expanded into the argument list of any command.
# So look only for "not-changed".
! grep -E "^trace: (built-in|exec|run_command): .*not-changed" trace.out
test_grep ! -E "^trace: (built-in|exec|run_command): .*not-changed" trace.out
'

test_expect_success 'hunk-editing handles custom comment char' '
Expand Down Expand Up @@ -1072,21 +1072,21 @@ test_expect_success 'setup different kinds of dirty submodules' '

test_expect_success 'status ignores dirty submodules (except HEAD)' '
git -C for-submodules add -i </dev/null >output &&
grep dirty-head output &&
grep dirty-both-ways output &&
! grep dirty-otherwise output
test_grep dirty-head output &&
test_grep dirty-both-ways output &&
test_grep ! dirty-otherwise output
'

test_expect_success 'handle submodules' '
echo 123 >>for-submodules/dirty-otherwise/initial.t &&

force_color git -C for-submodules add -p dirty-otherwise >output 2>&1 &&
grep "No changes" output &&
test_grep "No changes" output &&

force_color git -C for-submodules add -p dirty-head >output 2>&1 <y &&
git -C for-submodules ls-files --stage dirty-head >actual &&
rev="$(git -C for-submodules/dirty-head rev-parse HEAD)" &&
grep "$rev" actual
test_grep "$rev" actual
'

test_expect_success 'set up pathological context' '
Expand Down
Loading