Skip to content

add -i: close some regression test gaps #172

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

Closed
wants to merge 7 commits into from
Closed

add -i: close some regression test gaps #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
acfa161
t3701: add a test for advanced split-hunk editing
dscho Apr 3, 2019
ed870d3
t3701: avoid depending on the TTY prerequisite
dscho Mar 23, 2019
a8bd214
t3701: add a test for the different `add -p` prompts
dscho Mar 23, 2019
2219b37
t3701: verify the shown messages when nothing can be added
dscho Mar 24, 2019
3f219fd
t3701: verify that the diff.algorithm config setting is handled
dscho Mar 23, 2019
27c8cee
git add -p: use non-zero exit code when the diff generation failed
dscho Mar 23, 2019
04b22e5
apply --allow-overlap: fix a corner case
dscho Mar 20, 2019
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
10 changes: 10 additions & 0 deletions apply.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2661,6 +2661,16 @@ static int find_pos(struct apply_state *state,
unsigned long backwards, forwards, current;
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):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> From: Johannes Schindelin <[email protected]>
>
> Yes, yes, this is supposed to be only a band-aid option for `git add -p`
> not Doing The Right Thing. But as long as we carry the `--allow-overlap`
> option, we might just as well get it right.

It probably depends on the definition of "right", where it may not
even exist (otherwise it wouldn't be a band-aid but be a real
feature) ;-)

> This fixes the case where one hunk inserts a line before the first line,
> and is followed by a hunk whose context overlaps with the first one's
> and which appends a line at the end.

The in-code comment makes me wonder if we need to further loosen the
check in the other direction, though.  What makes begin more special
than end?  Can a hunk be seen that pretends to extend to the end but
no longer does because there was an overlapping hunk that has been
wiggled in?

