threads / discuss / 26110

Dangerous "git am --abort" behavior

Subject: Dangerous "git am --abort" behavior

## tl;dr

12 messages between Dec 20, 2010 and Dec 24, 2010.

replies: 11people: 6as markdown or json

Linus Torvalds· Dec 20, 2010, 18:31 UTC · lore

I just noticed this, and I wonder if it has bitten me before without me noticing: "git am --abort" can be really dangerous.

What happened today was that I had been doing a pull or two, and then applied an emailed patch with "git am" as usual. But as sometimes happens, I actually had a previous "git am" that had failed - in fact, it was the same patch that I applied today that had had an earlier version that no longer applied.

So I just did "git am --abort" to get rid of the old stale 'am' state, but that actually also ended up aborting my "git pull". Oops.

Happily, I noticed, and did a "git reset --hard @{1}" to get things back, but at no point did "git am" warn about the implicit "reset" it did, that threw away non-am state.

I suspect I've avoided this in the past because my normal approach to getting rid of stale am state tends to be just the manual "rm -rf .git/rebase-apply", but it's also possible that I've simply not noticed before.

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?

Comments?
                     Linus
Adam Monsen· Dec 20, 2010, 19:35 UTC · re: Linus Torvalds · lore

Re: Dangerous "git am --abort" behavior

Linus Torvalds writes:
Show 5 quoted lines
> What happened today was that I had been doing a pull or two, and then
> applied an emailed patch with "git am" as usual. But as sometimes
> happens, I actually had a previous "git am" that had failed - in fact,
> it was the same patch that I applied today that had had an earlier
> version that no longer applied.

It would be helpful if "git status" mentioned if a rebase or am operation is in progress... this might have helped you avoid the situation described.

Drew Northup· Dec 20, 2010, 21:52 UTC · re: Adam Monsen · lore

Re: Dangerous "git am --abort" behavior

On Mon, 2010-12-20 at 19:35 +0000, Adam Monsen wrote:
Show 9 quoted lines
> Linus Torvalds writes:
> > What happened today was that I had been doing a pull or two, and then
> > applied an emailed patch with "git am" as usual. But as sometimes
> > happens, I actually had a previous "git am" that had failed - in fact,
> > it was the same patch that I applied today that had had an earlier
> > version that no longer applied.
> 
> It would be helpful if "git status" mentioned if a rebase or am operation is in 
> progress... this might have helped you avoid the situation described.

Adam, Please don't cull the CC list...

In any case, are there any other mult-part operations for which we might want to report "In Progress" in the "git status" output? How do we best harvest and present this information to the user? This sounds like a more general-purpose project than Linus' original scope.

-- 
-Drew Northup N1XIM
   AKA RvnPhnx on OPN
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
Adam Monsen· Dec 20, 2010, 22:04 UTC · re: Drew Northup · lore

Re: Dangerous "git am --abort" behavior

Another thing a pal just showed me is the awesome Git Bash/Zsh completion stuff in contrib/completion/ . That's a good way to keep tabs on whether you're in the middle of a rebase or am (as well as many other statuses).

Drew Northup wrote:
> Please don't cull the CC list...

I didn't, I replied at http://thread.gmane.org/gmane.comp.version-control.git/164002 (by clicking "--Action--" and selecting "Followup".

Steven E. Harris· Dec 23, 2010, 22:06 UTC · re: Adam Monsen · lore

Re: Dangerous "git am --abort" behavior

Adam Monsen <haircut@gmail.com> writes:
> That's a good way to keep tabs on whether you're in the middle of a
> rebase or am (as well as many other statuses).
How so? How are you using it for this purpose?
-- 
Steven E. Harris
Junio C Hamano· Dec 23, 2010, 22:32 UTC · re: Steven E. Harris · lore

Re: Dangerous "git am --abort" behavior

"Steven E. Harris" <seh@panix.com> writes:
Show 6 quoted lines
> Adam Monsen <haircut@gmail.com> writes:
>
>> That's a good way to keep tabs on whether you're in the middle of a
>> rebase or am (as well as many other statuses).
>
> How so? How are you using it for this purpose?
There is this gem in the completion script:

# 3) Consider changing your PS1 to also show the current branch: # PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ '

With this, you will see something like:
    [junio@alter git.git (master|AM)]$

as your prompt while you are working to fix corrupt patch you tried but failed to apply.

Steven E. Harris· Dec 24, 2010, 00:24 UTC · re: Junio C Hamano · lore

Re: Dangerous "git am --abort" behavior

Junio C Hamano <gitster@pobox.com> writes:
> There is this gem in the completion script:

