patch, 7 partsadd -i: close some regression test gaps

While re-implementing git add -i and git add -p in C, I tried to make sure that there is test coverage for all of the features I convert from Perl to C, to give me some confidence in the correctness from running the test suite both with GIT_TEST_ADD_I_USE_BUILTIN=true and with GIT_TEST_ADD_I_USE_BUILTIN=false.

However, I discovered that there are a couple of gaps. This patch series intends to close them.

The first patch might actually not be considered a gap by some: it basically removes the need for the TTY prerequisite in the git add -i tests to verify that the output is colored all right. This change is rather crucial for me, though: on Windows, where the conversion to a built-in shows the most obvious benefits, there are no pseudo terminals (yet), therefore git.exe cannot work with them (even if the MSYS2 Perl interpreter used by Git for Windows knows about some sort of pty emulation). And I really wanted to make sure that the colors work on Windows, as I personally get a lot out of those color cues.

The patch series ends by addressing two issues that are not exactly covering testing gaps:

 * While adding a test case, I noticed that git add -p exited with success 
   when it could not even generate a diff. This is so obviously wrong that I
   had to fix it right away (I noticed, actually, because my in-progress
   built-in git add -p failed, and the Perl version did not), and I used the
   same test case to verify that this is fixed once and for all.
   
   
 * While working on covering those test gaps, I noticed a problem in an
   early version of the built-in version of git add -p where the git apply
   --allow-overlap mode failed to work properly, for little reason, and I
   fixed it real quick.
   
   It would seem that the --allow-overlap function is not only purposefully
   under-documented, but also purposefully under-tested, probably to
   discourage its use. I do not quite understand the aversion to that
   option, but I did not feel like I should put up a battle here, so I did
   not accompany this fix with a new test script.
   
   In the end, the built-in version of git add -p does not use the 
   --allow-overlap function at all, anyway. Which should make everybody a
   lot happier.

Johannes Schindelin (7):
  t3701: add a test for advanced split-hunk editing
  t3701: avoid depending on the TTY prerequisite
  t3701: add a test for the different `add -p` prompts
  t3701: verify the shown messages when nothing can be added
  t3701: verify that the diff.algorithm config setting is handled
  git add -p: use non-zero exit code when the diff generation failed
  apply --allow-overlap: fix a corner case

 apply.c                    | 10 +++++
 git-add--interactive.perl  |  8 ++--
 t/t3701-add-interactive.sh | 90 ++++++++++++++++++++++++++++++++++----
 3 files changed, 97 insertions(+), 11 deletions(-)

base-commit: 2e697ced9d647d6998d70f010d582ba8019fe3af
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-172%2Fdscho%2Fadd-i-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-172/dscho/add-i-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/172

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In this developer's workflows, it often happens that a hunk needs to be edited in a way that adds lines, and sometimes even reduces the number of context lines.

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 <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

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 <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

On Fri, Dec 06, 2019 at 01:08:20PM +0000, Johannes Schindelin via GitGitGadget wrote:

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.

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

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.

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

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.

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/20190524062724.GC25694@sigill.intra.peff.net/

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/20190520125016.GA13474@sigill.intra.peff.net/

[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/20150810052353.GB15441@sigill.intra.peff.net/

-Peff

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 line, and is followed by a hunk whose context overlaps with the first one's and which appends a line at the end.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 apply.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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?

Hi Junio,

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

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.

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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?

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

Thanks.

Hi Junio,

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

True.

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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 git-add--interactive.perl  | 8 +++++---
 t/t3701-add-interactive.sh | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Makes sense.