{"thread":{"id":"53191","subject":"[PATCH 0/2] Fix a git stash -p corner-case","startedAt":"2020-04-08T18:52:38Z","lastAt":"2020-04-08T18:52:41Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"395082","messageId":"pull.602.git.1586371954.gitgitgadget@gmail.com","threadId":"53191","inReplyTo":null,"subject":"[PATCH 0/2] Fix a git stash -p corner-case","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-08T18:52:32Z","receivedAt":"2020-04-08T18:52:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This corner case came up a couple of times in my personal usage of git stash\n-p: I wanted to stash only part of a split hunk, and it didn't work.\n\nI originally intended to do a full fix (for details, see the commit message\nof the second patch), but in the year since I wrote this patch pair, I found\nthe current work-around plenty satisfactory.\n\nJohannes Schindelin (2):\n t3904: fix incorrect demonstration of a bug\n stash -p: (partially) fix bug concerning split hunks\n\n builtin/stash.c | 2 +-\n t/t3904-stash-patch.sh | 6 +++---\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\n\nbase-commit: 9fadedd637b312089337d73c3ed8447e9f0aa775\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-602%2Fdscho%2Fstash-p-corner-case-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-602/dscho/stash-p-corner-case-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/602\n-- \ngitgitgadget\n"},{"id":"395083","messageId":"461fc76ed71fe79b61f3623b5ab094796723569b.1586371954.git.gitgitgadget@gmail.com","threadId":"53191","inReplyTo":"pull.602.git.1586371954.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t3904: fix incorrect demonstration of a bug","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-08T18:52:33Z","receivedAt":"2020-04-08T18:52:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 7e9e048661 (stash -p: demonstrate failure of split with mixed y/n,\n2015-04-16), a regression test for a known breakage that was added to\nthe test script `t3904-stash-patch.sh` that demonstrated that splitting\na hunk and trying to stash only part of that split hunk fails (but\nshouldn't).\n\nAs expected, it still fails, but for the wrong reason: once the bug is\nfixed, we would expect stderr to show nothing, yet the regression test\nexpects stderr to show something.\n\nLet's fix that by telling that regression test case to expect nothing to\nbe printed to stderr.\n\nWhile at it, also drop the obvious left-over from debugging where the\nregression test did not mind `git stash -p` to return a non-zero exit\nstatus.\n\nOf course, the regression test still fails, but this time for the\ncorrect reason.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3904-stash-patch.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh\nindex 9546b6f8a4e..ab7d7aa6de1 100755\n--- a/t/t3904-stash-patch.sh\n+++ b/t/t3904-stash-patch.sh\n@@ -106,8 +106,8 @@ test_expect_failure 'stash -p with split hunk' '\n \tccc\n \tEOF\n \tprintf \"%s\\n\" s n y q |\n-\ttest_might_fail git stash -p 2>error &&\n-\t! test_must_be_empty error &&\n+\tgit stash -p 2>error &&\n+\ttest_must_be_empty error &&\n \tgrep \"added line 1\" test &&\n \t! grep \"added line 2\" test\n '\n-- \ngitgitgadget\n\n"},{"id":"395084","messageId":"91ad9a99629e8529a4fa82ce721a54e63b29bf92.1586371954.git.gitgitgadget@gmail.com","threadId":"53191","inReplyTo":"pull.602.git.1586371954.gitgitgadget@gmail.com","subject":"[PATCH 2/2] stash -p: (partially) fix bug concerning split hunks","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-08T18:52:34Z","receivedAt":"2020-04-08T18:52:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen trying to stash part of the worktree changes by splitting a hunk\nand then only partially accepting the split bits and pieces, the user\nis presented with a rather cryptic error:\n\n\terror: patch failed: :\n\terror: test: patch does not apply\n\tCannot remove worktree changes\n\nand the command would fail to stash the desired parts of the worktree\nchanges (even if the `stash` ref was actually updated correctly).\n\nWe even have a test case demonstrating that failure, carrying it for\nfour years already.\n\nThe explanation: when splitting a hunk, the changed lines are no longer\nseparated by more than 3 lines (which is the amount of context lines\nGit's diffs use by default), but less than that. So when staging only\npart of the diff hunk for stashing, the resulting diff that we want to\napply to the worktree in reverse will contain those changes to be\ndropped surrounded by three context lines, but since the diff is\nrelative to HEAD rather than to the worktree, these context lines will\nnot match.\n\nExample time. Let's assume that the file README contains these lines:\n\n\tWe\n\tthe\n\tpeople\n\nand the worktree added some lines so that it contains these lines\ninstead:\n\n\tWe\n\tare\n\tthe\n\tkind\n\tpeople\n\nand the user tries to stash the line containing \"are\", then the command\nwill internally stage this line to a temporary index file and try to\nrevert the diff between HEAD and that index file. The diff hunk that\n`git stash` tries to revert will look somewhat like this:\n\n\t@@ -1776,3 +1776,4\n\t We\n\t+are\n\t the\n\t people\n\nIt is obvious, now, that the trailing context lines overlap with the\npart of the original diff hunk that the user did *not* want to stash.\n\nKeeping in mind that context lines in diffs serve the primary purpose of\nfinding the exact location when the diff does not apply precisely (but\nwhen the exact line number in the file to be patched differs from the\nline number indicated in the diff), we work around this by reducing the\namount of context lines: the diff was just generated.\n\nNote: this is not a *full* fix for the issue. Just as demonstrated in\nt3701's 'add -p works with pathological context lines' test case, there\nare ambiguities in the diff format. It is very rare in practice, of\ncourse, to encounter such repeated lines.\n\nThe full solution for such cases would be to replace the approach of\ngenerating a diff from the stash and then applying it in reverse by\nemulating `git revert` (i.e. doing a 3-way merge). However, in `git\nstash -p` it would not apply to `HEAD` but instead to the worktree,\nwhich makes this non-trivial to implement as long as we also maintain a\nscripted version of `add -i`.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/stash.c | 2 +-\n t/t3904-stash-patch.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 6d586ef06df..a43a92ec743 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1041,7 +1041,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,\n \t}\n \n \tcp_diff_tree.git_cmd = 1;\n-\targv_array_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"HEAD\",\n+\targv_array_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"-U1\", \"HEAD\",\n \t\t\t oid_to_hex(&info->w_tree), \"--\", NULL);\n \tif (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n \t\tret = -1;\ndiff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh\nindex ab7d7aa6de1..accfe3845c4 100755\n--- a/t/t3904-stash-patch.sh\n+++ b/t/t3904-stash-patch.sh\n@@ -89,7 +89,7 @@ test_expect_success 'none of this moved HEAD' '\n \tverify_saved_head\n '\n \n-test_expect_failure 'stash -p with split hunk' '\n+test_expect_success 'stash -p with split hunk' '\n \tgit reset --hard &&\n \tcat >test <<-\\EOF &&\n \taaa\n-- \ngitgitgadget\n"}]}