# Dangerous "git am --abort" behavior 12 messages from 2010-12-20 to 2010-12-24. Participants: Linus Torvalds, Adam Monsen, Drew Northup, Junio C Hamano, Peter Krefting, Steven E. Harris. Thread: https://gitlist.dev/t/26110 ## Linus Torvalds, 2010-12-20 18:31 Subject: Dangerous "git am --abort" behavior Message-ID: URL: https://gitlist.dev/e/AANLkTinP4SArMkjvTXOEG%3Dtf%3D8EcEdP9fPAB7F%3DiitSc%40mail.gmail.com ``` 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, 2010-12-20 19:35 Subject: Re: Dangerous "git am --abort" behavior Message-ID: URL: https://gitlist.dev/e/loom.20101220T203122-271%40post.gmane.org In-Reply-To: ``` 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. ``` ## Drew Northup, 2010-12-20 21:52 Subject: Re: Dangerous "git am --abort" behavior Message-ID: <1292881979.23145.5.camel@drew-northup.unet.maine.edu> URL: https://gitlist.dev/e/1292881979.23145.5.camel%40drew-northup.unet.maine.edu In-Reply-To: ``` 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 ``` ## Adam Monsen, 2010-12-20 22:04 Subject: Re: Dangerous "git am --abort" behavior Message-ID: URL: https://gitlist.dev/e/AANLkTikUn%2BMco3YeJ7Rj%3DxZrr1H5xr1Z0%3Dcknf1MdCqC%40mail.gmail.com In-Reply-To: <1292881979.23145.5.camel@drew-northup.unet.maine.edu> ``` 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". ``` ## Junio C Hamano, 2010-12-21 00:30 Subject: Re: Dangerous "git am --abort" behavior Message-ID: <7vtyi8arxp.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtyi8arxp.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Linus Torvalds 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. ``` ## Junio C Hamano, 2010-12-21 18:29 Subject: Re: Dangerous "git am --abort" behavior Message-ID: <7vsjxr7zdn.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsjxr7zdn.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vtyi8arxp.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Linus Torvalds 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 --- 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, 2010-12-21 18:46 Subject: Re: Dangerous "git am --abort" behavior Message-ID: URL: https://gitlist.dev/e/AANLkTimqxCBpF2tCqjsPMnc11nh4MZx2bh0gD7Q%3DduG%2B%40mail.gmail.com In-Reply-To: <7vsjxr7zdn.fsf@alter.siamese.dyndns.org> ``` On Tue, Dec 21, 2010 at 10:29 AM, Junio C Hamano 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 " 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, 2010-12-21 18:47 Subject: Re: Dangerous "git am --abort" behavior Message-ID: <7voc8f7ykg.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7voc8f7ykg.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vsjxr7zdn.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano 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 --- 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, 2010-12-22 09:49 Subject: Re: Dangerous "git am --abort" behavior Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.00.1012221046100.24315%40ds9.cixit.se In-Reply-To: ``` 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/ ``` ## Steven E. Harris, 2010-12-23 22:06 Subject: Re: Dangerous "git am --abort" behavior Message-ID: URL: https://gitlist.dev/e/m2tyi45ell.fsf%40Spindle.sehlabs.com In-Reply-To: ``` Adam Monsen 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, 2010-12-23 22:32 Subject: Re: Dangerous "git am --abort" behavior Message-ID: <7vtyi415oo.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtyi415oo.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` "Steven E. Harris" writes: > Adam Monsen 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, 2010-12-24 00:24 Subject: Re: Dangerous "git am --abort" behavior Message-ID: URL: https://gitlist.dev/e/m2pqss586m.fsf%40Spindle.sehlabs.com In-Reply-To: <7vtyi415oo.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano 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 ```