Re: Bug: 'git am --abort' can silently reset the wrong branch
- From
Jeff King <peff@peff.net>
- Date
- May 8, 2009, 08:28 UTC
- Message-ID
- <20090508082826.GA29737@coredump.intra.peff.net>
- In-Reply-To
- <20090506191945.GG6325@blackpad>
On Wed, May 06, 2009 at 04:19:46PM -0300, Eduardo Habkost wrote:
> I've ben bitten by this multiple times, while maintaining a multi-branch > repository. 'git am --abort' can drop the whole history of a branch, if you > switch branches before running it. Below are the steps to reproduce the > problem:
Yeah, it would be nice if there was a safety valve here to cause "git am --abort" to realize that you had switched branches out from under it and not bother with resetting HEAD (but it should still blow away .git/rebase-apply).
Below is a patch which accomplishes that. However, I'm not sure it is the right direction. Switching branches and clobbering some other branch with --abort is just _one_ thing you can do to screw yourself. You could also have been doing useful work on the _same_ branch, and that would get clobbered by --abort. However, I'm not sure if we have a good way of telling the difference between "work which I did to try to get these patches to apply, but which should be thrown away when I abort" and "work which I did because I forgot I had an active git-am".
So maybe just picking the changed-branch situation is the best we can do. I'd be interested to hear comments from others.
This patch works for git-am; we would also need to do something similar for rebase.
-- >8 -- am: try not to clobber alternate branches on --abort
A user in the middle of a failed git-am may forget that they are there and begin doing other work. They may later realize that the "am" session is still active (when trying to rebase or apply another patch), at which point they are tempted to "git am --abort". However, this resets their HEAD back to ORIG_HEAD, clobbering any work they have done in the meantime.
This patch detects the situation where the branch has changed and skips the reset step (which assumes the user has the tree in a state they like now).
---
diff --git a/git-am.sh b/git-am.sh index 6d1848b..4d5f408 100755 --- a/git-am.sh +++ b/git-am.sh @@ -244,10 +244,13 @@ then exec git rebase --abort fi git rerere clear - test -f "$dotest/dirtyindex" || { + if ! test -f "$dotest/dirtyindex" && + test "`git symbolic-ref -q HEAD`" = \ + "`cat "$dotest/start-ref"`" + then git read-tree --reset -u HEAD ORIG_HEAD git reset ORIG_HEAD - } + fi rm -fr "$dotest" exit ;; esac @@ -304,6 +307,7 @@ else git update-ref -d ORIG_HEAD >/dev/null 2>&1 fi fi + git symbolic-ref -q HEAD >"$dotest/start-ref" fi case "$resolved" in diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh index 2b912d7..8e62e76 100755 --- a/t/t4151-am-abort.sh +++ b/t/t4151-am-abort.sh @@ -62,4 +62,19 @@ do done +test_expect_success 'am --abort does not clobber changed branch' ' + rm -f otherfile-* && + git checkout -b other && + echo content >unrelated && + git add unrelated && + git commit -m other && + git rev-parse other >expect && + git checkout master && + test_must_fail git am 000[1245]-*.patch && + git checkout other && + git am --abort && + git rev-parse other >actual && + test_cmp expect actual +' + test_done