Re: Dangerous "git am --abort" behavior

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> ...
>> Maybe "git am" should actually save the last commit ID that it did,
>> and only do the "reset" if the current HEAD matches the rebase-apply
>> state and warns if it doesn't? Or maybe we could just introduce a new
>> "git am --clean" that just flushes any old pending state (ie does that
>> "clean_abort" thing, which is basically just the "rm -rf" I've done by
>> hand). Or both?
> ...
> Back then my tentative conclusion was actually to get rid of "am --abort"
> and give "am --clean", making the final "reset HEAD~$n" the responsiblity
> of the user.  But I forgot to pursue it.

So here is the first step in that direction.  I suspect that stop_here
should also record what the current branch is, and safe_to_abort should
check it (the potentially risky sequence is "after a failed am, check out
a different branch and then realize you need to 'am --abort'"), but that
is left to interested others ;-) or a later round.

-- >8 --
Subject: am --abort: keep unrelated commits since the last failure and warn

After making commits (either by pulling or doing their own work) after a
failed "am", the user will be reminded by next "am" invocation that there
was a failed "am" that the user needs to decide to resolve or to get rid
of the old "am" attempt.  The "am --abort" option was meant to help the
latter.  However, it rewinded the HEAD back to the beginning of the failed
"am" attempt, discarding commits made (perhaps by mistake) since.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-am.sh           |   27 +++++++++++++++++++++++++--
 t/t4151-am-abort.sh |    9 +++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index df09b42..cf1f64b 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -68,9 +68,31 @@ sq () {
 
 stop_here () {
     echo "$1" >"$dotest/next"
+    git rev-parse --verify -q HEAD >"$dotest/abort-safety"
     exit 1
 }
 
+safe_to_abort () {
+	if test -f "$dotest/dirtyindex"
+	then
+		return 1
+	fi
+
+	if ! test -s "$dotest/abort-safety"
+	then
+		return 0
+	fi
+
+	abort_safety=$(cat "$dotest/abort-safety")
+	if test "z$(git rev-parse --verify -q HEAD)" = "z$abort_safety"
+	then
+		return 0
+	fi
+	echo >&2 "You seem to have moved HEAD since the last 'am' failure."
+	echo >&2 "Not rewinding to ORIG_HEAD"
+	return 1
+}
+
 stop_here_user_resolve () {
     if [ -n "$resolvemsg" ]; then
 	    printf '%s\n' "$resolvemsg"
@@ -419,10 +441,11 @@ then
 			exec git rebase --abort
 		fi
 		git rerere clear
-		test -f "$dotest/dirtyindex" || {
+		if safe_to_abort
+		then
 			git read-tree --reset -u HEAD ORIG_HEAD
 			git reset ORIG_HEAD
-		}
+		fi
 		rm -fr "$dotest"
 		exit ;;
 	esac
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index b55c411..001b1e3 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -62,4 +62,13 @@ do
 
 done
 
+test_expect_success 'am --abort will keep the local commits' '
+	test_must_fail git am 0004-*.patch &&
+	test_commit unrelated &&
+	git rev-parse HEAD >expect &&
+	git am --abort &&
+	git rev-parse HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done