From 6c1ac7ef24566336843b45bd533365711e3bcc04 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 3 Apr 2019 22:15:39 +0200 Subject: [PATCH 1/7] t3701: add a test for advanced split-hunk editing In this developer's workflows, it often happens that a hunk needs to be edited in a way that adds lines, and even reduces the context Let's add a regression test for this. Note that just like the preceding test case, the new test case is *not* handled gracefully by the current `git add -p`. It will be handled correctly by the upcoming built-in `git add -p`, though. Signed-off-by: Johannes Schindelin --- t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index cf67756b85..856f4f33b2 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -403,6 +403,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 && From 3ceab76107c076b9bc5b0c4e869d9ac1349ec3bd Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 20:05:24 +0100 Subject: [PATCH 2/7] t3701: avoid depending on the TTY prerequisite 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 to talk 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. While at it, we avoid the pipes, as we do not want a SIGPIPE to break the regression test cases (which will be much more likely when we do not run everything through Perl because that is inherently slower). Signed-off-by: Johannes Schindelin --- t/t3701-add-interactive.sh | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 856f4f33b2..e79b16bc94 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -23,6 +23,17 @@ diff_cmp () { test_cmp "$1.filtered" "$2.filtered" } +# 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 && @@ -451,35 +462,38 @@ 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 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 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 What now>$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 && test_i18ncmp expect actual ' From f417cc7abd7b8ab1462faee11eac06142d3679bc Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 20:36:47 +0100 Subject: [PATCH 3/7] t3701: add a test for the different `add -p` prompts The `git add -p` command offers different prompts for regular diff hunks vs mode change pseudo hunks vs diffs deleting files. Let's cover this in the regresion test suite, in preparation for re-implementing `git add -p` in C. For the mode change prompt, we use a trick that lets this test case pass even on systems without executable bit, i.e. where `core.filemode = false` (such as Windows): we first add the file to the index with `git add --chmod=+x`, and then call `git add -p` with `core.filemode` forced to `true`. The file on disk has no executable bit set, therefore we will see a mode change. Signed-off-by: Johannes Schindelin --- t/t3701-add-interactive.sh | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index e79b16bc94..c469824c4c 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -105,7 +105,6 @@ test_expect_success 'revert works (commit)' ' grep "unchanged *+3/-0 file" output ' - test_expect_success 'setup expected' ' cat >expected <<-\EOF EOF @@ -274,6 +273,24 @@ 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/^\(Stage .*?\).*/\1/p" actual >actual.filtered && + cat >expect <<-\EOF && + Stage deletion [y,n,q,a,d,?]? + Stage mode change [y,n,q,a,d,j,J,g,/,?]? + Stage this hunk [y,n,q,a,d,K,g,/,e,?]? + EOF + test_cmp expect actual.filtered +' + test_expect_success 'setup again' ' git reset --hard && test_chmod +x file && From 37aec53d1f91827fe25ef03a7ae50743ead63302 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 24 Mar 2019 22:51:02 +0100 Subject: [PATCH 4/7] t3701: verify the shown messages when nothing can be added In preparation for re-implementing `git add -p` in pure C (where we will purposefully keep the implementation of `git add -p` separate from the implementation of `git add -i`), let's verify that the user is told the same things as in the Perl version when the diff file is either empty or contains only entries about binary files. Signed-off-by: Johannes Schindelin --- t/t3701-add-interactive.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index c469824c4c..08ab045d60 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -291,6 +291,17 @@ test_expect_success 'different prompts for mode change/deleted' ' 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 && From 0a372a71eb3a8148f01e7f0b97ac325fe7bbe572 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 19:36:22 +0100 Subject: [PATCH 5/7] t3701: verify that the diff.algorithm config setting is handled Without this patch, there is actually no test in Git's test suite that covers the diff.algorithm feature. Let's add one. We do this by passing a bogus value and then expecting `git diff-files` to produce the appropriate error message. Signed-off-by: Johannes Schindelin --- t/t3701-add-interactive.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 08ab045d60..53362882eb 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -524,6 +524,16 @@ test_expect_success 'detect bogus diffFilter output' ' test_must_fail force_color git add -p file && + git add file && + echo changed >file && + 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' ' git reset --hard && From 55e41f874889ca3d053a9e6327ba4e323912b365 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 17:37:45 +0100 Subject: [PATCH 6/7] git add -p: use non-zero exit code when the diff generation failed The first thing `git add -p` does is to generate a diff. If this diff cannot be generated, `git add -p` should not continue as if nothing happened, but instead fail. What we *actually* do here is much broader: we now verify for *every* `run_cmd_pipe()` call that the spawned process actually succeeded. Note that we have to change two callers in this patch, as we need to store the spawned process' output in a local variable, which means that the callers can no longer decide whether to interpret the `return <$fh>` in array or in scalar context. This bug was noticed while writing a test case for the diff.algorithm feature, and we let that test case double as a regression test for this fixed bug, too. Signed-off-by: Johannes Schindelin --- git-add--interactive.perl | 8 +++++--- t/t3701-add-interactive.sh | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index c20ae9e210..7269bfdae1 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -177,7 +177,9 @@ sub run_cmd_pipe { } else { my $fh = undef; open($fh, '-|', @_) or die; - return <$fh>; + my @out = <$fh>; + close $fh || die "Cannot close @_ ($!)"; + return @out; } } @@ -224,7 +226,7 @@ my $status_head = sprintf($status_fmt, __('staged'), __('unstaged'), __('path')) 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; } @@ -1127,7 +1129,7 @@ aborted and the hunk is left unchanged. 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) { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 53362882eb..ab218b71e5 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 && test_i18ngrep "error: option diff-algorithm accepts " err ' From 6198d09d4fe961f128979841a630f49d1392559c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 20 Mar 2019 12:10:11 +0100 Subject: [PATCH 7/7] apply --allow-overlap: fix a corner case 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. This fixes the case where one hunk inserts a line before the first one, and a hunk whose context overlaps with the first one's appends a line at the end. Signed-off-by: Johannes Schindelin --- apply.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/apply.c b/apply.c index cde95369bb..694bd3cf28 100644 --- a/apply.c +++ b/apply.c @@ -2660,6 +2660,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