Re: Bug: 'git am --abort' can silently reset the wrong branch

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