{"thread":{"id":"32511","subject":"[PATCH] merge: Honor prepare-commit-msg return code","startedAt":"2013-01-02T18:42:50Z","lastAt":"2013-01-03T17:33:29Z","messageCount":6,"participants":["Antoine Pelisse","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"205873","messageId":"1357152170-5511-1-git-send-email-apelisse@gmail.com","threadId":"32511","inReplyTo":null,"subject":"[PATCH] merge: Honor prepare-commit-msg return code","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-02T18:42:50Z","receivedAt":"2013-01-02T18:42:50Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"prepare-commit-msg hook is run when committing to prepare the log\nmessage. If the exit-status is non-zero, the commit should be aborted.\n\nWhile the script is run before committing a successful merge, the\nexit-status is ignored and a non-zero exit doesn't abort the commit.\n\nAbort the commit if prepare-commit-msg returns with a non-zero status\nwhen committing a successful merge.\n\nSigned-off-by: Antoine Pelisse <apelisse@gmail.com>\n---\n builtin/merge.c                    |  5 +++--\n t/t7505-prepare-commit-msg-hook.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a96e8ea..3a31c4b 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -800,8 +800,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tif (0 < option_edit)\n \t\tstrbuf_add_lines(&msg, \"# \", comment, strlen(comment));\n \twrite_merge_msg(&msg);\n-\trun_hook(get_index_file(), \"prepare-commit-msg\",\n-\t\t git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n+\tif (run_hook(get_index_file(), \"prepare-commit-msg\",\n+\t\t     git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL))\n+\t\tabort_commit(remoteheads, NULL);\n \tif (0 < option_edit) {\n \t\tif (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n \t\t\tabort_commit(remoteheads, NULL);\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 5b4b694..bc497bc 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -167,5 +167,18 @@ test_expect_success 'with failing hook (--no-verify)' '\n \n '\n \n+test_expect_success 'with failing hook (merge)' '\n+\n+\tgit checkout -B other HEAD@{1} &&\n+\techo \"more\" >> file &&\n+\tgit add file &&\n+\tchmod -x $HOOK &&\n+\tgit commit -m other &&\n+\tchmod +x $HOOK &&\n+\tgit checkout - &&\n+\thead=`git rev-parse HEAD` &&\n+\ttest_must_fail git merge other\n+\n+'\n \n test_done\n-- \n1.8.1.rc3.27.g3b73c7d.dirty\n"},{"id":"205874","messageId":"CALWbr2wCe4riywfZ6X3JhsHPvZZQJn8bkdtOakCQK+zJr-5iew@mail.gmail.com","threadId":"32511","inReplyTo":"1357152170-5511-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] merge: Honor prepare-commit-msg return code","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-02T19:05:26Z","receivedAt":"2013-01-02T19:05:26Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Wed, Jan 2, 2013 at 7:42 PM, Antoine Pelisse <apelisse@gmail.com> wrote:\n> prepare-commit-msg hook is run when committing to prepare the log\n> message. If the exit-status is non-zero, the commit should be aborted.\n>\n> While the script is run before committing a successful merge, the\n> exit-status is ignored and a non-zero exit doesn't abort the commit.\n>\n> Abort the commit if prepare-commit-msg returns with a non-zero status\n> when committing a successful merge.\n>\n> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>\n> ---\n>  builtin/merge.c                    |  5 +++--\n>  t/t7505-prepare-commit-msg-hook.sh | 13 +++++++++++++\n>  2 files changed, 16 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index a96e8ea..3a31c4b 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -800,8 +800,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n>         if (0 < option_edit)\n>                 strbuf_add_lines(&msg, \"# \", comment, strlen(comment));\n>         write_merge_msg(&msg);\n> -       run_hook(get_index_file(), \"prepare-commit-msg\",\n> -                git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n> +       if (run_hook(get_index_file(), \"prepare-commit-msg\",\n> +                    git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL))\n> +               abort_commit(remoteheads, NULL);\n>         if (0 < option_edit) {\n>                 if (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n>                         abort_commit(remoteheads, NULL);\n> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n> index 5b4b694..bc497bc 100755\n> --- a/t/t7505-prepare-commit-msg-hook.sh\n> +++ b/t/t7505-prepare-commit-msg-hook.sh\n> @@ -167,5 +167,18 @@ test_expect_success 'with failing hook (--no-verify)' '\n>\n>  '\n>\n> +test_expect_success 'with failing hook (merge)' '\n> +\n> +       git checkout -B other HEAD@{1} &&\n> +       echo \"more\" >> file &&\n> +       git add file &&\n> +       chmod -x $HOOK &&\n> +       git commit -m other &&\n> +       chmod +x $HOOK &&\n> +       git checkout - &&\n> +       head=`git rev-parse HEAD` &&\n\nThe line above is useless ... Sorry about the noise.\n\n> +       test_must_fail git merge other\n> +\n> +'\n>\n>  test_done\n> --\n> 1.8.1.rc3.27.g3b73c7d.dirty\n>\n"},{"id":"205879","messageId":"7v623f2uam.fsf@alter.siamese.dyndns.org","threadId":"32511","inReplyTo":"1357152170-5511-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] merge: Honor prepare-commit-msg return code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-02T20:40:01Z","receivedAt":"2013-01-02T20:40:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse <apelisse@gmail.com> writes:\n\n> prepare-commit-msg hook is run when committing to prepare the log\n> message. If the exit-status is non-zero, the commit should be aborted.\n\nI was scratching my head why you CC'ed Jay, until I dug up 65969d4\n(merge: honor prepare-commit-msg hook, 2011-02-14).\n\n> +test_expect_success 'with failing hook (merge)' '\n> +\n> +\tgit checkout -B other HEAD@{1} &&\n> +\techo \"more\" >> file &&\n> +\tgit add file &&\n> +\tchmod -x $HOOK &&\n\nI have a feeling that this will break folks without POSIXPERM\nprerequisite.\n\nHow about doing it this way instead?  This old test script seems to\nwant a lot of style clean-ups, but I refrained from doing any in\nthis fixlet.\n\nThanks.\n\n t/t7505-prepare-commit-msg-hook.sh | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex bc497bc..3573751 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -172,11 +172,12 @@ test_expect_success 'with failing hook (merge)' '\n \tgit checkout -B other HEAD@{1} &&\n \techo \"more\" >> file &&\n \tgit add file &&\n-\tchmod -x $HOOK &&\n+\trm -f \"$HOOK\" &&\n \tgit commit -m other &&\n-\tchmod +x $HOOK &&\n+\twrite_script \"$HOOK\" <<-EOF\n+\texit 1\n+\tEOF\n \tgit checkout - &&\n-\thead=`git rev-parse HEAD` &&\n \ttest_must_fail git merge other\n \n '\n"},{"id":"205882","messageId":"CALWbr2wWjwUnHFq1icMRuW=vjQDhTO1e_chffqUvDWY5za1Kiw@mail.gmail.com","threadId":"32511","inReplyTo":"7v623f2uam.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] merge: Honor prepare-commit-msg return code","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-02T21:02:30Z","receivedAt":"2013-01-02T21:02:30Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":">> prepare-commit-msg hook is run when committing to prepare the log\n>> message. If the exit-status is non-zero, the commit should be aborted.\n>\n> I was scratching my head why you CC'ed Jay, until I dug up 65969d4\n> (merge: honor prepare-commit-msg hook, 2011-02-14).\n\nI did as suggested in \"SubmittingPatches\" :)\n\n>> +test_expect_success 'with failing hook (merge)' '\n>> +\n>> +     git checkout -B other HEAD@{1} &&\n>> +     echo \"more\" >> file &&\n>> +     git add file &&\n>> +     chmod -x $HOOK &&\n>\n> I have a feeling that this will break folks without POSIXPERM\n> prerequisite.\n\nIt felt wrong when I did it, but kept it consistent within the file.\nIt indeed looks older than other test files I've seen so far but I\ndon't feel like I know the test general-style-and-policy enough to fix\nit myself either.\n\n> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n> index bc497bc..3573751 100755\n> --- a/t/t7505-prepare-commit-msg-hook.sh\n> +++ b/t/t7505-prepare-commit-msg-hook.sh\n> @@ -172,11 +172,12 @@ test_expect_success 'with failing hook (merge)' '\n>         git checkout -B other HEAD@{1} &&\n>         echo \"more\" >> file &&\n>         git add file &&\n> -       chmod -x $HOOK &&\n> +       rm -f \"$HOOK\" &&\n>         git commit -m other &&\n> -       chmod +x $HOOK &&\n> +       write_script \"$HOOK\" <<-EOF\n> +       exit 1\n> +       EOF\n>         git checkout - &&\n> -       head=`git rev-parse HEAD` &&\n>         test_must_fail git merge other\n>\n>  '\n\nWhat about moving the hook file then ? Not very important to me, just\na suggestion as it would keep the shebang.\n\nCheers,\n"},{"id":"205885","messageId":"7vwqvv1dta.fsf@alter.siamese.dyndns.org","threadId":"32511","inReplyTo":"CALWbr2wWjwUnHFq1icMRuW=vjQDhTO1e_chffqUvDWY5za1Kiw@mail.gmail.com","subject":"Re: [PATCH] merge: Honor prepare-commit-msg return code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-02T21:21:21Z","receivedAt":"2013-01-02T21:21:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse <apelisse@gmail.com> writes:\n\n>>> prepare-commit-msg hook is run when committing to prepare the log\n>>> message. If the exit-status is non-zero, the commit should be aborted.\n>>\n>> I was scratching my head why you CC'ed Jay, until I dug up 65969d4\n>> (merge: honor prepare-commit-msg hook, 2011-02-14).\n>\n> I did as suggested in \"SubmittingPatches\" :)\n\nOh, that wasn't meant as a complaint.  I am tempted to rewrite the\nlog message like so, though:\n\n    65969d4 (merge: honor prepare-commit-msg hook, 2011-02-14) tried to\n    make \"git commit\" and \"git merge\" consistent, because a merge that\n    required user assistance has to be concluded with \"git commit\", but\n    only \"git commit\" triggered prepare-commit-msg hook.  When it added\n    a call to run the prepare-commit-msg hook, however, it forgot to\n    check the exit code from the hook like \"git commit\" does, and ended\n    up replacing one inconsistency with another.\n\n>> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n>> index bc497bc..3573751 100755\n>> --- a/t/t7505-prepare-commit-msg-hook.sh\n>> +++ b/t/t7505-prepare-commit-msg-hook.sh\n>> @@ -172,11 +172,12 @@ test_expect_success 'with failing hook (merge)' '\n>>         git checkout -B other HEAD@{1} &&\n>>         echo \"more\" >> file &&\n>>         git add file &&\n>> -       chmod -x $HOOK &&\n>> +       rm -f \"$HOOK\" &&\n>>         git commit -m other &&\n>> -       chmod +x $HOOK &&\n>> +       write_script \"$HOOK\" <<-EOF\n>> +       exit 1\n>> +       EOF\n>>         git checkout - &&\n>> -       head=`git rev-parse HEAD` &&\n>>         test_must_fail git merge other\n>>\n>>  '\n>\n> What about moving the hook file then ? Not very important to me, just\n> a suggestion as it would keep the shebang.\n\nStrictly speaking, the way $HOOK is prepared in the original is\nwrong.  The script is always run under \"#!/bin/sh\" instead of the\nshell the user told us to use with $SHELL_PATH.  For a simple one\nliner that only exits with 1, it does not matter, though.\n\nMany test scripts got this wrong and that was the reason we later\nadded write_script helper function to the test suite.\n"},{"id":"205939","messageId":"CALWbr2zAWrGBSvMFOYMzhFnPejv6_uU3LH6dfOwimXs_1v1eMg@mail.gmail.com","threadId":"32511","inReplyTo":"7vwqvv1dta.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] merge: Honor prepare-commit-msg return code","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-03T17:33:29Z","receivedAt":"2013-01-03T17:33:29Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"> Oh, that wasn't meant as a complaint.  I am tempted to rewrite the\n> log message like so, though:\n>\n>     65969d4 (merge: honor prepare-commit-msg hook, 2011-02-14) tried to\n>     make \"git commit\" and \"git merge\" consistent, because a merge that\n>     required user assistance has to be concluded with \"git commit\", but\n>     only \"git commit\" triggered prepare-commit-msg hook.  When it added\n>     a call to run the prepare-commit-msg hook, however, it forgot to\n>     check the exit code from the hook like \"git commit\" does, and ended\n>     up replacing one inconsistency with another.\n\nThat's fine with me\n\n>>> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n>>> index bc497bc..3573751 100755\n>>> --- a/t/t7505-prepare-commit-msg-hook.sh\n>>> +++ b/t/t7505-prepare-commit-msg-hook.sh\n>>> @@ -172,11 +172,12 @@ test_expect_success 'with failing hook (merge)' '\n>>>         git checkout -B other HEAD@{1} &&\n>>>         echo \"more\" >> file &&\n>>>         git add file &&\n>>> -       chmod -x $HOOK &&\n>>> +       rm -f \"$HOOK\" &&\n>>>         git commit -m other &&\n>>> -       chmod +x $HOOK &&\n>>> +       write_script \"$HOOK\" <<-EOF\n>>> +       exit 1\n>>> +       EOF\n>>>         git checkout - &&\n>>> -       head=`git rev-parse HEAD` &&\n>>>         test_must_fail git merge other\n>>>\n>>>  '\n>>\n>> What about moving the hook file then ? Not very important to me, just\n>> a suggestion as it would keep the shebang.\n>\n> Strictly speaking, the way $HOOK is prepared in the original is\n> wrong.  The script is always run under \"#!/bin/sh\" instead of the\n> shell the user told us to use with $SHELL_PATH.  For a simple one\n> liner that only exits with 1, it does not matter, though.\n>\n> Many test scripts got this wrong and that was the reason we later\n> added write_script helper function to the test suite.\n\nSo let's keep your suggestion and squash the commit.\n\nThanks,\n"}]}