{"thread":{"id":"34126","subject":"[PATCH] am: handle stray $dotest directory case","startedAt":"2013-06-13T14:17:11Z","lastAt":"2013-06-13T22:49:22Z","messageCount":6,"participants":["Ramkumar Ramachandra","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"220731","messageId":"1371133031-28049-1-git-send-email-artagnon@gmail.com","threadId":"34126","inReplyTo":null,"subject":"[PATCH] am: handle stray $dotest directory case","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-13T14:17:11Z","receivedAt":"2013-06-13T14:17:11Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The following bug has been observed since rr/rebase-autostash:\n\n $ git am # no input file\n ^C\n $ git am --abort\n Resolve operation not in progress, we are not resuming.\n\nThis happens because the following test fails:\n\n test -d \"$dotest\" && test -f \"$dotest/last\" && test -f \"$dotest/next\"\n\nand am precludes the possibility of a stray $dotest directory\nexisting (when $dotest/{last,next} are not present).\n\nFix the bug by checking for a stray $dotest directory explicitly and\nremoving it on --abort.\n\nReported-by: Junio C Hamano \nSigned-off-by: Ramkumar Ramachandra \n---\n git-am.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 1cf3d1d..f46a123 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -506,6 +506,11 @@ then\n \tesac\n \trm -f \"$dotest/dirtyindex\"\n else\n+\t# Possible stray $dotest directory\n+\tif test -d \"$dotest\" && test t = \"$abort\"; then\n+\t\tclean_abort\n+\tfi\n+\n \t# Make sure we are not given --skip, --resolved, nor --abort\n \ttest \"$skip$resolved$abort\" = \"\" ||\n \t\tdie \"$(gettext \"Resolve operation not in progress, we are not resuming.\")\"\n-- \n1.8.3.1.379.ged35616\n"},{"id":"220742","messageId":"7v61xivsxh.fsf@alter.siamese.dyndns.org","threadId":"34126","inReplyTo":"1371133031-28049-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] am: handle stray $dotest directory case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-13T17:09:30Z","receivedAt":"2013-06-13T17:09:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> The following bug has been observed since rr/rebase-autostash:\n>\n> $ git am # no input file\n> ^C\n> $ git am --abort\n> Resolve operation not in progress, we are not resuming.\n>\n> This happens because the following test fails:\n>\n> test -d \"$dotest\" && test -f \"$dotest/last\" && test -f \"$dotest/next\"\n>\n> and am precludes the possibility of a stray $dotest directory\n> existing (when $dotest/{last,next} are not present).\n\nWhy did the original code sequence that read:\n\n\tif test -d \"$dotest\"\n then\n\t\t... handle skip, resolved, abort, because\n ... these can be run ONLY when we know we have\n ... started an \"am\" session.\n\t\t... catch new \"git am mbox\" invocation and error\n ... out, because that should not be allowed when\n ... we know we have started an \"am\" session.\n\nhad to change its guarding condition to\n\n\tif test -d \"$dotest\" && test -f \"$dotest/last\" && test -f \"$dotest/next\"\n\nin the first place? Perhaps _that_ guarding condition is what needs\nto be fixed, by reverting it back to just \"does $dotest exist?\"\n\nAdding a single case workaround smells like a band-aid to me.\n\n> Fix the bug by checking for a stray $dotest directory explicitly and\n> removing it on --abort.\n>\n> Reported-by: Junio C Hamano \n> Signed-off-by: Ramkumar Ramachandra \n> ---\n> git-am.sh | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/git-am.sh b/git-am.sh\n> index 1cf3d1d..f46a123 100755\n> --- a/git-am.sh\n> +++ b/git-am.sh\n> @@ -506,6 +506,11 @@ then\n> \tesac\n> \trm -f \"$dotest/dirtyindex\"\n> else\n> +\t# Possible stray $dotest directory\n> +\tif test -d \"$dotest\" && test t = \"$abort\"; then\n> +\t\tclean_abort\n> +\tfi\n> +\n> \t# Make sure we are not given --skip, --resolved, nor --abort\n> \ttest \"$skip$resolved$abort\" = \"\" ||\n> \t\tdie \"$(gettext \"Resolve operation not in progress, we are not resuming.\")\"\n"},{"id":"220745","messageId":"CALkWK0mUnNTfJQuGQAoMtnsOXvXU+z+W6D_MMuio1sq4vkucjA@mail.gmail.com","threadId":"34126","inReplyTo":"7v61xivsxh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] am: handle stray $dotest directory case","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-13T17:33:56Z","receivedAt":"2013-06-13T17:33:56Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Perhaps _that_ guarding condition is what needs\n> to be fixed, by reverting it back to just \"does $dotest exist?\"\n>\n> Adding a single case workaround smells like a band-aid to me.\n\nLike I pointed out earlier, the original codepath is not equipped to\nhandle this case. A \"normal\" git am --abort runs:\n\n git read-tree --reset -u HEAD ORIG_HEAD\n git reset ORIG_HEAD\n\nblowing away the top commit in the scenario you outlined.\n\nThis happens because that codepath incorrectly believes that an am is\n\"in progress\". What this means is that it believes that some of the\nam code actually got executed in the previous run, setting ORIG_HEAD\netc. In your scenario\n\n % git am\n ^C\n\nnothing is executed, and a stray directory is left behind.\n\nIf anything, I think the check for $dotest/{next,last} has made the\ncode more robust by correctly verifying that some code did get\nexecuted, and that an am is indeed in progress. The bug you have\noutlined is equivalent to:\n\n % mkdir .git/rebase-apply\n % git am --abort\n\nTherefore, the fix is to treat it as exactly that: a stray directory\nthat needs to be cleaned up in the codepath that treats it as a \"fresh\nrun\"; not going through the \"normal\" am --abort logic and blowing away\nthe top commit.\n\nMakes sense?\n"},{"id":"220764","messageId":"7vmwqtua5r.fsf@alter.siamese.dyndns.org","threadId":"34126","inReplyTo":"CALkWK0mUnNTfJQuGQAoMtnsOXvXU+z+W6D_MMuio1sq4vkucjA@mail.gmail.com","subject":"Re: [PATCH] am: handle stray $dotest directory case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-13T18:40:16Z","receivedAt":"2013-06-13T18:40:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>> Perhaps _that_ guarding condition is what needs\n>> to be fixed, by reverting it back to just \"does $dotest exist?\"\n>>\n>> Adding a single case workaround smells like a band-aid to me.\n>\n> Like I pointed out earlier, the original codepath is not equipped to\n> handle this case. A \"normal\" git am --abort runs:\n>\n> git read-tree --reset -u HEAD ORIG_HEAD\n> git reset ORIG_HEAD\n\nHmph, when did ORIG_HEAD set, and what commit does it point at?\n\nAs \"git am\" reading from stdin, waiting, hasn't moved HEAD yet at\nall, I think two things need to happen to fix that:\n\n (1) at around the call to check_patch_format and split_patches,\n clear ORIG_HEAD (this may have to be done only !$rebasing,\n though).\n\n (2) safe_to_abort() should make sure ORIG_HEAD exists; otherwise it\n is unsafe.\n\nBut that is entirely an independent issue (I am going to agree with\nyou in the end).\n\n> blowing away the top commit in the scenario you outlined.\n>\n> This happens because that codepath incorrectly believes that an am is\n> \"in progress\". What this means is that it believes that some of the\n> am code actually got executed in the previous run, setting ORIG_HEAD\n> etc. In your scenario\n>\n> % git am\n> ^C\n>\n> nothing is executed, and a stray directory is left behind.\n\nThat is a correct observation. But it needed a bit of thinking to\nreach your conclusion that special casing this and handling --abort\nin a new different codepath is the right solution.\n\n> If anything, I think the check for $dotest/{next,last} has made the\n> code more robust by correctly verifying that some code did get\n> executed, and that an am is indeed in progress. The bug you have\n> outlined is equivalent to:\n>\n> % mkdir .git/rebase-apply\n> % git am --abort\n\nYes. Or a previous \"git am\" run lost \"$dotest/last\" by a bug and\nthen the user asked to \"am --abort\". Either case, the best you can\ndo is probably to blow away .git/rebase-apply directory.\n\nHow would \"am --skip\", \"am --resolved\", or \"am anothermbox\" behave\nin this \"we already have $dotest because the user started one\nsession but killed it\" case, which used to be covered by -d $dotest\nalone but now flows to the other side of the if/else/fi codepath?\nDo they need a similar treatment, or would they naturally error out\nas they should?\n"},{"id":"220767","messageId":"CALkWK0kO=KY4=Y7ex47LJ2F0oEfF+R9g3REO7rddDLL4c8CuEw@mail.gmail.com","threadId":"34126","inReplyTo":"7vmwqtua5r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] am: handle stray $dotest directory case","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-13T19:07:27Z","receivedAt":"2013-06-13T19:07:27Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Hmph, when did ORIG_HEAD set, and what commit does it point at?\n\nBy some unrelated previous operation (eg. pull, rebase, merge). The\npoint is that at any point in \"normal operation\", ORIG_HEAD exists,\nand usually points to @~N, for some N. If I rm .git/ORIG_HEAD by\nhand, the am --abort obviously errors out:\n\n $ git am --abort\n fatal: Not a valid object name ORIG_HEAD\n fatal: ambiguous argument 'ORIG_HEAD': unknown revision or path not\nin the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\n> As \"git am\" reading from stdin, waiting, hasn't moved HEAD yet at\n> all, I think two things need to happen to fix that:\n>\n> (1) at around the call to check_patch_format and split_patches,\n> clear ORIG_HEAD (this may have to be done only !$rebasing,\n> though).\n>\n> (2) safe_to_abort() should make sure ORIG_HEAD exists; otherwise it\n> is unsafe.\n>\n> But that is entirely an independent issue (I am going to agree with\n> you in the end).\n\nExactly. It might be nice to fix those two things (are there any\nobserved bugs?), but it is entirely orthogonal to our issue.\n\n> That is a correct observation. But it needed a bit of thinking to\n> reach your conclusion that special casing this and handling --abort\n> in a new different codepath is the right solution.\n\nYeah, the commit message is lacking.\n\n> How would \"am --skip\", \"am --resolved\", or \"am anothermbox\" behave\n> in this \"we already have $dotest because the user started one\n> session but killed it\" case, which used to be covered by -d $dotest\n> alone but now flows to the other side of the if/else/fi codepath?\n> Do they need a similar treatment, or would they naturally error out\n> as they should?\n\nam --skip and am --resolved will error out, saying \"Resolve operation\nnot in progress, we are not resuming\". The message needs to be\ntweaked.\n\nam anothermbox will work just fine, implicitly overwriting the\nexisting $dotest directory. But yeah, we could explicitly remove the\ndirectory and allow the mkdir -p to re-create it.\n\nI'll probably work on these follow-ups in the morning. Thanks for poking.\n"},{"id":"220775","messageId":"7vtxl1sk25.fsf@alter.siamese.dyndns.org","threadId":"34126","inReplyTo":"CALkWK0kO=KY4=Y7ex47LJ2F0oEfF+R9g3REO7rddDLL4c8CuEw@mail.gmail.com","subject":"Re: [PATCH] am: handle stray $dotest directory case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-13T22:49:22Z","receivedAt":"2013-06-13T22:49:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n>> But that is entirely an independent issue (I am going to agree with\n>> you in the end).\n>\n> Exactly. It might be nice to fix those two things (are there any\n> observed bugs?), but it is entirely orthogonal to our issue.\n\nOK.\n\n>> That is a correct observation. But it needed a bit of thinking to\n>> reach your conclusion that special casing this and handling --abort\n>> in a new different codepath is the right solution.\n>\n> Yeah, the commit message is lacking.\n\nOK. I'll queue this on 'pu', not in 'next' so that it can be\nrerolled.\n\n> I'll probably work on these follow-ups in the morning. Thanks for poking.\n\nThanks.\n"}]}