{"thread":{"id":"52393","subject":"[PATCH 0/7] add -i: close some regression test gaps","startedAt":"2019-12-06T13:08:33Z","lastAt":"2019-12-17T05:53:40Z","messageCount":15,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin","SZEDER Gábor","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"387603","messageId":"pull.172.git.1575637705.gitgitgadget@gmail.com","threadId":"52393","inReplyTo":null,"subject":"[PATCH 0/7] add -i: close some regression test gaps","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-06T13:08:18Z","receivedAt":"2019-12-06T13:08:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"While re-implementing git add -i and git add -p in C, I tried to make sure\nthat there is test coverage for all of the features I convert from Perl to\nC, to give me some confidence in the correctness from running the test suite\nboth with GIT_TEST_ADD_I_USE_BUILTIN=true and with \nGIT_TEST_ADD_I_USE_BUILTIN=false.\n\nHowever, I discovered that there are a couple of gaps. This patch series\nintends to close them.\n\nThe first patch might actually not be considered a gap by some: it basically\nremoves the need for the TTY prerequisite in the git add -i tests to verify\nthat the output is colored all right. This change is rather crucial for me,\nthough: on Windows, where the conversion to a built-in shows the most\nobvious benefits, there are no pseudo terminals (yet), therefore git.exe \ncannot work with them (even if the MSYS2 Perl interpreter used by Git for\nWindows knows about some sort of pty emulation). And I really wanted to make\nsure that the colors work on Windows, as I personally get a lot out of those\ncolor cues.\n\nThe patch series ends by addressing two issues that are not exactly covering\ntesting gaps:\n\n * While adding a test case, I noticed that git add -p exited with success \n when it could not even generate a diff. This is so obviously wrong that I\n had to fix it right away (I noticed, actually, because my in-progress\n built-in git add -p failed, and the Perl version did not), and I used the\n same test case to verify that this is fixed once and for all.\n \n \n * While working on covering those test gaps, I noticed a problem in an\n early version of the built-in version of git add -p where the git apply\n --allow-overlap mode failed to work properly, for little reason, and I\n fixed it real quick.\n \n It would seem that the --allow-overlap function is not only purposefully\n under-documented, but also purposefully under-tested, probably to\n discourage its use. I do not quite understand the aversion to that\n option, but I did not feel like I should put up a battle here, so I did\n not accompany this fix with a new test script.\n \n In the end, the built-in version of git add -p does not use the \n --allow-overlap function at all, anyway. Which should make everybody a\n lot happier.\n\nJohannes Schindelin (7):\n t3701: add a test for advanced split-hunk editing\n t3701: avoid depending on the TTY prerequisite\n t3701: add a test for the different `add -p` prompts\n t3701: verify the shown messages when nothing can be added\n t3701: verify that the diff.algorithm config setting is handled\n git add -p: use non-zero exit code when the diff generation failed\n apply --allow-overlap: fix a corner case\n\n apply.c | 10 +++++\n git-add--interactive.perl | 8 ++--\n t/t3701-add-interactive.sh | 90 ++++++++++++++++++++++++++++++++++----\n 3 files changed, 97 insertions(+), 11 deletions(-)\n\n\nbase-commit: 2e697ced9d647d6998d70f010d582ba8019fe3af\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-172%2Fdscho%2Fadd-i-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-172/dscho/add-i-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/172\n-- \ngitgitgadget\n"},{"id":"387606","messageId":"acfa1614afe1dfdf4bd63057824b3f033691c41d.1575637705.git.gitgitgadget@gmail.com","threadId":"52393","inReplyTo":"pull.172.git.1575637705.gitgitgadget@gmail.com","subject":"[PATCH 1/7] t3701: add a test for advanced split-hunk editing","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-06T13:08:19Z","receivedAt":"2019-12-06T13:08:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn this developer's workflows, it often happens that a hunk needs to be\nedited in a way that adds lines, and sometimes even reduces the number\nof context lines.\n\nLet's add a regression test for this.\n\nNote that just like the preceding test case, the new test case is *not*\nhandled gracefully by the current `git add -p`. It will be handled\ncorrectly by the upcoming built-in `git add -p`, though.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex d4f9386621..4da99e27af 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -403,6 +403,28 @@ test_expect_failure 'split hunk \"add -p (no, yes, edit)\"' '\n \t! grep \"^+31\" actual\n '\n \n+test_expect_failure 'edit, adding lines to the first hunk' '\n+\ttest_write_lines 10 11 20 30 40 50 51 60 >test &&\n+\tgit reset &&\n+\ttr _ \" \" >patch <<-EOF &&\n+\t@@ -1,5 +1,6 @@\n+\t_10\n+\t+11\n+\t+12\n+\t_20\n+\t+21\n+\t+22\n+\t_30\n+\tEOF\n+\t# test sequence is s(plit), e(dit), n(o)\n+\t# q n q q is there to make sure we exit at the end.\n+\tprintf \"%s\\n\" s e n q n q q |\n+\tEDITOR=./fake_editor.sh git add -p 2>error &&\n+\ttest_must_be_empty error &&\n+\tgit diff --cached >actual &&\n+\tgrep \"^+22\" actual\n+'\n+\n test_expect_success 'patch mode ignores unmerged entries' '\n \tgit reset --hard &&\n \ttest_commit conflict &&\n-- \ngitgitgadget\n\n"},{"id":"387610","messageId":"ed870d34a8479366df786e76e2770df344469a41.1575637705.git.gitgitgadget@gmail.com","threadId":"52393","inReplyTo":"pull.172.git.1575637705.gitgitgadget@gmail.com","subject":"[PATCH 2/7] t3701: avoid depending on the TTY prerequisite","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-06T13:08:20Z","receivedAt":"2019-12-06T13:08:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe TTY prerequisite is a rather heavy one: it not only requires Perl to\nwork, but also the IO/Pty.pm module (with native support, and it\nrequires pseudo terminals, too).\n\nIn particular, test cases marked with the TTY prerequisite would be\nskipped in Git for Windows' SDK.\n\nIn the case of `git add -p`, we do not actually need that big a hammer,\nas we do not want to test any functionality that requires a pseudo\nterminal; all we want is for the interactive add command to use color,\neven when being called from within the test suite.\n\nAnd we found exactly such a trick earlier already: when we added a test\ncase to verify that the main loop of `git add -i` is colored\nappropriately. Let's use that trick instead of the TTY prerequisite.\n\nWhile at it, we avoid the pipes, as we do not want a SIGPIPE to break\nthe regression test cases (which will be much more likely when we do not\nrun everything through Perl because that is inherently slower).\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 28 +++++++++++++++++++++-------\n 1 file changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 4da99e27af..793ce28297 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -23,6 +23,17 @@ diff_cmp () {\n \ttest_cmp \"$1.filtered\" \"$2.filtered\"\n }\n \n+# This function uses a trick to manipulate the interactive add to use color:\n+# the `want_color()` function special-cases the situation where a pager was\n+# spawned and Git now wants to output colored text: to detect that situation,\n+# the environment variable `GIT_PAGER_IN_USE` is set. However, color is\n+# suppressed despite that environment variable if the `TERM` variable\n+# indicates a dumb terminal, so we set that variable, too.\n+\n+force_color () {\n+\tenv GIT_PAGER_IN_USE=true TERM=vt100 \"$@\"\n+}\n+\n test_expect_success 'setup (initial)' '\n \techo content >file &&\n \tgit add file &&\n@@ -451,35 +462,38 @@ test_expect_success 'patch mode ignores unmerged entries' '\n \tdiff_cmp expected diff\n '\n \n-test_expect_success TTY 'diffs can be colorized' '\n+test_expect_success 'diffs can be colorized' '\n \tgit reset --hard &&\n \n \techo content >test &&\n-\tprintf y | test_terminal git add -p >output 2>&1 &&\n+\tprintf y >y &&\n+\tforce_color git add -p >output 2>&1 test &&\n \ttest_config interactive.diffFilter \"sed s/^/foo:/\" &&\n-\tprintf y | test_terminal git add -p >output 2>&1 &&\n+\tprintf y >y &&\n+\tforce_color git add -p >output 2>&1 test &&\n \ttest_config interactive.diffFilter \"echo too-short\" &&\n-\tprintf y | test_must_fail test_terminal git add -p\n+\tprintf y >y &&\n+\ttest_must_fail force_color git add -p What now>$SP\n \tBye.\n \tEOF\n-\ttest_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored &&\n+\ttest_write_lines h | force_color git add -i >actual.colored &&\n \ttest_decode_color actual &&\n \ttest_i18ncmp expect actual\n '\n-- \ngitgitgadget\n\n"},{"id":"387604","messageId":"a8bd214c905ff76d3651eec05766ad5b774847e8.1575637705.git.gitgitgadget@gmail.com","threadId":"52393","inReplyTo":"pull.172.git.1575637705.gitgitgadget@gmail.com","subject":"[PATCH 3/7] t3701: add a test for the different `add -p` prompts","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-06T13:08:21Z","receivedAt":"2019-12-06T13:08:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `git add -p` command offers different prompts for regular diff hunks\nvs mode change pseudo hunks vs diffs deleting files.\n\nLet's cover this in the regresion test suite, in preparation for\nre-implementing `git add -p` in C.\n\nFor the mode change prompt, we use a trick that lets this test case pass\neven on systems without executable bit, i.e. where `core.filemode =\nfalse` (such as Windows): we first add the file to the index with `git\nadd --chmod=+x`, and then call `git add -p` with `core.filemode` forced\nto `true`. The file on disk has no executable bit set, therefore we will\nsee a mode change.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 19 ++++++++++++++++++-\n 1 file changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 793ce28297..c90aaa25b0 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -105,7 +105,6 @@ test_expect_success 'revert works (commit)' '\n \tgrep \"unchanged *+3/-0 file\" output\n '\n \n-\n test_expect_success 'setup expected' '\n \tcat >expected <<-\\EOF\n \tEOF\n@@ -274,6 +273,24 @@ test_expect_success FILEMODE 'stage mode and hunk' '\n \n # end of tests disabled when filemode is not usable\n \n+test_expect_success 'different prompts for mode change/deleted' '\n+\tgit reset --hard &&\n+\t>file &&\n+\t>deleted &&\n+\tgit add --chmod=+x file deleted &&\n+\techo changed >file &&\n+\trm deleted &&\n+\ttest_write_lines n n n |\n+\tgit -c core.filemode=true add -p >actual &&\n+\tsed -n \"s/^\\(([0-9/]*) Stage .*?\\).*/\\1/p\" actual >actual.filtered &&\n+\tcat >expect <<-\\EOF &&\n+\t(1/1) Stage deletion [y,n,q,a,d,?]?\n+\t(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,?]?\n+\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]?\n+\tEOF\n+\ttest_cmp expect actual.filtered\n+'\n+\n test_expect_success 'setup again' '\n \tgit reset --hard &&\n \ttest_chmod +x file &&\n-- \ngitgitgadget\n\n"},{"id":"387605","messageId":"3f219fd789b3cf1c4d9c42b71a175d31df1afeeb.1575637705.git.gitgitgadget@gmail.com","threadId":"52393","inReplyTo":"pull.172.git.1575637705.gitgitgadget@gmail.com","subject":"[PATCH 5/7] t3701: verify that the diff.algorithm config setting is handled","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-06T13:08:23Z","receivedAt":"2019-12-06T13:08:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWithout this patch, there is actually no test in Git's test suite that\ncovers the diff.algorithm feature. Let's add one.\n\nWe do this by passing a bogus value and then expecting `git diff-files`\nto produce the appropriate error message.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 797610e96d..f43634102e 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -524,6 +524,16 @@ test_expect_success 'detect bogus diffFilter output' '\n \ttest_must_fail force_color git add -p file &&\n+\tgit add file &&\n+\techo changed >file &&\n+\tgit -c diff.algorithm=bogus add -p 2>err &&\n+\ttest_i18ngrep \"error: option diff-algorithm accepts \" err\n+'\n+\n test_expect_success 'patch-mode via -i prompts for files' '\n \tgit reset --hard &&\n \n-- \ngitgitgadget\n\n"},{"id":"387608","messageId":"2219b37fd58dea0d503777177ea79f68ea892dc7.1575637705.git.gitgitgadget@gmail.com","threadId":"52393","inReplyTo":"pull.172.git.1575637705.gitgitgadget@gmail.com","subject":"[PATCH 4/7] t3701: verify the shown messages when nothing can be added","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-06T13:08:22Z","receivedAt":"2019-12-06T13:08:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn preparation for re-implementing `git add -p` in pure C (where we will\npurposefully keep the implementation of `git add -p` separate from the\nimplementation of `git add -i`), let's verify that the user is told the\nsame things as in the Perl version when the diff file is either empty or\ncontains only entries about binary files.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex c90aaa25b0..797610e96d 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -291,6 +291,17 @@ test_expect_success 'different prompts for mode change/deleted' '\n \ttest_cmp expect actual.filtered\n '\n \n+test_expect_success 'correct message when there is nothing to do' '\n+\tgit reset --hard &&\n+\tgit add -p 2>err &&\n+\ttest_i18ngrep \"No changes\" err &&\n+\tprintf \"\\\\0123\" >binary &&\n+\tgit add binary &&\n+\tprintf \"\\\\0abc\" >binary &&\n+\tgit add -p 2>err &&\n+\ttest_i18ngrep \"Only binary files changed\" err\n+'\n+\n test_expect_success 'setup again' '\n \tgit reset --hard &&\n \ttest_chmod +x file &&\n-- \ngitgitgadget\n\n"},{"id":"387607","messageId":"15c4cce5a027d56c7ddbe5523cf0f3beabd06ed7.1575637705.git.gitgitgadget@gmail.com","threadId":"52393","inReplyTo":"pull.172.git.1575637705.gitgitgadget@gmail.com","subject":"[PATCH 7/7] apply --allow-overlap: fix a corner case","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-06T13:08:25Z","receivedAt":"2019-12-06T13:08:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nYes, yes, this is supposed to be only a band-aid option for `git add -p`\nnot Doing The Right Thing. But as long as we carry the `--allow-overlap`\noption, we might just as well get it right.\n\nThis fixes the case where one hunk inserts a line before the first line,\nand is followed by a hunk whose context overlaps with the first one's\nand which appends a line at the end.\n\nSigned-off-by: Johannes Schindelin \n---\n apply.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/apply.c b/apply.c\nindex f8a046a6a5..720a631eaa 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2661,6 +2661,16 @@ static int find_pos(struct apply_state *state,\n \tunsigned long backwards, forwards, current;\n \tint backwards_lno, forwards_lno, current_lno;\n \n+\t/*\n+\t * When running with --allow-overlap, it is possible that a hunk is\n+\t * seen that pretends to start at the beginning (but no longer does),\n+\t * and that *still* needs to match the end. So trust `match_end` more\n+\t * than `match_beginning`.\n+\t */\n+\tif (state->allow_overlap && match_beginning && match_end &&\n+\t img->nr - preimage->nr != 0)\n+\t\tmatch_beginning = 0;\n+\n \t/*\n \t * If match_beginning or match_end is specified, there is no\n \t * point starting from a wrong line that will never match and\n-- \ngitgitgadget\n"},{"id":"387609","messageId":"27c8ceeb1150dafeaf5b83831487f6bd66c04839.1575637705.git.gitgitgadget@gmail.com","threadId":"52393","inReplyTo":"pull.172.git.1575637705.gitgitgadget@gmail.com","subject":"[PATCH 6/7] git add -p: use non-zero exit code when the diff generation failed","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-06T13:08:24Z","receivedAt":"2019-12-06T13:08:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe first thing `git add -p` does is to generate a diff. If this diff\ncannot be generated, `git add -p` should not continue as if nothing\nhappened, but instead fail.\n\nWhat we *actually* do here is much broader: we now verify for *every*\n`run_cmd_pipe()` call that the spawned process actually succeeded.\n\nNote that we have to change two callers in this patch, as we need to\nstore the spawned process' output in a local variable, which means that\nthe callers can no longer decide whether to interpret the `return <$fh>`\nin array or in scalar context.\n\nThis bug was noticed while writing a test case for the diff.algorithm\nfeature, and we let that test case double as a regression test for this\nfixed bug, too.\n\nSigned-off-by: Johannes Schindelin \n---\n git-add--interactive.perl | 8 +++++---\n t/t3701-add-interactive.sh | 2 +-\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 52659bb74c..10fd30ae16 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -177,7 +177,9 @@ sub run_cmd_pipe {\n \t} else {\n \t\tmy $fh = undef;\n \t\topen($fh, '-|', @_) or die;\n-\t\treturn <$fh>;\n+\t\tmy @out = <$fh>;\n+\t\tclose $fh || die \"Cannot close @_ ($!)\";\n+\t\treturn @out;\n \t}\n }\n \n@@ -224,7 +226,7 @@ sub list_untracked {\n \tsub get_empty_tree {\n \t\treturn $empty_tree if defined $empty_tree;\n \n-\t\t$empty_tree = run_cmd_pipe(qw(git hash-object -t tree /dev/null));\n+\t\t($empty_tree) = run_cmd_pipe(qw(git hash-object -t tree /dev/null));\n \t\tchomp $empty_tree;\n \t\treturn $empty_tree;\n \t}\n@@ -1127,7 +1129,7 @@ sub edit_hunk_manually {\n EOF2\n \tclose $fh;\n \n-\tchomp(my $editor = run_cmd_pipe(qw(git var GIT_EDITOR)));\n+\tchomp(my ($editor) = run_cmd_pipe(qw(git var GIT_EDITOR)));\n \tsystem('sh', '-c', $editor.' \"$@\"', $editor, $hunkfile);\n \n \tif ($? != 0) {\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex f43634102e..5db6432e33 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -530,7 +530,7 @@ test_expect_success 'diff.algorithm is passed to `git diff-files`' '\n \t>file &&\n \tgit add file &&\n \techo changed >file &&\n-\tgit -c diff.algorithm=bogus add -p 2>err &&\n+\ttest_must_fail git -c diff.algorithm=bogus add -p 2>err &&\n \ttest_i18ngrep \"error: option diff-algorithm accepts \" err\n '\n \n-- \ngitgitgadget\n\n"},{"id":"387612","messageId":"xmqqr21hy3cf.fsf@gitster-ct.c.googlers.com","threadId":"52393","inReplyTo":"15c4cce5a027d56c7ddbe5523cf0f3beabd06ed7.1575637705.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 7/7] apply --allow-overlap: fix a corner case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-06T13:45:52Z","receivedAt":"2019-12-06T13:45:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> Yes, yes, this is supposed to be only a band-aid option for `git add -p`\n> not Doing The Right Thing. But as long as we carry the `--allow-overlap`\n> option, we might just as well get it right.\n\nIt probably depends on the definition of \"right\", where it may not\neven exist (otherwise it wouldn't be a band-aid but be a real\nfeature) ;-)\n\n> This fixes the case where one hunk inserts a line before the first line,\n> and is followed by a hunk whose context overlaps with the first one's\n> and which appends a line at the end.\n\nThe in-code comment makes me wonder if we need to further loosen the\ncheck in the other direction, though. What makes begin more special\nthan end? Can a hunk be seen that pretends to extend to the end but\nno longer does because there was an overlapping hunk that has been\nwiggled in?\n\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> apply.c | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/apply.c b/apply.c\n> index f8a046a6a5..720a631eaa 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -2661,6 +2661,16 @@ static int find_pos(struct apply_state *state,\n> \tunsigned long backwards, forwards, current;\n> \tint backwards_lno, forwards_lno, current_lno;\n> \n> +\t/*\n> +\t * When running with --allow-overlap, it is possible that a hunk is\n> +\t * seen that pretends to start at the beginning (but no longer does),\n> +\t * and that *still* needs to match the end. So trust `match_end` more\n> +\t * than `match_beginning`.\n> +\t */\n> +\tif (state->allow_overlap && match_beginning && match_end &&\n> +\t img->nr - preimage->nr != 0)\n> +\t\tmatch_beginning = 0;\n> +\n> \t/*\n> \t * If match_beginning or match_end is specified, there is no\n> \t * point starting from a wrong line that will never match and\n"},{"id":"387614","messageId":"nycvar.QRO.7.76.6.1912061502110.31080@tvgsbejvaqbjf.bet","threadId":"52393","inReplyTo":"xmqqr21hy3cf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 7/7] apply --allow-overlap: fix a corner case","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-06T14:11:41Z","receivedAt":"2019-12-06T14:12:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 6 Dec 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > Yes, yes, this is supposed to be only a band-aid option for `git add -p`\n> > not Doing The Right Thing. But as long as we carry the `--allow-overlap`\n> > option, we might just as well get it right.\n>\n> It probably depends on the definition of \"right\", where it may not\n> even exist (otherwise it wouldn't be a band-aid but be a real\n> feature) ;-)\n\nIndeed. My hope is that we can get rid of it once the scripted\n`git-add--interactive.perl` is removed in favor of the built-in add -i/-p.\nThis is a long way off, of course.\n\n> > This fixes the case where one hunk inserts a line before the first line,\n> > and is followed by a hunk whose context overlaps with the first one's\n> > and which appends a line at the end.\n>\n> The in-code comment makes me wonder if we need to further loosen the\n> check in the other direction, though. What makes begin more special\n> than end? Can a hunk be seen that pretends to extend to the end but\n> no longer does because there was an overlapping hunk that has been\n> wiggled in?\n\nThe beginning is more special than the end because it is associated with\nthe line number 1. The end line number is flexible already.\n\nThere is another difference: after splitting hunks, the first hunk is\napplied first, and may render the line numbers of succeeding hunks\nincorrect. The same is not true for the last hunk: it cannot render the\npreceding hunks' line numbers incorrect, as it has not been applied yet.\n\nI think that's why `--allow-overlap` works with this patch when adding\nlines both to the beginning and to the end after splitting a single hunk.\n\nCiao,\nDscho\n\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > apply.c | 10 ++++++++++\n> > 1 file changed, 10 insertions(+)\n> >\n> > diff --git a/apply.c b/apply.c\n> > index f8a046a6a5..720a631eaa 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -2661,6 +2661,16 @@ static int find_pos(struct apply_state *state,\n> > \tunsigned long backwards, forwards, current;\n> > \tint backwards_lno, forwards_lno, current_lno;\n> >\n> > +\t/*\n> > +\t * When running with --allow-overlap, it is possible that a hunk is\n> > +\t * seen that pretends to start at the beginning (but no longer does),\n> > +\t * and that *still* needs to match the end. So trust `match_end` more\n> > +\t * than `match_beginning`.\n> > +\t */\n> > +\tif (state->allow_overlap && match_beginning && match_end &&\n> > +\t img->nr - preimage->nr != 0)\n> > +\t\tmatch_beginning = 0;\n> > +\n> > \t/*\n> > \t * If match_beginning or match_end is specified, there is no\n> > \t * point starting from a wrong line that will never match and\n>\n"},{"id":"387632","messageId":"xmqqa785xv9w.fsf@gitster-ct.c.googlers.com","threadId":"52393","inReplyTo":"nycvar.QRO.7.76.6.1912061502110.31080@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 7/7] apply --allow-overlap: fix a corner case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-06T16:40:11Z","receivedAt":"2019-12-06T16:40:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The beginning is more special than the end because it is associated with\n> the line number 1. The end line number is flexible already.\n\nYeah, we do not insist \"the lineno must be X\" at the end like we do\nat the beginning, but we still insist \"there cannot be no post\ncontext if we are adding at the end\" just like there cannot be any\npre context for a patch that adds at the beginning, no?\n\n> There is another difference: after splitting hunks, the first hunk is\n> applied first, and may render the line numbers of succeeding hunks\n> incorrect. The same is not true for the last hunk: it cannot render the\n> preceding hunks' line numbers incorrect, as it has not been applied yet.\n\nThis truly may make quite a difference, especially because the hunks\nare applied in order.\n\nThanks.\n"},{"id":"387634","messageId":"nycvar.QRO.7.76.6.1912061849300.31080@tvgsbejvaqbjf.bet","threadId":"52393","inReplyTo":"xmqqa785xv9w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 7/7] apply --allow-overlap: fix a corner case","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-06T17:56:51Z","receivedAt":"2019-12-06T17:57:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 6 Dec 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > The beginning is more special than the end because it is associated with\n> > the line number 1. The end line number is flexible already.\n>\n> Yeah, we do not insist \"the lineno must be X\" at the end like we do\n> at the beginning, but we still insist \"there cannot be no post\n> context if we are adding at the end\" just like there cannot be any\n> pre context for a patch that adds at the beginning, no?\n\nTrue.\n\n> > There is another difference: after splitting hunks, the first hunk is\n> > applied first, and may render the line numbers of succeeding hunks\n> > incorrect. The same is not true for the last hunk: it cannot render the\n> > preceding hunks' line numbers incorrect, as it has not been applied yet.\n>\n> This truly may make quite a difference, especially because the hunks\n> are applied in order.\n\nI think you're right, I fooled myself into believing that the line number\n1 is special, but the real culprit is that the second hunk is applied\n_after_ the first one may have changed the (overlapping) context. The same\nis not true for the second-to-last hunk: it will always be applied before\nthe end of the file can possibly become no longer the end of the file.\n\nI'll try to think up a concise paragraph about this, and stick it into the\ncommit message.\n\nCiao,\nDscho\n"},{"id":"387647","messageId":"xmqqh82dw57m.fsf@gitster-ct.c.googlers.com","threadId":"52393","inReplyTo":"27c8ceeb1150dafeaf5b83831487f6bd66c04839.1575637705.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/7] git add -p: use non-zero exit code when the diff generation failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-06T20:48:29Z","receivedAt":"2019-12-06T20:48:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index f43634102e..5db6432e33 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -530,7 +530,7 @@ test_expect_success 'diff.algorithm is passed to `git diff-files`' '\n> \t>file &&\n> \tgit add file &&\n> \techo changed >file &&\n> -\tgit -c diff.algorithm=bogus add -p 2>err &&\n> +\ttest_must_fail git -c diff.algorithm=bogus add -p 2>err &&\n\nMakes sense.\n\n> \ttest_i18ngrep \"error: option diff-algorithm accepts \" err\n> '\n"},{"id":"388248","messageId":"20191216121859.GP6527@szeder.dev","threadId":"52393","inReplyTo":"ed870d34a8479366df786e76e2770df344469a41.1575637705.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/7] t3701: avoid depending on the TTY prerequisite","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-12-16T12:18:59Z","receivedAt":"2019-12-16T12:19:07Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Dec 06, 2019 at 01:08:20PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> The TTY prerequisite is a rather heavy one: it not only requires Perl to\n> work, but also the IO/Pty.pm module (with native support, and it\n> requires pseudo terminals, too).\n> \n> In particular, test cases marked with the TTY prerequisite would be\n> skipped in Git for Windows' SDK.\n> \n> In the case of `git add -p`, we do not actually need that big a hammer,\n> as we do not want to test any functionality that requires a pseudo\n> terminal; all we want is for the interactive add command to use color,\n> even when being called from within the test suite.\n> \n> And we found exactly such a trick earlier already: when we added a test\n> case to verify that the main loop of `git add -i` is colored\n> appropriately. Let's use that trick instead of the TTY prerequisite.\n\nIt's much more interesting _what_ that trick is than when it was\nfound. Is it setting TERM=vt100, or is it setting both TERM=vt100 and\nGIT_PAGER_IN_USE=true? I'm inclined to think the latter, but I'm not\nsure I interpreted the comment below right.\n\n> +# This function uses a trick to manipulate the interactive add to use color:\n> +# the `want_color()` function special-cases the situation where a pager was\n> +# spawned and Git now wants to output colored text: to detect that situation,\n> +# the environment variable `GIT_PAGER_IN_USE` is set. However, color is\n\nPerhaps a s/is set/has to be set/ would have helped my interpreter,\ndunno.\n\n> +# suppressed despite that environment variable if the `TERM` variable\n> +# indicates a dumb terminal, so we set that variable, too.\n> +\n> +force_color () {\n> +\tenv GIT_PAGER_IN_USE=true TERM=vt100 \"$@\"\n> +}\n\nIn any case, there are a couple of tests in other test scripts that\ntest color relying on the TTY prereq. So maybe it would be worth to\nmake this into a \"global\" helper function by adding it to\n'test-lib-functions.sh', so we can drop a few more prereqs.\n\nOTOH, some of those other tests have descriptions like:\n\n t3203-branch-output.sh:test_expect_success TTY '%(color) present with tty'\n t7004-tag.sh:test_expect_success TTY '%(color) present with tty'\n\ni.e. their description is specific about checking the behaviour with a\ntty, so I'm not entirely sure.\n\n"},{"id":"388330","messageId":"20191217055338.GC2762303@coredump.intra.peff.net","threadId":"52393","inReplyTo":"20191216121859.GP6527@szeder.dev","subject":"Re: [PATCH 2/7] t3701: avoid depending on the TTY prerequisite","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-17T05:53:38Z","receivedAt":"2019-12-17T05:53:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 16, 2019 at 01:18:59PM +0100, SZEDER Gábor wrote:\n\n> > And we found exactly such a trick earlier already: when we added a test\n> > case to verify that the main loop of `git add -i` is colored\n> > appropriately. Let's use that trick instead of the TTY prerequisite.\n> \n> It's much more interesting _what_ that trick is than when it was\n> found. Is it setting TERM=vt100, or is it setting both TERM=vt100 and\n> GIT_PAGER_IN_USE=true? I'm inclined to think the latter, but I'm not\n> sure I interpreted the comment below right.\n\nIt's both. GIT_PAGER_IN_USE tells Git not to bother checking isatty(),\nand then TERM=vt100 is necessary to override test-lib's TERM=dumb\nspecifically for the color code.\n\n> In any case, there are a couple of tests in other test scripts that\n> test color relying on the TTY prereq. So maybe it would be worth to\n> make this into a \"global\" helper function by adding it to\n> 'test-lib-functions.sh', so we can drop a few more prereqs.\n> \n> OTOH, some of those other tests have descriptions like:\n> \n> t3203-branch-output.sh:test_expect_success TTY '%(color) present with tty'\n> t7004-tag.sh:test_expect_success TTY '%(color) present with tty'\n> \n> i.e. their description is specific about checking the behaviour with a\n> tty, so I'm not entirely sure.\n\nHmm. I have mixed feelings on this. I do like the simplicity of avoiding\ntest_terminal (which is unportable and has also contributed to some\nconfusing behavior in the past[1]). But it also takes us further away from\na real-world setup.\n\nThat might be OK for the tests you quoted above, if we're OK with\nassuming the equivalence of isatty() and GIT_PAGER_IN_USE for the color\ncode (though we'd probably want to make sure that's tested _somewhere_).\n\nBut it obviously would not work for test-terminal callers that are\nchecking the pager behavior. And I suspect it may create other oddities;\ne.g., a script which calls a sub-command that looks at GIT_PAGER_IN_USE,\neven though the sub-command's output is going to a pipe. Though one\ncould argue that's a bug[2] (that could be triggered by _actually_\nsending the script's output to pager).\n\nIf we're going to get rid of test-terminal.pl (and I would be happy\nenough to see it go), I'd rather we mock things up at the isatty()\nlevel, something like what I showed in:\n\n https://lore.kernel.org/git/20190524062724.GC25694@sigill.intra.peff.net/\n\nThat gives us a more realistic setup, and we could reliably use it\neverywhere that test-terminal is used.\n\n-Peff\n\n[1] I had issues a while back with test-terminal's stdin feature being\n racy:\n\n https://lore.kernel.org/git/20190520125016.GA13474@sigill.intra.peff.net/\n\n[2] Long ago I had a patch to make PAGER_IN_USE more careful by making\n sure that our pipe is the same as the pager pipe. It did (and does)\n work, but it would need some portability adjustments. I never\n bothered to follow up because it really does seem to be a pretty\n unlikely setup in practice. But if you're curious:\n\n https://lore.kernel.org/git/20150810052353.GB15441@sigill.intra.peff.net/\n\n-Peff\n"}]}