Dangerous "git am --abort" behavior

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.

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

Re: Dangerous "git am --abort" behavior

[Adam Monsen]

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.

Re: Dangerous "git am --abort" behavior

[Drew Northup]

On Mon, 2010-12-20 at 19:35 +0000, Adam Monsen wrote:
> 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

Re: Dangerous "git am --abort" behavior

[Adam Monsen]

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

Re: Dangerous "git am --abort" behavior

[Steven E. Harris]

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

Re: Dangerous "git am --abort" behavior

[Junio C Hamano]

"Steven E. Harris" <seh@panix.com> writes:

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

Re: Dangerous "git am --abort" behavior

[Steven E. Harris]

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

Re: Dangerous "git am --abort" behavior

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

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

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

Re: Dangerous "git am --abort" behavior

[Junio C Hamano]

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

Re: Dangerous "git am --abort" behavior

[Linus Torvalds]

On Tue, Dec 21, 2010 at 10:29 AM, Junio C Hamano <gitster@pobox.com> 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

Re: Dangerous "git am --abort" behavior

[Junio C Hamano]

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

> 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

Re: Dangerous "git am --abort" behavior

[Peter Krefting]

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/