{"thread":{"id":"39409","subject":"[PATCH 2/2] rebase -i: fix post-rewrite hook with failed exec command","startedAt":"2015-05-22T13:15:49Z","lastAt":"2015-06-01T22:17:41Z","messageCount":7,"participants":["Matthieu Moy","Junio C Hamano","Roberto Tyley"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"261901","messageId":"0000014d7bc3f6bf-72bd5f07-9e26-411a-8484-e9b86a1bf429-000000@eu-west-1.amazonses.com","threadId":"39409","inReplyTo":null,"subject":"[PATCH 2/2] rebase -i: fix post-rewrite hook with failed exec command","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-05-22T13:15:49Z","receivedAt":"2015-05-22T13:15:49Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Usually, when 'git rebase' stops before completing the rebase, it is to\ngive the user an opportunity to edit a commit (e.g. with the 'edit'\ncommand). In such cases, 'git rebase' leaves the sha1 of the commit being\nrewritten in \"$state_dir\"/stopped-sha, and subsequent 'git rebase\n--continue' will call the post-rewrite hook with this sha1 as <old-sha1>\nargument to the post-rewrite hook.\n\nThe case of 'git rebase' stopping because of a failed 'exec' command is\ndifferent: it gives the opportunity to the user to examine or fix the\nfailure, but does not stop saying \"here's a commit to edit, use\n--continue when you're done\". So, there's no reason to call the\npost-rewrite hook for 'exec' commands. If the user did rewrite the\ncommit, it would be with 'git commit --amend' which already called the\npost-rewrite hook.\n\nFix the behavior to leave no stopped-sha file in case of failed exec\ncommand, and teach 'git rebase --continue' to skip record_in_rewritten if\nno stopped-sha file is found.\n---\n git-rebase--interactive.sh   | 10 +++++-----\n t/t5407-post-rewrite-hook.sh |  2 +-\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 08e5d86..1c321e4 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -486,7 +486,7 @@ do_pick () {\n }\n \n do_next () {\n-\trm -f \"$msg\" \"$author_script\" \"$amend\" || exit\n+\trm -f \"$msg\" \"$author_script\" \"$amend\" \"$state_dir\"/stopped-sha || exit\n \tread -r command sha1 rest < \"$todo\"\n \tcase \"$command\" in\n \t\"$comment_char\"*|''|noop)\n@@ -576,9 +576,6 @@ do_next () {\n \t\tread -r command rest < \"$todo\"\n \t\tmark_action_done\n \t\tprintf 'Executing: %s\\n' \"$rest\"\n-\t\t# \"exec\" command doesn't take a sha1 in the todo-list.\n-\t\t# => can't just use $sha1 here.\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@@ -874,7 +871,10 @@ first and then run 'git rebase --continue' again.\"\n \t\tfi\n \tfi\n \n-\trecord_in_rewritten \"$(cat \"$state_dir\"/stopped-sha)\"\n+\tif test -r \"$state_dir\"/stopped-sha\n+\tthen\n+\t\trecord_in_rewritten \"$(cat \"$state_dir\"/stopped-sha)\"\n+\tfi\n \n \trequire_clean_work_tree \"rebase\"\n \tdo_rest\ndiff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\nindex 53a4062..06ffad6 100755\n--- a/t/t5407-post-rewrite-hook.sh\n+++ b/t/t5407-post-rewrite-hook.sh\n@@ -212,7 +212,7 @@ EOF\n \tverify_hook_input\n '\n \n-test_expect_failure 'git rebase -i (exec)' '\n+test_expect_success 'git rebase -i (exec)' '\n \tgit reset --hard D &&\n \tclear_hook_input &&\n \tFAKE_LINES=\"edit 1 exec_false 2\" git rebase -i B &&\n\n---\nhttps://github.com/git/git/pull/138"},{"id":"261904","messageId":"0000014d7bc3f7a5-332dd95f-907f-4f46-a5d6-6b9e5dc70b0a-000000@eu-west-1.amazonses.com","threadId":"39409","inReplyTo":"0000014d7bc3f6bf-72bd5f07-9e26-411a-8484-e9b86a1bf429-000000@eu-west-1.amazonses.com","subject":"[PATCH 1/2] rebase -i: demonstrate incorrect behavior of post-rewrite","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-05-22T13:15:50Z","receivedAt":"2015-05-22T13:15:50Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"The 'exec' command is sending the current commit to stopped-sha, which is\nsupposed to contain the original commit (before rebase). As a result, if\nan 'exec' command fails, the next 'git rebase --continue' will send the\ncurrent commit as <old-sha1> to the post-rewrite hook.\n\nThe test currently fails with :\n\n--- expected.data       2015-05-21 17:55:29.000000000 +0000\n+++ [...]post-rewrite.data      2015-05-21 17:55:29.000000000 +0000\n@@ -1,2 +1,3 @@\n 2362ae8e1b1b865e6161e6f0e165ffb974abf018 488028e9fac0b598b70cbeb594258a917e3f6fab\n+488028e9fac0b598b70cbeb594258a917e3f6fab 488028e9fac0b598b70cbeb594258a917e3f6fab\n babc8a4c7470895886fc129f1a015c486d05a351 8edffcc4e69a4e696a1d4bab047df450caf99507\n---\n t/t5407-post-rewrite-hook.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\nindex ea2e0d4..53a4062 100755\n--- a/t/t5407-post-rewrite-hook.sh\n+++ b/t/t5407-post-rewrite-hook.sh\n@@ -212,4 +212,21 @@ EOF\n \tverify_hook_input\n '\n \n+test_expect_failure 'git rebase -i (exec)' '\n+\tgit reset --hard D &&\n+\tclear_hook_input &&\n+\tFAKE_LINES=\"edit 1 exec_false 2\" git rebase -i B &&\n+\techo something >bar &&\n+\tgit add bar &&\n+\t# Fails because of exec false\n+\ttest_must_fail git rebase --continue &&\n+\tgit rebase --continue &&\n+\techo rebase >expected.args &&\n+\tcat >expected.data <<EOF &&\n+$(git rev-parse C) $(git rev-parse HEAD^)\n+$(git rev-parse D) $(git rev-parse HEAD)\n+EOF\n+\tverify_hook_input\n+'\n+\n test_done\n\n\n---\nhttps://github.com/git/git/pull/138"},{"id":"261910","messageId":"xmqq1ti8heu9.fsf@gitster.dls.corp.google.com","threadId":"39409","inReplyTo":"0000014d7bc3f7a5-332dd95f-907f-4f46-a5d6-6b9e5dc70b0a-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH 1/2] rebase -i: demonstrate incorrect behavior of post-rewrite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-22T14:22:22Z","receivedAt":"2015-05-22T14:22:22Z","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> The 'exec' command is sending the current commit to stopped-sha, which is\n> supposed to contain the original commit (before rebase). As a result, if\n> an 'exec' command fails, the next 'git rebase --continue' will send the\n> current commit as <old-sha1> to the post-rewrite hook.\n>\n> The test currently fails with :\n>\n> --- expected.data       2015-05-21 17:55:29.000000000 +0000\n> +++ [...]post-rewrite.data      2015-05-21 17:55:29.000000000 +0000\n> @@ -1,2 +1,3 @@\n>  2362ae8e1b1b865e6161e6f0e165ffb974abf018 488028e9fac0b598b70cbeb594258a917e3f6fab\n> +488028e9fac0b598b70cbeb594258a917e3f6fab 488028e9fac0b598b70cbeb594258a917e3f6fab\n>  babc8a4c7470895886fc129f1a015c486d05a351 8edffcc4e69a4e696a1d4bab047df450caf99507\n\nIndent displayed material like the above a bit, please.\nAnd please sign-off your patches.\n\n> ---\n>  t/t5407-post-rewrite-hook.sh | 17 +++++++++++++++++\n>  1 file changed, 17 insertions(+)\n>\n> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\n> index ea2e0d4..53a4062 100755\n> --- a/t/t5407-post-rewrite-hook.sh\n> +++ b/t/t5407-post-rewrite-hook.sh\n> @@ -212,4 +212,21 @@ EOF\n>  \tverify_hook_input\n>  '\n>  \n> +test_expect_failure 'git rebase -i (exec)' '\n> +\tgit reset --hard D &&\n> +\tclear_hook_input &&\n> +\tFAKE_LINES=\"edit 1 exec_false 2\" git rebase -i B &&\n> +\techo something >bar &&\n> +\tgit add bar &&\n> +\t# Fails because of exec false\n> +\ttest_must_fail git rebase --continue &&\n> +\tgit rebase --continue &&\n> +\techo rebase >expected.args &&\n> +\tcat >expected.data <<EOF &&\n> +$(git rev-parse C) $(git rev-parse HEAD^)\n> +$(git rev-parse D) $(git rev-parse HEAD)\n> +EOF\n\nBy using a dash to start the here-document like this:\n\n\tcat >expect <<-\\EOF &&\n\t$(git rev-parse C) $(git rev-parse HEAD^)\n        ...\n        EOF\n\nyou can tab-indent the contents and the end marker at the same level\nto make it easier to read.\n\n> +\tverify_hook_input\n> +'\n> +\n>  test_done\n>\n>\n> ---\n> https://github.com/git/git/pull/138\n"},{"id":"261916","messageId":"vpqd21soead.fsf@anie.imag.fr","threadId":"39409","inReplyTo":"xmqq1ti8heu9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] rebase -i: demonstrate incorrect behavior of post-rewrite","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-05-22T14:52:26Z","receivedAt":"2015-05-22T14:52: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> Matthieu Moy <Matthieu.Moy@imag.fr> writes:\n>\n>> The 'exec' command is sending the current commit to stopped-sha, which is\n>> supposed to contain the original commit (before rebase). As a result, if\n>> an 'exec' command fails, the next 'git rebase --continue' will send the\n>> current commit as <old-sha1> to the post-rewrite hook.\n>>\n>> The test currently fails with :\n>>\n>> --- expected.data       2015-05-21 17:55:29.000000000 +0000\n>> +++ [...]post-rewrite.data      2015-05-21 17:55:29.000000000 +0000\n>> @@ -1,2 +1,3 @@\n>>  2362ae8e1b1b865e6161e6f0e165ffb974abf018 488028e9fac0b598b70cbeb594258a917e3f6fab\n>> +488028e9fac0b598b70cbeb594258a917e3f6fab 488028e9fac0b598b70cbeb594258a917e3f6fab\n>>  babc8a4c7470895886fc129f1a015c486d05a351 8edffcc4e69a4e696a1d4bab047df450caf99507\n>\n> Indent displayed material like the above a bit, please.\n\nOK, will do.\n\n> And please sign-off your patches.\n\nAh, I was testing submitGit, and forgot that send-email was usually\ndoing this for me.\n\n>> +\tcat >expected.data <<EOF &&\n>> +$(git rev-parse C) $(git rev-parse HEAD^)\n>> +$(git rev-parse D) $(git rev-parse HEAD)\n>> +EOF\n>\n> By using a dash to start the here-document like this:\n>\n> \tcat >expect <<-\\EOF &&\n> \t$(git rev-parse C) $(git rev-parse HEAD^)\n>         ...\n>         EOF\n>\n> you can tab-indent the contents and the end marker at the same level\n> to make it easier to read.\n\nI usually do that but I just mimicked the surrounding code for\nconsistency. If you really prefer the <<-\\EOF I can resend with an\nadditional \"modernize style\" patch before and this one properly\nformatted.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"261919","messageId":"xmqqh9r4fwgc.fsf@gitster.dls.corp.google.com","threadId":"39409","inReplyTo":"xmqq1ti8heu9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] rebase -i: demonstrate incorrect behavior of post-rewrite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-22T15:44:51Z","receivedAt":"2015-05-22T15:44:51Z","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>> +\tcat >expected.data <<EOF &&\n>> +$(git rev-parse C) $(git rev-parse HEAD^)\n>> +$(git rev-parse D) $(git rev-parse HEAD)\n>> +EOF\n>\n> By using a dash to start the here-document like this:\n> ...\n\nSorry, I should have checked, as I know you know that <<-EOF thing.\nYour patch is done this way to be consistent with existing ones.\n\nI'll do a separate patch to clean them all up on top.\n\nThanks.\n\n-- >8 --\nSubject: [PATCH] t5407: use <<- to align the expected output\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t5407-post-rewrite-hook.sh | 80 ++++++++++++++++++++++----------------------\n 1 file changed, 40 insertions(+), 40 deletions(-)\n\ndiff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\nindex 06ffad6..7a48236 100755\n--- a/t/t5407-post-rewrite-hook.sh\n+++ b/t/t5407-post-rewrite-hook.sh\n@@ -61,10 +61,10 @@ test_expect_success 'git rebase' '\n \tgit add foo &&\n \tgit rebase --continue &&\n \techo rebase >expected.args &&\n-\tcat >expected.data <<EOF &&\n-$(git rev-parse C) $(git rev-parse HEAD^)\n-$(git rev-parse D) $(git rev-parse HEAD)\n-EOF\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse C) $(git rev-parse HEAD^)\n+\t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n \tverify_hook_input\n '\n \n@@ -77,9 +77,9 @@ test_expect_success 'git rebase --skip' '\n \tgit add foo &&\n \tgit rebase --continue &&\n \techo rebase >expected.args &&\n-\tcat >expected.data <<EOF &&\n-$(git rev-parse D) $(git rev-parse HEAD)\n-EOF\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n \tverify_hook_input\n '\n \n@@ -89,9 +89,9 @@ test_expect_success 'git rebase --skip the last one' '\n \ttest_must_fail git rebase --onto D A &&\n \tgit rebase --skip &&\n \techo rebase >expected.args &&\n-\tcat >expected.data <<EOF &&\n-$(git rev-parse E) $(git rev-parse HEAD)\n-EOF\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse E) $(git rev-parse HEAD)\n+\tEOF\n \tverify_hook_input\n '\n \n@@ -103,10 +103,10 @@ test_expect_success 'git rebase -m' '\n \tgit add foo &&\n \tgit rebase --continue &&\n \techo rebase >expected.args &&\n-\tcat >expected.data <<EOF &&\n-$(git rev-parse C) $(git rev-parse HEAD^)\n-$(git rev-parse D) $(git rev-parse HEAD)\n-EOF\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse C) $(git rev-parse HEAD^)\n+\t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n \tverify_hook_input\n '\n \n@@ -119,9 +119,9 @@ test_expect_success 'git rebase -m --skip' '\n \tgit add foo &&\n \tgit rebase --continue &&\n \techo rebase >expected.args &&\n-\tcat >expected.data <<EOF &&\n-$(git rev-parse D) $(git rev-parse HEAD)\n-EOF\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n \tverify_hook_input\n '\n \n@@ -148,10 +148,10 @@ test_expect_success 'git rebase -i (unchanged)' '\n \tgit add foo &&\n \tgit rebase --continue &&\n \techo rebase >expected.args &&\n-\tcat >expected.data <<EOF &&\n-$(git rev-parse C) $(git rev-parse HEAD^)\n-$(git rev-parse D) $(git rev-parse HEAD)\n-EOF\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse C) $(git rev-parse HEAD^)\n+\t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n \tverify_hook_input\n '\n \n@@ -163,9 +163,9 @@ test_expect_success 'git rebase -i (skip)' '\n \tgit add foo &&\n \tgit rebase --continue &&\n \techo rebase >expected.args &&\n-\tcat >expected.data <<EOF &&\n-$(git rev-parse D) $(git rev-parse HEAD)\n-EOF\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n \tverify_hook_input\n '\n \n@@ -177,10 +177,10 @@ test_expect_success 'git rebase -i (squash)' '\n \tgit add foo &&\n \tgit rebase --continue &&\n \techo rebase >expected.args &&\n-\tcat >expected.data <<EOF &&\n-$(git rev-parse C) $(git rev-parse HEAD)\n-$(git rev-parse D) $(git rev-parse HEAD)\n-EOF\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse C) $(git rev-parse HEAD)\n+\t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n \tverify_hook_input\n '\n \n@@ -189,10 +189,10 @@ test_expect_success 'git rebase -i (fixup without conflict)' '\n \tclear_hook_input &&\n \tFAKE_LINES=\"1 fixup 2\" git rebase -i B &&\n \techo rebase >expected.args &&\n-\tcat >expected.data <<EOF &&\n-$(git rev-parse C) $(git rev-parse HEAD)\n-$(git rev-parse D) $(git rev-parse HEAD)\n-EOF\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse C) $(git rev-parse HEAD)\n+\t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n \tverify_hook_input\n '\n \n@@ -205,10 +205,10 @@ test_expect_success 'git rebase -i (double edit)' '\n \tgit add foo &&\n \tgit rebase --continue &&\n \techo rebase >expected.args &&\n-\tcat >expected.data <<EOF &&\n-$(git rev-parse C) $(git rev-parse HEAD^)\n-$(git rev-parse D) $(git rev-parse HEAD)\n-EOF\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse C) $(git rev-parse HEAD^)\n+\t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n \tverify_hook_input\n '\n \n@@ -222,10 +222,10 @@ test_expect_success 'git rebase -i (exec)' '\n \ttest_must_fail git rebase --continue &&\n \tgit rebase --continue &&\n \techo rebase >expected.args &&\n-\tcat >expected.data <<EOF &&\n-$(git rev-parse C) $(git rev-parse HEAD^)\n-$(git rev-parse D) $(git rev-parse HEAD)\n-EOF\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse C) $(git rev-parse HEAD^)\n+\t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n \tverify_hook_input\n '\n \n-- \n2.4.1-439-gcfa393f\n"},{"id":"261920","messageId":"xmqqd21sfvsm.fsf@gitster.dls.corp.google.com","threadId":"39409","inReplyTo":"vpqd21soead.fsf@anie.imag.fr","subject":"Re: [PATCH 1/2] rebase -i: demonstrate incorrect behavior of post-rewrite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-22T15:59:05Z","receivedAt":"2015-05-22T15:59:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n>> And please sign-off your patches.\n>\n> Ah, I was testing submitGit, and forgot that send-email was usually\n> doing this for me.\n\nAh, should have noticed from the message-id.\n\nRoberto, isn't your threading of multi-patch series busted?\n\nWhy is 1/2 a follow-up to 2/2?  Do you have a time-machine ;-)?\n"},{"id":"262655","messageId":"CAFY1edb75T91EMM6v4wWz09HZruTsioVmXxmZYjnGpK+_qshow@mail.gmail.com","threadId":"39409","inReplyTo":"xmqqd21sfvsm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] rebase -i: demonstrate incorrect behavior of post-rewrite","fromName":"Roberto Tyley","fromEmail":"roberto.tyley@gmail.com","sentAt":"2015-06-01T22:17:41Z","receivedAt":"2015-06-01T22:17:41Z","isPatch":true,"sender":{"key":"roberto.tyley@gmail.com","avatar":"https://avatars.githubusercontent.com/u/52038?v=4"},"body":"On 22 May 2015 at 16:59, Junio C Hamano <gitster@pobox.com> wrote:\n> Roberto, isn't your threading of multi-patch series busted?\n>\n> Why is 1/2 a follow-up to 2/2?  Do you have a time-machine ;-)?\n\nOh, embarrassing, I better destroy the time-machine:\n\nhttps://github.com/rtyley/submitgit/pull/5\n\nThis was due to me not realising that the GitHub API returns commit lists for\nPRs in reverse-chronological order... thanks for pointing that out!\n"}]}