>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  apply.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/apply.c b/apply.c
> index f8a046a6a5..720a631eaa 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -2661,6 +2661,16 @@ static int find_pos(struct apply_state *state,
>  	unsigned long backwards, forwards, current;
>  	int backwards_lno, forwards_lno, current_lno;
>  
> +	/*
> +	 * When running with --allow-overlap, it is possible that a hunk is
> +	 * seen that pretends to start at the beginning (but no longer does),
> +	 * and that *still* needs to match the end. So trust `match_end` more
> +	 * than `match_beginning`.
> +	 */
> +	if (state->allow_overlap && match_beginning && match_end &&
> +	    img->nr - preimage->nr != 0)
> +		match_beginning = 0;
> +
>  	/*
>  	 * If match_beginning or match_end is specified, there is no
>  	 * point starting from a wrong line that will never match and

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

Hi Junio,

On Fri, 6 Dec 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[email protected]>
> writes:
>
> > From: Johannes Schindelin <[email protected]>
> >
> > Yes, yes, this is supposed to be only a band-aid option for `git add -p`
> > not Doing The Right Thing. But as long as we carry the `--allow-overlap`
> > option, we might just as well get it right.
>
> It probably depends on the definition of "right", where it may not
> even exist (otherwise it wouldn't be a band-aid but be a real
> feature) ;-)

Indeed. My hope is that we can get rid of it once the scripted
`git-add--interactive.perl` is removed in favor of the built-in add -i/-p.
This is a long way off, of course.

> > This fixes the case where one hunk inserts a line before the first line,
> > and is followed by a hunk whose context overlaps with the first one's
> > and which appends a line at the end.
>
> The in-code comment makes me wonder if we need to further loosen the
> check in the other direction, though.  What makes begin more special
> than end?  Can a hunk be seen that pretends to extend to the end but
> no longer does because there was an overlapping hunk that has been
> wiggled in?

The beginning is more special than the end because it is associated with
the line number 1. The end line number is flexible already.

There is another difference: after splitting hunks, the first hunk is
applied first, and may render the line numbers of succeeding hunks
incorrect. The same is not true for the last hunk: it cannot render the
preceding hunks' line numbers incorrect, as it has not been applied yet.

I think that's why `--allow-overlap` works with this patch when adding
lines both to the beginning and to the end after splitting a single hunk.

Ciao,
Dscho

> > Signed-off-by: Johannes Schindelin <[email protected]>
> > ---
> >  apply.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/apply.c b/apply.c
> > index f8a046a6a5..720a631eaa 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -2661,6 +2661,16 @@ static int find_pos(struct apply_state *state,
> >  	unsigned long backwards, forwards, current;
> >  	int backwards_lno, forwards_lno, current_lno;
> >
> > +	/*
> > +	 * When running with --allow-overlap, it is possible that a hunk is
> > +	 * seen that pretends to start at the beginning (but no longer does),
> > +	 * and that *still* needs to match the end. So trust `match_end` more
> > +	 * than `match_beginning`.
> > +	 */
> > +	if (state->allow_overlap && match_beginning && match_end &&
> > +	    img->nr - preimage->nr != 0)
> > +		match_beginning = 0;
> > +
> >  	/*
> >  	 * If match_beginning or match_end is specified, there is no
> >  	 * point starting from a wrong line that will never match and
>

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

Johannes Schindelin <[email protected]> writes:

> The beginning is more special than the end because it is associated with
> the line number 1. The end line number is flexible already.

Yeah, we do not insist "the lineno must be X" at the end like we do
at the beginning, but we still insist "there cannot be no post
context if we are adding at the end" just like there cannot be any
pre context for a patch that adds at the beginning, no?

> There is another difference: after splitting hunks, the first hunk is
> applied first, and may render the line numbers of succeeding hunks
> incorrect. The same is not true for the last hunk: it cannot render the
> preceding hunks' line numbers incorrect, as it has not been applied yet.

This truly may make quite a difference, especially because the hunks
are applied in order.

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

Hi Junio,

On Fri, 6 Dec 2019, Junio C Hamano wrote:

> Johannes Schindelin <[email protected]> writes:
>
> > The beginning is more special than the end because it is associated with
> > the line number 1. The end line number is flexible already.
>
> Yeah, we do not insist "the lineno must be X" at the end like we do
> at the beginning, but we still insist "there cannot be no post
> context if we are adding at the end" just like there cannot be any
> pre context for a patch that adds at the beginning, no?

True.

> > There is another difference: after splitting hunks, the first hunk is
> > applied first, and may render the line numbers of succeeding hunks
> > incorrect. The same is not true for the last hunk: it cannot render the
> > preceding hunks' line numbers incorrect, as it has not been applied yet.
>
> This truly may make quite a difference, especially because the hunks
> are applied in order.

I think you're right, I fooled myself into believing that the line number
1 is special, but the real culprit is that the second hunk is applied
_after_ the first one may have changed the (overlapping) context. The same
is not true for the second-to-last hunk: it will always be applied before
the end of the file can possibly become no longer the end of the file.

I'll try to think up a concise paragraph about this, and stick it into the
commit message.

Ciao,
Dscho

int backwards_lno, forwards_lno, current_lno;

/*
* When running with --allow-overlap, it is possible that a hunk is
* seen that pretends to start at the beginning (but no longer does),
* and that *still* needs to match the end. So trust `match_end` more
* than `match_beginning`.
*/
if (state->allow_overlap && match_beginning && match_end &&
img->nr - preimage->nr != 0)
match_beginning = 0;

/*
* If match_beginning or match_end is specified, there is no
* point starting from a wrong line that will never match and
Expand Down
8 changes: 5 additions & 3 deletions git-add--interactive.perl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,9 @@ sub run_cmd_pipe {
} else {
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):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index f43634102e..5db6432e33 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -530,7 +530,7 @@ test_expect_success 'diff.algorithm is passed to `git diff-files`' '
>  	>file &&
>  	git add file &&
>  	echo changed >file &&
> -	git -c diff.algorithm=bogus add -p 2>err &&
> +	test_must_fail git -c diff.algorithm=bogus add -p 2>err &&

Makes sense.

>  	test_i18ngrep "error: option diff-algorithm accepts " err
>  '

my $fh = undef;
open($fh, '-|', @_) or die;
return <$fh>;
my @out = <$fh>;
close $fh || die "Cannot close @_ ($!)";
return @out;
}
}

Expand Down Expand Up @@ -224,7 +226,7 @@ sub list_untracked {
sub get_empty_tree {
return $empty_tree if defined $empty_tree;

$empty_tree = run_cmd_pipe(qw(git hash-object -t tree /dev/null));
($empty_tree) = run_cmd_pipe(qw(git hash-object -t tree /dev/null));
chomp $empty_tree;
return $empty_tree;
}
Expand Down Expand Up @@ -1127,7 +1129,7 @@ sub edit_hunk_manually {
EOF2
close $fh;

chomp(my $editor = run_cmd_pipe(qw(git var GIT_EDITOR)));
chomp(my ($editor) = run_cmd_pipe(qw(git var GIT_EDITOR)));
system('sh', '-c', $editor.' "$@"', $editor, $hunkfile);

if ($? != 0) {
Expand Down
90 changes: 82 additions & 8 deletions t/t3701-add-interactive.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,17 @@ diff_cmp () {
test_cmp "$1.filtered" "$2.filtered"
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, SZEDER Gábor wrote (reply to this):

On Fri, Dec 06, 2019 at 01:08:20PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> 
> The TTY prerequisite is a rather heavy one: it not only requires Perl to
> work, but also the IO/Pty.pm module (with native support, and it
> requires pseudo terminals, too).
> 
> In particular, test cases marked with the TTY prerequisite would be
> skipped in Git for Windows' SDK.
> 
> In the case of `git add -p`, we do not actually need that big a hammer,
> as we do not want to test any functionality that requires a pseudo
> terminal; all we want is for the interactive add command to use color,
> even when being called from within the test suite.
> 
> And we found exactly such a trick earlier already: when we added a test
> case to verify that the main loop of `git add -i` is colored
> appropriately. Let's use that trick instead of the TTY prerequisite.

It's much more interesting _what_ that trick is than when it was
found.  Is it setting TERM=vt100, or is it setting both TERM=vt100 and
GIT_PAGER_IN_USE=true?  I'm inclined to think the latter, but I'm not
sure I interpreted the comment below right.

> +# This function uses a trick to manipulate the interactive add to use color:
> +# the `want_color()` function special-cases the situation where a pager was
> +# spawned and Git now wants to output colored text: to detect that situation,
> +# the environment variable `GIT_PAGER_IN_USE` is set. However, color is

Perhaps a s/is set/has to be set/ would have helped my interpreter,
dunno.

> +# suppressed despite that environment variable if the `TERM` variable
> +# indicates a dumb terminal, so we set that variable, too.
> +
> +force_color () {
> +	env GIT_PAGER_IN_USE=true TERM=vt100 "$@"
> +}

In any case, there are a couple of tests in other test scripts that
test color relying on the TTY prereq.  So maybe it would be worth to
make this into a "global" helper function by adding it to
'test-lib-functions.sh', so we can drop a few more prereqs.

OTOH, some of those other tests have descriptions like:

  t3203-branch-output.sh:test_expect_success TTY '%(color) present with tty'
  t7004-tag.sh:test_expect_success TTY '%(color) present with tty'

i.e. their description is specific about checking the behaviour with a
tty, so I'm not entirely sure.

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

On Mon, Dec 16, 2019 at 01:18:59PM +0100, SZEDER Gábor wrote:

> > And we found exactly such a trick earlier already: when we added a test
> > case to verify that the main loop of `git add -i` is colored
> > appropriately. Let's use that trick instead of the TTY prerequisite.
> 
> It's much more interesting _what_ that trick is than when it was
> found.  Is it setting TERM=vt100, or is it setting both TERM=vt100 and
> GIT_PAGER_IN_USE=true?  I'm inclined to think the latter, but I'm not
> sure I interpreted the comment below right.

It's both. GIT_PAGER_IN_USE tells Git not to bother checking isatty(),
and then TERM=vt100 is necessary to override test-lib's TERM=dumb
specifically for the color code.

> In any case, there are a couple of tests in other test scripts that
> test color relying on the TTY prereq.  So maybe it would be worth to
> make this into a "global" helper function by adding it to
> 'test-lib-functions.sh', so we can drop a few more prereqs.
> 
> OTOH, some of those other tests have descriptions like:
> 
>   t3203-branch-output.sh:test_expect_success TTY '%(color) present with tty'
>   t7004-tag.sh:test_expect_success TTY '%(color) present with tty'
> 
> i.e. their description is specific about checking the behaviour with a
> tty, so I'm not entirely sure.

Hmm. I have mixed feelings on this. I do like the simplicity of avoiding
test_terminal (which is unportable and has also contributed to some
confusing behavior in the past[1]). But it also takes us further away from
a real-world setup.

That might be OK for the tests you quoted above, if we're OK with
assuming the equivalence of isatty() and GIT_PAGER_IN_USE for the color
code (though we'd probably want to make sure that's tested _somewhere_).

But it obviously would not work for test-terminal callers that are
checking the pager behavior. And I suspect it may create other oddities;
e.g., a script which calls a sub-command that looks at GIT_PAGER_IN_USE,
even though the sub-command's output is going to a pipe. Though one
could argue that's a bug[2] (that could be triggered by _actually_
sending the script's output to pager).

If we're going to get rid of test-terminal.pl (and I would be happy
enough to see it go), I'd rather we mock things up at the isatty()
level, something like what I showed in:

  https://lore.kernel.org/git/[email protected]/

That gives us a more realistic setup, and we could reliably use it
everywhere that test-terminal is used.

-Peff

[1] I had issues a while back with test-terminal's stdin feature being
    racy:

      https://lore.kernel.org/git/[email protected]/

[2] Long ago I had a patch to make PAGER_IN_USE more careful by making
    sure that our pipe is the same as the pager pipe. It did (and does)
    work, but it would need some portability adjustments. I never
    bothered to follow up because it really does seem to be a pretty
    unlikely setup in practice. But if you're curious:

      https://lore.kernel.org/git/[email protected]/

-Peff

}

# This function uses a trick to manipulate the interactive add to use color:
# the `want_color()` function special-cases the situation where a pager was
# spawned and Git now wants to output colored text: to detect that situation,
# the environment variable `GIT_PAGER_IN_USE` is set. However, color is
# suppressed despite that environment variable if the `TERM` variable
# indicates a dumb terminal, so we set that variable, too.

force_color () {
env GIT_PAGER_IN_USE=true TERM=vt100 "$@"
}

test_expect_success 'setup (initial)' '
echo content >file &&
git add file &&
Expand Down Expand Up @@ -94,7 +105,6 @@ test_expect_success 'revert works (commit)' '
grep "unchanged *+3/-0 file" output
'


test_expect_success 'setup expected' '
cat >expected <<-\EOF
EOF
Expand Down Expand Up @@ -263,6 +273,35 @@ test_expect_success FILEMODE 'stage mode and hunk' '

# end of tests disabled when filemode is not usable

test_expect_success 'different prompts for mode change/deleted' '
git reset --hard &&
>file &&
>deleted &&
git add --chmod=+x file deleted &&
echo changed >file &&
rm deleted &&
test_write_lines n n n |
git -c core.filemode=true add -p >actual &&
sed -n "s/^\(([0-9/]*) Stage .*?\).*/\1/p" actual >actual.filtered &&
cat >expect <<-\EOF &&
(1/1) Stage deletion [y,n,q,a,d,?]?
(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,?]?
(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]?
EOF
test_cmp expect actual.filtered
'

test_expect_success 'correct message when there is nothing to do' '
git reset --hard &&
git add -p 2>err &&
test_i18ngrep "No changes" err &&
printf "\\0123" >binary &&
git add binary &&
printf "\\0abc" >binary &&
git add -p 2>err &&
test_i18ngrep "Only binary files changed" err
'

test_expect_success 'setup again' '
git reset --hard &&
test_chmod +x file &&
Expand Down Expand Up @@ -403,6 +442,28 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
! grep "^+31" actual
'

test_expect_failure 'edit, adding lines to the first hunk' '
test_write_lines 10 11 20 30 40 50 51 60 >test &&
git reset &&
tr _ " " >patch <<-EOF &&
@@ -1,5 +1,6 @@
_10
+11
+12
_20
+21
+22
_30
EOF
# test sequence is s(plit), e(dit), n(o)
# q n q q is there to make sure we exit at the end.
printf "%s\n" s e n q n q q |
EDITOR=./fake_editor.sh git add -p 2>error &&
test_must_be_empty error &&
git diff --cached >actual &&
grep "^+22" actual
'

test_expect_success 'patch mode ignores unmerged entries' '
git reset --hard &&
test_commit conflict &&
Expand All @@ -429,35 +490,48 @@ test_expect_success 'patch mode ignores unmerged entries' '
diff_cmp expected diff
'

test_expect_success TTY 'diffs can be colorized' '
test_expect_success 'diffs can be colorized' '
git reset --hard &&

echo content >test &&
printf y | test_terminal git add -p >output 2>&1 &&
printf y >y &&
force_color git add -p >output 2>&1 <y &&

# 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_expect_success TTY 'diffFilter filters diff' '
test_expect_success 'diffFilter filters diff' '
git reset --hard &&

echo content >test &&
test_config interactive.diffFilter "sed s/^/foo:/" &&
printf y | test_terminal git add -p >output 2>&1 &&
printf y >y &&
force_color git add -p >output 2>&1 <y &&

# 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_expect_success TTY 'detect bogus diffFilter output' '
test_expect_success 'detect bogus diffFilter output' '
git reset --hard &&

echo content >test &&
test_config interactive.diffFilter "echo too-short" &&
printf y | test_must_fail test_terminal git add -p
printf y >y &&
test_must_fail force_color git add -p <y
'

test_expect_success 'diff.algorithm is passed to `git diff-files`' '
git reset --hard &&

>file &&
git add file &&
echo changed >file &&
test_must_fail git -c diff.algorithm=bogus add -p 2>err &&
test_i18ngrep "error: option diff-algorithm accepts " err
'

test_expect_success 'patch-mode via -i prompts for files' '
Expand Down Expand Up @@ -667,7 +741,7 @@ test_expect_success 'show help from add--helper' '
<BOLD;BLUE>What now<RESET>>$SP
Bye.
EOF
test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored &&
test_write_lines h | force_color git add -i >actual.colored &&
test_decode_color <actual.colored >actual &&
test_i18ncmp expect actual
'
Expand Down