{"thread":{"id":"26110","subject":"Dangerous \"git am --abort\" behavior","startedAt":"2010-12-20T18:31:05Z","lastAt":"2010-12-24T00:24:49Z","messageCount":12,"participants":["Linus Torvalds","Adam Monsen","Drew Northup","Junio C Hamano","Peter Krefting","Steven E. Harris"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"158404","messageId":"AANLkTinP4SArMkjvTXOEG=tf=8EcEdP9fPAB7F=iitSc@mail.gmail.com","threadId":"26110","inReplyTo":null,"subject":"Dangerous \"git am --abort\" behavior","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-12-20T18:31:05Z","receivedAt":"2010-12-20T18:31:05Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"I just noticed this, and I wonder if it has bitten me before without\nme noticing: \"git am --abort\" can be really dangerous.\n\nWhat happened today was that I had been doing a pull or two, and then\napplied an emailed patch with \"git am\" as usual. But as sometimes\nhappens, I actually had a previous \"git am\" that had failed - in fact,\nit was the same patch that I applied today that had had an earlier\nversion that no longer applied.\n\nSo I just did \"git am --abort\" to get rid of the old stale 'am' state,\nbut that actually also ended up aborting my \"git pull\". Oops.\n\nHappily, I noticed, and did a \"git reset --hard @{1}\" to get things\nback, but at no point did \"git am\" warn about the implicit \"reset\" it\ndid, that threw away non-am state.\n\nI suspect I've avoided this in the past because my normal approach to\ngetting rid of stale am state tends to be just the manual \"rm -rf\n.git/rebase-apply\", but it's also possible that I've simply not\nnoticed before.\n\nMaybe \"git am\" should actually save the last commit ID that it did,\nand only do the \"reset\" if the current HEAD matches the rebase-apply\nstate and warns if it doesn't? Or maybe we could just introduce a new\n\"git am --clean\" that just flushes any old pending state (ie does that\n\"clean_abort\" thing, which is basically just the \"rm -rf\" I've done by\nhand). Or both?\n\nComments?\n\n Linus\n"},{"id":"158411","messageId":"loom.20101220T203122-271@post.gmane.org","threadId":"26110","inReplyTo":"AANLkTinP4SArMkjvTXOEG=tf=8EcEdP9fPAB7F=iitSc@mail.gmail.com","subject":"Re: Dangerous \"git am --abort\" behavior","fromName":"Adam Monsen","fromEmail":"haircut@gmail.com","sentAt":"2010-12-20T19:35:00Z","receivedAt":"2010-12-20T19:35:00Z","isPatch":false,"sender":{"key":"haircut@gmail.com","avatar":"https://avatars.githubusercontent.com/u/50639?v=4"},"body":"Linus Torvalds writes:\n> What happened today was that I had been doing a pull or two, and then\n> applied an emailed patch with \"git am\" as usual. But as sometimes\n> happens, I actually had a previous \"git am\" that had failed - in fact,\n> it was the same patch that I applied today that had had an earlier\n> version that no longer applied.\n\nIt would be helpful if \"git status\" mentioned if a rebase or am operation is in \nprogress... this might have helped you avoid the situation described.\n"},{"id":"158416","messageId":"1292881979.23145.5.camel@drew-northup.unet.maine.edu","threadId":"26110","inReplyTo":"loom.20101220T203122-271@post.gmane.org","subject":"Re: Dangerous \"git am --abort\" behavior","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2010-12-20T21:52:59Z","receivedAt":"2010-12-20T21:52:59Z","isPatch":false,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Mon, 2010-12-20 at 19:35 +0000, Adam Monsen wrote:\n> Linus Torvalds writes:\n> > What happened today was that I had been doing a pull or two, and then\n> > applied an emailed patch with \"git am\" as usual. But as sometimes\n> > happens, I actually had a previous \"git am\" that had failed - in fact,\n> > it was the same patch that I applied today that had had an earlier\n> > version that no longer applied.\n> \n> It would be helpful if \"git status\" mentioned if a rebase or am operation is in \n> progress... this might have helped you avoid the situation described.\n\nAdam,\nPlease don't cull the CC list...\n\nIn any case, are there any other mult-part operations for which we might\nwant to report \"In Progress\" in the \"git status\" output? How do we best\nharvest and present this information to the user? This sounds like a\nmore general-purpose project than Linus' original scope.\n\n-- \n-Drew Northup N1XIM\n AKA RvnPhnx on OPN\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"158417","messageId":"AANLkTikUn+Mco3YeJ7Rj=xZrr1H5xr1Z0=cknf1MdCqC@mail.gmail.com","threadId":"26110","inReplyTo":"1292881979.23145.5.camel@drew-northup.unet.maine.edu","subject":"Re: Dangerous \"git am --abort\" behavior","fromName":"Adam Monsen","fromEmail":"haircut@gmail.com","sentAt":"2010-12-20T22:04:02Z","receivedAt":"2010-12-20T22:04:02Z","isPatch":false,"sender":{"key":"haircut@gmail.com","avatar":"https://avatars.githubusercontent.com/u/50639?v=4"},"body":"Another thing a pal just showed me is the awesome Git Bash/Zsh\ncompletion stuff in contrib/completion/ . That's a good way to keep\ntabs on whether you're in the middle of a rebase or am (as well as\nmany other statuses).\n\nDrew Northup wrote:\n> Please don't cull the CC list...\n\nI didn't, I replied at\nhttp://thread.gmane.org/gmane.comp.version-control.git/164002 (by\nclicking \"--Action--\" and selecting \"Followup\".\n"},{"id":"158422","messageId":"7vtyi8arxp.fsf@alter.siamese.dyndns.org","threadId":"26110","inReplyTo":"AANLkTinP4SArMkjvTXOEG=tf=8EcEdP9fPAB7F=iitSc@mail.gmail.com","subject":"Re: Dangerous \"git am --abort\" behavior","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-21T00:30:10Z","receivedAt":"2010-12-21T00:30:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> I just noticed this, and I wonder if it has bitten me before without\n> me noticing: \"git am --abort\" can be really dangerous.\n>\n> What happened today was that I had been doing a pull or two, and then\n> applied an emailed patch with \"git am\" as usual. But as sometimes\n> happens, I actually had a previous \"git am\" that had failed - in fact,\n> it was the same patch that I applied today that had had an earlier\n> version that no longer applied.\n\nI never got into this as I use bash completion in my PS1 in the real life,\nbut I've seen this happen while playing around, and I can see myself\neasily getting hurt by this behaviour without status in PS1.\n\n> Maybe \"git am\" should actually save the last commit ID that it did,\n> and only do the \"reset\" if the current HEAD matches the rebase-apply\n> state and warns if it doesn't? Or maybe we could just introduce a new\n> \"git am --clean\" that just flushes any old pending state (ie does that\n> \"clean_abort\" thing, which is basically just the \"rm -rf\" I've done by\n> hand). Or both?\n\nI sometimes wanted \"--clean\" myself, so it is a no-brainer to decide that\nit would be a good thing to add.\n\nThe last time I thought about this issue, I wasn't sure about \"compare\nwith the last commit\"---mostly because it wasn't clear what ramifications\nit would have. When you get refusal from \"am --abort\", how would you\nrecover from it?\n\nBack then my tentative conclusion was actually to get rid of \"am --abort\"\nand give \"am --clean\", making the final \"reset HEAD~$n\" the responsiblity\nof the user. But I forgot to pursue it.\n"},{"id":"158447","messageId":"7vsjxr7zdn.fsf@alter.siamese.dyndns.org","threadId":"26110","inReplyTo":"7vtyi8arxp.fsf@alter.siamese.dyndns.org","subject":"Re: Dangerous \"git am --abort\" behavior","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-21T18:29:56Z","receivedAt":"2010-12-21T18:29:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Linus Torvalds writes:\n> ...\n>> Maybe \"git am\" should actually save the last commit ID that it did,\n>> and only do the \"reset\" if the current HEAD matches the rebase-apply\n>> state and warns if it doesn't? Or maybe we could just introduce a new\n>> \"git am --clean\" that just flushes any old pending state (ie does that\n>> \"clean_abort\" thing, which is basically just the \"rm -rf\" I've done by\n>> hand). Or both?\n> ...\n> Back then my tentative conclusion was actually to get rid of \"am --abort\"\n> and give \"am --clean\", making the final \"reset HEAD~$n\" the responsiblity\n> of the user. But I forgot to pursue it.\n\nSo here is the first step in that direction. I suspect that stop_here\nshould also record what the current branch is, and safe_to_abort should\ncheck it (the potentially risky sequence is \"after a failed am, check out\na different branch and then realize you need to 'am --abort'\"), but that\nis left to interested others ;-) or a later round.\n\n-- >8 --\nSubject: am --abort: keep unrelated commits since the last failure and warn\n\nAfter making commits (either by pulling or doing their own work) after a\nfailed \"am\", the user will be reminded by next \"am\" invocation that there\nwas a failed \"am\" that the user needs to decide to resolve or to get rid\nof the old \"am\" attempt. The \"am --abort\" option was meant to help the\nlatter. However, it rewinded the HEAD back to the beginning of the failed\n\"am\" attempt, discarding commits made (perhaps by mistake) since.\n\nSigned-off-by: Junio C Hamano \n---\n git-am.sh | 27 +++++++++++++++++++++++++--\n t/t4151-am-abort.sh | 9 +++++++++\n 2 files changed, 34 insertions(+), 2 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex df09b42..cf1f64b 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -68,9 +68,31 @@ sq () {\n \n stop_here () {\n echo \"$1\" >\"$dotest/next\"\n+ git rev-parse --verify -q HEAD >\"$dotest/abort-safety\"\n exit 1\n }\n \n+safe_to_abort () {\n+\tif test -f \"$dotest/dirtyindex\"\n+\tthen\n+\t\treturn 1\n+\tfi\n+\n+\tif ! test -s \"$dotest/abort-safety\"\n+\tthen\n+\t\treturn 0\n+\tfi\n+\n+\tabort_safety=$(cat \"$dotest/abort-safety\")\n+\tif test \"z$(git rev-parse --verify -q HEAD)\" = \"z$abort_safety\"\n+\tthen\n+\t\treturn 0\n+\tfi\n+\techo >&2 \"You seem to have moved HEAD since the last 'am' failure.\"\n+\techo >&2 \"Not rewinding to ORIG_HEAD\"\n+\treturn 1\n+}\n+\n stop_here_user_resolve () {\n if [ -n \"$resolvemsg\" ]; then\n \t printf '%s\\n' \"$resolvemsg\"\n@@ -419,10 +441,11 @@ then\n \t\t\texec git rebase --abort\n \t\tfi\n \t\tgit rerere clear\n-\t\ttest -f \"$dotest/dirtyindex\" || {\n+\t\tif safe_to_abort\n+\t\tthen\n \t\t\tgit read-tree --reset -u HEAD ORIG_HEAD\n \t\t\tgit reset ORIG_HEAD\n-\t\t}\n+\t\tfi\n \t\trm -fr \"$dotest\"\n \t\texit ;;\n \tesac\ndiff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\nindex b55c411..001b1e3 100755\n--- a/t/t4151-am-abort.sh\n+++ b/t/t4151-am-abort.sh\n@@ -62,4 +62,13 @@ do\n \n done\n \n+test_expect_success 'am --abort will keep the local commits' '\n+\ttest_must_fail git am 0004-*.patch &&\n+\ttest_commit unrelated &&\n+\tgit rev-parse HEAD >expect &&\n+\tgit am --abort &&\n+\tgit rev-parse HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"158448","messageId":"AANLkTimqxCBpF2tCqjsPMnc11nh4MZx2bh0gD7Q=duG+@mail.gmail.com","threadId":"26110","inReplyTo":"7vsjxr7zdn.fsf@alter.siamese.dyndns.org","subject":"Re: Dangerous \"git am --abort\" behavior","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-12-21T18:46:27Z","receivedAt":"2010-12-21T18:46:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Tue, Dec 21, 2010 at 10:29 AM, Junio C Hamano wrote:\n>\n> So here is the first step in that direction.  I suspect that stop_here\n> should also record what the current branch is, and safe_to_abort should\n> check it (the potentially risky sequence is \"after a failed am, check out\n> a different branch and then realize you need to 'am --abort'\"), but that\n> is left to interested others ;-) or a later round.\n\nYeah, this patch looks good to me.\n\nAnd if you've switched branches, and do a \"git am --abort\" which still\nsees the expected commit, I actually think your patch does the right\nthing: we will rewind that new branch to ORIG_HEAD, and I think that\nis actually the semantics we want.\n\nSo what you can do with this is:\n\n - \"git am \" fails in the middle\n\n - you go \"hmm. I'm happy with what we did so far, but let's go back\nto check what's up\"\n\n - \"git checkout -b test-branch ; git am --abort\"\n\n - work on the original base and maybe try to re-apply the mbox with\nsoem manual editing or whatever...\n\nand that's exactly the semantics that your patch allows, which seems\nto be very flexible and useful. No?\n\nSo the only thing it disallows is having \"git am --abort\" actually\nabort some unrelated commit, which is I think the exact behavior we\nwant. In fact, if somebody has done a \"git pull\" or something, then\n\"ORIG_HEAD\" really doesn't mean what git am thinks it means. So I\nwonder if we should check ORIG_HEAD against \"beginning of 'git am'\"\ntoo, the way you check HEAD against the \"abort-safely\" point?\n\nAgain, if ORIG_HEAD doesn't match (for whatever reason - maybe\nsomebody switched branches and did a 'git reset --hard\" in that other\nbranch, and then switched back?), then \"git am --abort\" shouldn't\nabort to some random point that came from some non-am workflow, no?\n\nBut with the HEAD check, you'd really have to _work_ at screwing up,\nso the ORIG_HEAD check seems to be much less important.\n\n Linus\n"},{"id":"158449","messageId":"7voc8f7ykg.fsf@alter.siamese.dyndns.org","threadId":"26110","inReplyTo":"7vsjxr7zdn.fsf@alter.siamese.dyndns.org","subject":"Re: Dangerous \"git am --abort\" behavior","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-21T18:47:27Z","receivedAt":"2010-12-21T18:47:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> So here is the first step in that direction. I suspect that stop_here\n> should also record what the current branch is, and safe_to_abort should\n> check it (the potentially risky sequence is \"after a failed am, check out\n> a different branch and then realize you need to 'am --abort'\"), but that\n> is left to interested others ;-) or a later round.\n\nAnd here is that later round...\n\n-- >8 --\nSubject: [PATCH] am --abort: also check the current branch\n\nIf the user checks out another branch after an \"am\" failure, am --abort\nwould have rewound the tip of that branch back to where the last failed\n\"am\" started from, which would not be fun.\n\nSigned-off-by: Junio C Hamano \n---\n git-am.sh | 10 +++++++---\n t/t4151-am-abort.sh | 17 +++++++++++++++++\n 2 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex e5671f6..ca3f910 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -68,7 +68,9 @@ sq () {\n \n stop_here () {\n echo \"$1\" >\"$dotest/next\"\n- git rev-parse --verify -q HEAD >\"$dotest/abort-safety\"\n+ head=$(git rev-parse --verify -q HEAD)\n+ branch=$(git symbolic-ref -q HEAD)\n+ echo \"$head,$branch\" >\"$dotest/abort-safety\"\n exit 1\n }\n \n@@ -84,11 +86,13 @@ safe_to_abort () {\n \tfi\n \n \tabort_safety=$(cat \"$dotest/abort-safety\")\n-\tif test \"z$(git rev-parse --verify -q HEAD)\" = \"z$abort_safety\"\n+\thead=$(git rev-parse --verify -q HEAD)\n+\tbranch=$(git symbolic-ref -q HEAD)\n+\tif test \"z$head,$branch\" = \"z$abort_safety\"\n \tthen\n \t\treturn 0\n \tfi\n-\techo >&2 \"You seem to have moved HEAD since the last 'am' failure.\"\n+\techo >&2 \"You seem to have done some other things since the last 'am' failure.\"\n \techo >&2 \"Not rewinding to ORIG_HEAD\"\n \treturn 1\n }\ndiff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\nindex 001b1e3..23a9fb0 100755\n--- a/t/t4151-am-abort.sh\n+++ b/t/t4151-am-abort.sh\n@@ -71,4 +71,21 @@ test_expect_success 'am --abort will keep the local commits' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'am --abort will keep unrelated branch' '\n+\tgit reset --hard &&\n+\ttest_commit foo &&\n+\ttest_must_fail git am 0004-*.patch &&\n+\tgit checkout -b unrelated HEAD^ &&\n+\t(\n+\t\tgit rev-parse HEAD\n+\t\tgit symbolic-ref HEAD\n+\t) >expect &&\n+\tgit am --abort &&\n+\t(\n+\t\tgit rev-parse HEAD\n+\t\tgit symbolic-ref HEAD\n+\t) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.3.4.768.g2fa91\n"},{"id":"158476","messageId":"alpine.DEB.2.00.1012221046100.24315@ds9.cixit.se","threadId":"26110","inReplyTo":"AANLkTinP4SArMkjvTXOEG=tf=8EcEdP9fPAB7F=iitSc@mail.gmail.com","subject":"Re: Dangerous \"git am --abort\" behavior","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2010-12-22T09:49:19Z","receivedAt":"2010-12-22T09:49:19Z","isPatch":false,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Linus Torvalds:\n\n> I just noticed this, and I wonder if it has bitten me before without\n> me noticing: \"git am --abort\" can be really dangerous.\n\nIndeed, I have been bitten by that several times, having worked heavily on \napplying patches at $dayjob for a while now. I have taken to habit to always \ndo the same \"rm -rf .git/rebase-apply\" that you mention before doing anything \ninvolving am or rebase...\n\n> Or maybe we could just introduce a new \"git am --clean\" that just flushes \n> any old pending state (ie does that \"clean_abort\" thing, which is \n> basically just the \"rm -rf\" I've done by hand).\n\nThat would be very helpful, as manually doing a \"rm -rf\" inside the .git \ndirectory does make me nervous each time I do it...\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"158537","messageId":"m2tyi45ell.fsf@Spindle.sehlabs.com","threadId":"26110","inReplyTo":"AANLkTikUn+Mco3YeJ7Rj=xZrr1H5xr1Z0=cknf1MdCqC@mail.gmail.com","subject":"Re: Dangerous \"git am --abort\" behavior","fromName":"Steven E. Harris","fromEmail":"seh@panix.com","sentAt":"2010-12-23T22:06:14Z","receivedAt":"2010-12-23T22:06:14Z","isPatch":false,"sender":{"key":"seh@panix.com","avatar":"https://gravatar.com/avatar/d59ec0f7c010ee73cd67db381a5b865206fed17fd4278f12cdb6277db30033fc?d=mp&s=160"},"body":"Adam Monsen writes:\n\n> That's a good way to keep tabs on whether you're in the middle of a\n> rebase or am (as well as many other statuses).\n\nHow so? How are you using it for this purpose?\n\n-- \nSteven E. Harris\n"},{"id":"158538","messageId":"7vtyi415oo.fsf@alter.siamese.dyndns.org","threadId":"26110","inReplyTo":"m2tyi45ell.fsf@Spindle.sehlabs.com","subject":"Re: Dangerous \"git am --abort\" behavior","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-23T22:32:23Z","receivedAt":"2010-12-23T22:32:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Steven E. Harris\" writes:\n\n> Adam Monsen writes:\n>\n>> That's a good way to keep tabs on whether you're in the middle of a\n>> rebase or am (as well as many other statuses).\n>\n> How so? How are you using it for this purpose?\n\nThere is this gem in the completion script:\n\n# 3) Consider changing your PS1 to also show the current branch:\n# PS1='[\\u@\\h \\W$(__git_ps1 \" (%s)\")]\\$ '\n\nWith this, you will see something like:\n\n [junio@alter git.git (master|AM)]$ \n\nas your prompt while you are working to fix corrupt patch you tried but\nfailed to apply.\n"},{"id":"158540","messageId":"m2pqss586m.fsf@Spindle.sehlabs.com","threadId":"26110","inReplyTo":"7vtyi415oo.fsf@alter.siamese.dyndns.org","subject":"Re: Dangerous \"git am --abort\" behavior","fromName":"Steven E. Harris","fromEmail":"seh@panix.com","sentAt":"2010-12-24T00:24:49Z","receivedAt":"2010-12-24T00:24:49Z","isPatch":false,"sender":{"key":"seh@panix.com","avatar":"https://gravatar.com/avatar/d59ec0f7c010ee73cd67db381a5b865206fed17fd4278f12cdb6277db30033fc?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> There is this gem in the completion script:\n\nThanks. I use zsh, and didn't want to force zsh to load the bash\ncompletion scripts. After fumbling around first with the \"vcs_info\"\nfacility from zshcontrib, which worked acceptably, I found the\n\"zsh-git-prompt\" project¹, integrated that, and am now amazed.\n\n\nFootnotes: \n¹ https://github.com/olivierverdier/zsh-git-prompt\n\n-- \nSteven E. Harris\n"}]}