Thanks. I use zsh, and didn't want to force zsh to load the bash completion scripts. After fumbling around first with the "vcs_info" facility from zshcontrib, which worked acceptably, I found the "zsh-git-prompt" project¹, integrated that, and am now amazed.

Footnotes: 
¹ https://github.com/olivierverdier/zsh-git-prompt
-- 
Steven E. Harris
Junio C Hamano· Dec 21, 2010, 00:30 UTC · re: Linus Torvalds · lore

Re: Dangerous "git am --abort" behavior

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 8 quoted lines
> I just noticed this, and I wonder if it has bitten me before without
> me noticing: "git am --abort" can be really dangerous.
>
> What happened today was that I had been doing a pull or two, and then
> applied an emailed patch with "git am" as usual. But as sometimes
> happens, I actually had a previous "git am" that had failed - in fact,
> it was the same patch that I applied today that had had an earlier
> version that no longer applied.

I never got into this as I use bash completion in my PS1 in the real life, but I've seen this happen while playing around, and I can see myself easily getting hurt by this behaviour without status in PS1.

Show 6 quoted lines
> 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?

I sometimes wanted "--clean" myself, so it is a no-brainer to decide that it would be a good thing to add.

The last time I thought about this issue, I wasn't sure about "compare with the last commit"---mostly because it wasn't clear what ramifications it would have. When you get refusal from "am --abort", how would you recover from it?

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.

Junio C Hamano· Dec 21, 2010, 18:29 UTC · re: Junio C Hamano · lore

Re: Dangerous "git am --abort" behavior

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> 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
Linus Torvalds· Dec 21, 2010, 18:46 UTC · re: Junio C Hamano · lore

Re: Dangerous "git am --abort" behavior

On Tue, Dec 21, 2010 at 10:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
>
> 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
Junio C Hamano· Dec 21, 2010, 18:47 UTC · re: Junio C Hamano · lore

Re: Dangerous "git am --abort" behavior

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> 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.
And here is that later round...
-- >8 --
Subject: [PATCH] am --abort: also check the current branch

If the user checks out another branch after an "am" failure, am --abort would have rewound the tip of that branch back to where the last failed "am" started from, which would not be fun.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-am.sh           |   10 +++++++---
 t/t4151-am-abort.sh |   17 +++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index e5671f6..ca3f910 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -68,7 +68,9 @@ sq () {
 
 stop_here () {
     echo "$1" >"$dotest/next"
-    git rev-parse --verify -q HEAD >"$dotest/abort-safety"
+    head=$(git rev-parse --verify -q HEAD)
+    branch=$(git symbolic-ref -q HEAD)
+    echo "$head,$branch" >"$dotest/abort-safety"
     exit 1
 }
 
@@ -84,11 +86,13 @@ safe_to_abort () {
 	fi
 
 	abort_safety=$(cat "$dotest/abort-safety")
-	if test "z$(git rev-parse --verify -q HEAD)" = "z$abort_safety"
+	head=$(git rev-parse --verify -q HEAD)
+	branch=$(git symbolic-ref -q HEAD)
+	if test "z$head,$branch" = "z$abort_safety"
 	then
 		return 0
 	fi
-	echo >&2 "You seem to have moved HEAD since the last 'am' failure."
+	echo >&2 "You seem to have done some other things since the last 'am' failure."
 	echo >&2 "Not rewinding to ORIG_HEAD"
 	return 1
 }
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index 001b1e3..23a9fb0 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -71,4 +71,21 @@ test_expect_success 'am --abort will keep the local commits' '
 	test_cmp expect actual
 '
 
+test_expect_success 'am --abort will keep unrelated branch' '
+	git reset --hard &&
+	test_commit foo &&
+	test_must_fail git am 0004-*.patch &&
+	git checkout -b unrelated HEAD^ &&
+	(
+		git rev-parse HEAD
+		git symbolic-ref HEAD
+	) >expect &&
+	git am --abort &&
+	(
+		git rev-parse HEAD
+		git symbolic-ref HEAD
+	) >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
1.7.3.4.768.g2fa91
Peter Krefting· Dec 22, 2010, 09:49 UTC · re: Linus Torvalds · lore

Re: Dangerous "git am --abort" behavior

Linus Torvalds:
> I just noticed this, and I wonder if it has bitten me before without
> me noticing: "git am --abort" can be really dangerous.

Indeed, I have been bitten by that several times, having worked heavily on applying patches at $dayjob for a while now. I have taken to habit to always do the same "rm -rf .git/rebase-apply" that you mention before doing anything involving am or rebase...

> 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).

That would be very helpful, as manually doing a "rm -rf" inside the .git directory does make me nervous each time I do it...

-- 
\\// Peter - http://www.softwolves.pp.se/

← back to recent threads