{"thread":{"id":"28048","subject":"rebase -i: Should --continue auto-amend after failed exec?","startedAt":"2011-08-08T21:11:55Z","lastAt":"2011-08-25T13:04:27Z","messageCount":9,"participants":["Johannes Sixt","Matthieu Moy","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"173163","messageId":"4E40511B.7090206@kdbg.org","threadId":"28048","inReplyTo":null,"subject":"rebase -i: Should --continue auto-amend after failed exec?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-08-08T21:11:55Z","receivedAt":"2011-08-08T21:11:55Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"If after a failed \"exec\" instruction there are staged changes, then currently rebase\n--continue fails with:\n\n.../git-rebase--interactive: line 774: .../.git/rebase-merge/author-script: No such file or directory\n\nBut shouldn't this amend the HEAD commit? The documentation is not clear\n(from git-rebase.txt):\n\n  The interactive rebase will stop when a command fails (i.e. exits with\n  non-0 status) to give you an opportunity to fix the problem. You can\n  continue with `git rebase --continue`.\n\nThis may be interpreted to work like \"edit\", and IMO would be a very useful\nmodus operandi.\n\nHere is a test case.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 47c8371..2146e47 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -527,6 +527,21 @@ test_expect_success 'auto-amend only edited commits after \"edit\"' '\n \tgit rebase --abort\n '\n \n+test_expect_failure 'auto-amend after failed \"exec\"' '\n+\ttest_tick &&\n+\ttest_when_finished \"git rebase --abort || :\" &&\n+\t(\n+\t\tFAKE_LINES=\"1 exec_false\" &&\n+\t\texport FAKE_LINES &&\n+\t\ttest_must_fail git rebase -i HEAD^\n+\t) &&\n+\techo \"edited again\" > file7 &&\n+\tgit add file7 &&\n+\tFAKE_COMMIT_MESSAGE=\"edited file7 again\" git rebase --continue &&\n+\tactual=$(git show HEAD:file7) &&\n+\ttest \"edited again\" = \"$actual\"\n+'\n+\n test_expect_success 'rebase a detached HEAD' '\n \tgrandparent=$(git rev-parse HEAD~2) &&\n \tgit checkout $(git rev-parse HEAD) &&\n"},{"id":"174151","messageId":"vpqk4a3rkwb.fsf@bauges.imag.fr","threadId":"28048","inReplyTo":"4E40511B.7090206@kdbg.org","subject":"Re: rebase -i: Should --continue auto-amend after failed exec?","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-08-24T13:36:52Z","receivedAt":"2011-08-24T13:36:52Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> If after a failed \"exec\" instruction there are staged changes, then currently rebase\n> --continue fails with:\n>\n> .../git-rebase--interactive: line 774: .../.git/rebase-merge/author-script: No such file or directory\n\nThat's obviously bad, there should at least be an accurate error\nmessage.\n\n> But shouldn't this amend the HEAD commit? The documentation is not clear\n> (from git-rebase.txt):\n>\n>   The interactive rebase will stop when a command fails (i.e. exits with\n>   non-0 status) to give you an opportunity to fix the problem. You can\n>   continue with `git rebase --continue`.\n>\n> This may be interpreted to work like \"edit\", and IMO would be a very useful\n> modus operandi.\n\nI'm not sure. What happens in \"edit\" is that when reaching the \"edit\"\nline, git-rebase--interactive.sh calls die_with_patch, which writes\nauthor information in .git/rebase-merge/author-script, which really\nmeans \"I've stopped on this commit, this is the one that we should\nimplicitely amend with --continue\".\n\nThe case of \"exec\" is a bit different: you don't stop \"on a commit\", but\nafter doing something else. You can hardly guess whether the staged\nchanges are meant to amend the existing commit, or to make a new one.\n\nActually, that could even be\n\npick deadbeef Existing commit\nexec foo > bar.txt; git add bar.txt; git commit -m \"added during rebase\"\nexec false\npick c00ffee Another commit\n\nthen auto-amending may be really confusing: should it amend the HEAD\ncommit that you've just created (this would really go against the logic\nof .git/rebase-merge/author-script) or the last picked commit (which you\ncan't really do since it's not HEAD)?\n\nI think it's best to abort, with an accurate error message pointing the\nuser to both solutions (commit --amend && rebase --continue or commit &&\nrebase --continue). I'll try a patch.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"174153","messageId":"1314194508-12067-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"28048","inReplyTo":"vpqk4a3rkwb.fsf@bauges.imag.fr","subject":"[PATCH] rebase -i: clean error message for --continue after failed exec","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2011-08-24T14:01:48Z","receivedAt":"2011-08-24T14:01:48Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"If after a failed \"exec\" instruction there are staged changes, before\nthis patch, rebase --continue fails with this message, which is obviously\nwrong:\n\n.../git-rebase--interactive: line 774: .../.git/rebase-merge/author-script: No such file or directory\n\nWe could try auto-amending HEAD, but this goes against the logic of\n.git/rebase-merge/author-script (see also the testcase 'auto-amend only\nedited commits after \"edit\"' in t3404-rebase-interactive.sh) to\nauto-amend something the user hasn't explicitely asked to edit.\n\nInstead of doing anything automatically, detect the situation and give a\nclean error message. While we're there, also clarify the error message in\ncase '. \"$author_script\"' fails, which now corresponds to really weird\nsenario where the author script exists but can't be read.\n\nTest-case-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\nHere's my attempt at a patch. After noticing the existance of a test\n'auto-amend only edited commits after \"edit\"', I'm convinced that\nauto-amending is not the right thing here.\n\n git-rebase--interactive.sh    |   18 +++++++++++++++++-\n t/t3404-rebase-interactive.sh |   14 ++++++++++++++\n 2 files changed, 31 insertions(+), 1 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex c6ba7c1..5c94506 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -647,8 +647,24 @@ continue)\n \tthen\n \t\t: Nothing to commit -- skip this\n \telse\n+\t\tif ! test -f \"$author_script\"\n+\t\tthen\n+\t\t\tdie \"You have staged changes in your working tree. If these changes are meant to be\n+squashed into the previous commit, run:\n+\n+  git commit --amend\n+\n+If they are meant to go into a new commit, run:\n+\n+  git commit\n+\n+In both case, once you're done, continue with:\n+\n+  git rebase --continue\n+\"\n+\t\tfi\n \t\t. \"$author_script\" ||\n-\t\t\tdie \"Cannot find the author identity\"\n+\t\t\tdie \"Error trying to find the author identity to amend commit\"\n \t\tcurrent_head=\n \t\tif test -f \"$amend\"\n \t\tthen\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 8538813..b981572 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -527,6 +527,20 @@ test_expect_success 'auto-amend only edited commits after \"edit\"' '\n \tgit rebase --abort\n '\n \n+test_expect_success 'clean error after failed \"exec\"' '\n+\ttest_tick &&\n+\ttest_when_finished \"git rebase --abort || :\" &&\n+\t(\n+\t\tFAKE_LINES=\"1 exec_false\" &&\n+\t\texport FAKE_LINES &&\n+\t\ttest_must_fail git rebase -i HEAD^\n+\t) &&\n+\techo \"edited again\" > file7 &&\n+\tgit add file7 &&\n+\ttest_must_fail git rebase --continue 2>error &&\n+\tgrep \"You have staged changes in your working tree.\" error\n+'\n+\n test_expect_success 'rebase a detached HEAD' '\n \tgrandparent=$(git rev-parse HEAD~2) &&\n \tgit checkout $(git rev-parse HEAD) &&\n-- \n1.7.6.580.gc2862.dirty\n"},{"id":"174175","messageId":"7v62lmps6k.fsf@alter.siamese.dyndns.org","threadId":"28048","inReplyTo":"1314194508-12067-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH] rebase -i: clean error message for --continue after failed exec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-24T18:42:27Z","receivedAt":"2011-08-24T18:42:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n\n> If after a failed \"exec\" instruction there are staged changes,...\n\nI have to wonder why whatever \"exec\" runs is mucking with the index in the\nfirst place. Shouldn't we forbid it?\n"},{"id":"174176","messageId":"7vy5yiod1h.fsf@alter.siamese.dyndns.org","threadId":"28048","inReplyTo":"7v62lmps6k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rebase -i: clean error message for --continue after failed exec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-24T18:54:50Z","receivedAt":"2011-08-24T18:54:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n>\n>> If after a failed \"exec\" instruction there are staged changes,...\n>\n> I have to wonder why whatever \"exec\" runs is mucking with the index in the\n> first place. Shouldn't we forbid it?\n\nI suspect your patch amounts to the same thing of forbidding, but\ndetecting the lack of $author_script feels like it is covering up the\nsymptom and not directly going for the cause of the symptom.\n\nI wonder if doing something like this would be more direct approach to\nachieve the same thing.\n\n git-rebase--interactive.sh |   13 ++++++++++---\n 1 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex c6ba7c1..31026dc 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -472,24 +472,31 @@ do_next () {\n \t\tgit rev-parse --verify HEAD > \"$state_dir\"/stopped-sha\n \t\t${SHELL:-@SHELL_PATH@} -c \"$rest\" # Actual execution\n \t\tstatus=$?\n+\t\t# Run in subshell because require_clean_work_tree can die.\n+\t\tdirty=f\n+\t\t(require_clean_work_tree \"rebase\") || dirty=t\n \t\tif test \"$status\" -ne 0\n \t\tthen\n \t\t\twarn \"Execution failed: $rest\"\n+\t\t\ttest \"$dirty\" = f ||\n+\t\t\twarn \"and made changes to the index and/or the working tree\"\n+\n \t\t\twarn \"You can fix the problem, and then run\"\n \t\t\twarn\n \t\t\twarn \"\tgit rebase --continue\"\n \t\t\twarn\n \t\t\texit \"$status\"\n \t\tfi\n-\t\t# Run in subshell because require_clean_work_tree can die.\n-\t\tif ! (require_clean_work_tree \"rebase\")\n-\t\tthen\n+\t\tif test \"$dirty\" = t\n+\t\t\twarn \"Execution succeeded: $rest\"\n+\t\t\twarn \"but left changes to the index and/or the working tree\"\n \t\t\twarn \"Commit or stash your changes, and then run\"\n \t\t\twarn\n \t\t\twarn \"\tgit rebase --continue\"\n \t\t\twarn\n \t\t\texit 1\n \t\tfi\n+\n \t\t;;\n \t*)\n \t\twarn \"Unknown command: $command $sha1 $rest\"\n"},{"id":"174184","messageId":"20110824202027.GA28900@sigill.intra.peff.net","threadId":"28048","inReplyTo":"7v62lmps6k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rebase -i: clean error message for --continue after failed exec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-24T20:20:27Z","receivedAt":"2011-08-24T20:20:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 24, 2011 at 11:42:27AM -0700, Junio C Hamano wrote:\n\n> Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n> \n> > If after a failed \"exec\" instruction there are staged changes,...\n> \n> I have to wonder why whatever \"exec\" runs is mucking with the index in the\n> first place. Shouldn't we forbid it?\n\nCertainly my only user has ever been \"exec make test\". But I wonder if\nsomebody is crazy enough to auto-generate some content and commit it.\nOTOH, shouldn't it then be their responsibility to make the commit?\nI.e., I can see at least the potential for mucking with the index, but I\nreally don't see a reason for _leaving_ the index in a mucked state.\n\n-Peff\n"},{"id":"174185","messageId":"20110824202233.GB28900@sigill.intra.peff.net","threadId":"28048","inReplyTo":"20110824202027.GA28900@sigill.intra.peff.net","subject":"Re: [PATCH] rebase -i: clean error message for --continue after failed exec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-24T20:22:33Z","receivedAt":"2011-08-24T20:22:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 24, 2011 at 04:20:27PM -0400, Jeff King wrote:\n\n> Certainly my only user has ever been \"exec make test\". But I wonder if\n\nEr, s/user/use.\n\n> somebody is crazy enough to auto-generate some content and commit it.\n> OTOH, shouldn't it then be their responsibility to make the commit?\n> I.e., I can see at least the potential for mucking with the index, but I\n> really don't see a reason for _leaving_ the index in a mucked state.\n\nHaving just read your followup patch, it looks sane. Exec commands are\nfree to do whatever they like with the index as long as it is left in a\nclean state. That keeps the door open for semi-sane use cases, but will\ncatch unintended index manipulation.\n\n-Peff\n"},{"id":"174211","messageId":"vpqzkiydl21.fsf@bauges.imag.fr","threadId":"28048","inReplyTo":"7vy5yiod1h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rebase -i: clean error message for --continue after failed exec","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-08-25T07:09:26Z","receivedAt":"2011-08-25T07:09:26Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n>>\n>>> If after a failed \"exec\" instruction there are staged changes,...\n>>\n>> I have to wonder why whatever \"exec\" runs is mucking with the index in the\n>> first place. Shouldn't we forbid it?\n>\n> I suspect your patch amounts to the same thing of forbidding, but\n> detecting the lack of $author_script feels like it is covering up the\n> symptom and not directly going for the cause of the symptom.\n>\n> I wonder if doing something like this would be more direct approach to\n> achieve the same thing.\n\nNot the same thing, but both patches could go well together.\n\nMine covers\n\n  pick deadbeef\n  exec make test\n  # :-( make test failed, I'm going to fix it\n  hack hack hack\n  git add changes\n  # OK, seems fixed.\n  git rebase --continue\n  # --> rebase tells me I forgot to commit my fixup patch\n\ni.e. the user changed the index interactively, not within exec. Yours\ncovers the case where the command itself changes the index.\n\n> +\t\t# Run in subshell because require_clean_work_tree can die.\n> +\t\tdirty=f\n> +\t\t(require_clean_work_tree \"rebase\") || dirty=t\n\nThis will display error messages like\n\n  Cannot rebase: You have unstaged changes\n\nand right after\n\n>  \t\t\twarn \"Execution failed: $rest\"\n> +\t\t\ttest \"$dirty\" = f ||\n> +\t\t\twarn \"and made changes to the index and/or the working tree\"\n\nwhich sounds redundant. This should probably be\n\n(require_clean_work_tree \"rebase\" 2>/dev/null) || dirty=t\n\nbut looking more closely at the patch, you're not the one introducing\nthis, it was already there since 92c62a3f.\n\n>  \t\tfi\n> +\n>  \t\t;;\n>  \t*)\n\nI think this one can be removed, there's usually no blank line before ;;\nin the code.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"174225","messageId":"4E56485B.10206@kdbg.org","threadId":"28048","inReplyTo":"vpqzkiydl21.fsf@bauges.imag.fr","subject":"Re: [PATCH] rebase -i: clean error message for --continue after failed exec","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-08-25T13:04:27Z","receivedAt":"2011-08-25T13:04:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.08.2011 09:09, schrieb Matthieu Moy:\n> Junio C Hamano<gitster@pobox.com>  writes:\n>> I wonder if doing something like this would be more direct approach to\n>> achieve the same thing.\n>\n> Not the same thing, but both patches could go well together.\n>\n> Mine covers\n>\n>    pick deadbeef\n>    exec make test\n>    # :-( make test failed, I'm going to fix it\n>    hack hack hack\n>    git add changes\n>    # OK, seems fixed.\n>    git rebase --continue\n>    # -->  rebase tells me I forgot to commit my fixup patch\n\nThis is exactly my use-case that discovered the problem and which I would \nlike to see fixed, FWIW.\n\n-- Hannes\n"}]}