Fix a git stash -p corner-case
#602
Conversation
In 7e9e048 (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>
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>
|
/submit |
|
Submitted as pull.602.git.1586371954.gitgitgadget@gmail.com |
|
This branch is now known as |
|
This patch series was integrated into pu via git@5e55a9f. |
|
This patch series was integrated into pu via git@503f577. |
|
This patch series was integrated into pu via git@88ec648. |
|
This patch series was integrated into pu via git@c363b17. |
|
This patch series was integrated into pu via git@67bf5c6. |
|
This patch series was integrated into pu via git@aba56a1. |
|
This patch series was integrated into pu via git@3368f88. |
|
This patch series was integrated into pu via git@07b9d9a. |
|
This patch series was integrated into pu via git@c68ca94. |
|
This patch series was integrated into pu via git@f6b2d52. |
|
This patch series was integrated into pu via git@6dd0025. |
|
This patch series was integrated into pu via git@c58d47d. |
|
This patch series was integrated into pu via git@13a8e7d. |
|
This patch series was integrated into pu via git@e71e511. |
|
This patch series was integrated into pu via git@0eedf3e. |
|
This patch series was integrated into next via git@435bf60. |
|
This patch series was integrated into pu via git@2fe0e92. |
|
This patch series was integrated into pu via git@0b6b30e. |
|
This patch series was integrated into pu via git@44e7ce4. |
|
This patch series was integrated into pu via git@e81ecff. |
|
This patch series was integrated into next via git@e81ecff. |
|
This patch series was integrated into master via git@e81ecff. |
|
Closed via e81ecff. |
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.