[PATCH 0/2] Fix a git stash -p corner-case

[PATCH 0/2] Fix a git stash -p corner-case

[Johannes Schindelin via GitGitGadget]

This corner case came up a couple of times in my personal usage of git stash
-p: I wanted to stash only part of a split hunk, and it didn't work.

I originally intended to do a full fix (for details, see the commit message
of the second patch), but in the year since I wrote this patch pair, I found
the current work-around plenty satisfactory.

Johannes Schindelin (2):
  t3904: fix incorrect demonstration of a bug
  stash -p: (partially) fix bug concerning split hunks

 builtin/stash.c        | 2 +-
 t/t3904-stash-patch.sh | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)


base-commit: 9fadedd637b312089337d73c3ed8447e9f0aa775
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-602%2Fdscho%2Fstash-p-corner-case-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-602/dscho/stash-p-corner-case-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/602
-- 
gitgitgadget

[PATCH 1/2] t3904: fix incorrect demonstration of a bug

[Johannes Schindelin via GitGitGadget]

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

In 7e9e048661 (stash -p: demonstrate failure of split with mixed y/n,
2015-04-16), a regression test for a known breakage that was added to
the test script `t3904-stash-patch.sh` that demonstrated that splitting
a hunk and trying to stash only part of that split hunk fails (but
shouldn't).

As expected, it still fails, but for the wrong reason: once the bug is
fixed, we would expect stderr to show nothing, yet the regression test
expects stderr to show something.

Let's fix that by telling that regression test case to expect nothing to
be printed to stderr.

While at it, also drop the obvious left-over from debugging where the
regression test did not mind `git stash -p` to return a non-zero exit
status.

Of course, the regression test still fails, but this time for the
correct reason.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3904-stash-patch.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index 9546b6f8a4e..ab7d7aa6de1 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -106,8 +106,8 @@ test_expect_failure 'stash -p with split hunk' '
 	ccc
 	EOF
 	printf "%s\n" s n y q |
-	test_might_fail git stash -p 2>error &&
-	! test_must_be_empty error &&
+	git stash -p 2>error &&
+	test_must_be_empty error &&
 	grep "added line 1" test &&
 	! grep "added line 2" test
 '
-- 
gitgitgadget

[PATCH 2/2] stash -p: (partially) fix bug concerning split hunks

[Johannes Schindelin via GitGitGadget]

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

When trying to stash part of the worktree changes by splitting a hunk
and then only partially accepting the split bits and pieces, the user
is presented with a rather cryptic error:

	error: patch failed: <file>:<line>
	error: test: patch does not apply
	Cannot remove worktree changes

and the command would fail to stash the desired parts of the worktree
changes (even if the `stash` ref was actually updated correctly).

We even have a test case demonstrating that failure, carrying it for
four years already.

The explanation: when splitting a hunk, the changed lines are no longer
separated by more than 3 lines (which is the amount of context lines
Git's diffs use by default), but less than that. So when staging only
part of the diff hunk for stashing, the resulting diff that we want to
apply to the worktree in reverse will contain those changes to be
dropped surrounded by three context lines, but since the diff is
relative to HEAD rather than to the worktree, these context lines will
not match.

Example time. Let's assume that the file README contains these lines:

	We
	the
	people

and the worktree added some lines so that it contains these lines
instead:

	We
	are
	the
	kind
	people

and the user tries to stash the line containing "are", then the command
will internally stage this line to a temporary index file and try to
revert the diff between HEAD and that index file. The diff hunk that
`git stash` tries to revert will look somewhat like this:

	@@ -1776,3 +1776,4
	 We
	+are
	 the
	 people

It is obvious, now, that the trailing context lines overlap with the
part of the original diff hunk that the user did *not* want to stash.

Keeping in mind that context lines in diffs serve the primary purpose of
finding the exact location when the diff does not apply precisely (but
when the exact line number in the file to be patched differs from the
line number indicated in the diff), we work around this by reducing the
amount of context lines: the diff was just generated.

Note: this is not a *full* fix for the issue. Just as demonstrated in
t3701's 'add -p works with pathological context lines' test case, there
are ambiguities in the diff format. It is very rare in practice, of
course, to encounter such repeated lines.

The full solution for such cases would be to replace the approach of
generating a diff from the stash and then applying it in reverse by
emulating `git revert` (i.e. doing a 3-way merge). However, in `git
stash -p` it would not apply to `HEAD` but instead to the worktree,
which makes this non-trivial to implement as long as we also maintain a
scripted version of `add -i`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/stash.c        | 2 +-
 t/t3904-stash-patch.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 6d586ef06df..a43a92ec743 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1041,7 +1041,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 	}
 
 	cp_diff_tree.git_cmd = 1;
-	argv_array_pushl(&cp_diff_tree.args, "diff-tree", "-p", "HEAD",
+	argv_array_pushl(&cp_diff_tree.args, "diff-tree", "-p", "-U1", "HEAD",
 			 oid_to_hex(&info->w_tree), "--", NULL);
 	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
 		ret = -1;
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index ab7d7aa6de1..accfe3845c4 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -89,7 +89,7 @@ test_expect_success 'none of this moved HEAD' '
 	verify_saved_head
 '
 
-test_expect_failure 'stash -p with split hunk' '
+test_expect_success 'stash -p with split hunk' '
 	git reset --hard &&
 	cat >test <<-\EOF &&
 	aaa
-- 
gitgitgadget