Re: Dangerous "git am --abort" behavior

Junio C Hamano [off-list ref] writes:

Linus Torvalds [off-list ref] writes:
...

quoted

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 <redacted>
---
 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

On Tue, Dec 21, 2010 at 10:29 AM, Junio C Hamano [off-list ref] wrote:

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.

Yeah, this patch looks good to me.

And if you've switched branches, and do a "git am --abort" which still
sees the expected commit, I actually think your patch does the right
thing: we will rewind that new branch to ORIG_HEAD, and I think that
is actually the semantics we want.

So what you can do with this is:

 - "git am <mbox-file>" fails in the middle

 - you go "hmm. I'm happy with what we did so far, but let's go back
to check what's up"

 - "git checkout -b test-branch ; git am --abort"

 - work on the original base and maybe try to re-apply the mbox with
soem manual editing or whatever...

and that's exactly the semantics that your patch allows, which seems
to be very flexible and useful. No?

So the only thing it disallows is having "git am --abort" actually
abort some unrelated commit, which is I think the exact behavior we
want. In fact, if somebody has done a "git pull" or something, then
"ORIG_HEAD" really doesn't mean what git am thinks it means. So I
wonder if we should check ORIG_HEAD against "beginning of 'git am'"
too, the way you check HEAD against the "abort-safely" point?

Again, if ORIG_HEAD doesn't match (for whatever reason - maybe
somebody switched branches and did a 'git reset --hard" in that other
branch, and then switched back?), then "git am --abort" shouldn't
abort to some random point that came from some non-am workflow, no?

But with the HEAD check, you'd really have to _work_ at screwing up,
so the ORIG_HEAD check seems to be much less important.

                              Linus