[PATCH] merge: Honor prepare-commit-msg return code

[PATCH] merge: Honor prepare-commit-msg return code

[Antoine Pelisse]

prepare-commit-msg hook is run when committing to prepare the log
message. If the exit-status is non-zero, the commit should be aborted.

While the script is run before committing a successful merge, the
exit-status is ignored and a non-zero exit doesn't abort the commit.

Abort the commit if prepare-commit-msg returns with a non-zero status
when committing a successful merge.

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 builtin/merge.c                    |  5 +++--
 t/t7505-prepare-commit-msg-hook.sh | 13 +++++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index a96e8ea..3a31c4b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -800,8 +800,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	if (0 < option_edit)
 		strbuf_add_lines(&msg, "# ", comment, strlen(comment));
 	write_merge_msg(&msg);
-	run_hook(get_index_file(), "prepare-commit-msg",
-		 git_path("MERGE_MSG"), "merge", NULL, NULL);
+	if (run_hook(get_index_file(), "prepare-commit-msg",
+		     git_path("MERGE_MSG"), "merge", NULL, NULL))
+		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {
 		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
 			abort_commit(remoteheads, NULL);
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 5b4b694..bc497bc 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -167,5 +167,18 @@ test_expect_success 'with failing hook (--no-verify)' '
 
 '
 
+test_expect_success 'with failing hook (merge)' '
+
+	git checkout -B other HEAD@{1} &&
+	echo "more" >> file &&
+	git add file &&
+	chmod -x $HOOK &&
+	git commit -m other &&
+	chmod +x $HOOK &&
+	git checkout - &&
+	head=`git rev-parse HEAD` &&
+	test_must_fail git merge other
+
+'
 
 test_done
-- 
1.8.1.rc3.27.g3b73c7d.dirty

Re: [PATCH] merge: Honor prepare-commit-msg return code

[Antoine Pelisse]

On Wed, Jan 2, 2013 at 7:42 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
> prepare-commit-msg hook is run when committing to prepare the log
> message. If the exit-status is non-zero, the commit should be aborted.
>
> While the script is run before committing a successful merge, the
> exit-status is ignored and a non-zero exit doesn't abort the commit.
>
> Abort the commit if prepare-commit-msg returns with a non-zero status
> when committing a successful merge.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
>  builtin/merge.c                    |  5 +++--
>  t/t7505-prepare-commit-msg-hook.sh | 13 +++++++++++++
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a96e8ea..3a31c4b 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -800,8 +800,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>         if (0 < option_edit)
>                 strbuf_add_lines(&msg, "# ", comment, strlen(comment));
>         write_merge_msg(&msg);
> -       run_hook(get_index_file(), "prepare-commit-msg",
> -                git_path("MERGE_MSG"), "merge", NULL, NULL);
> +       if (run_hook(get_index_file(), "prepare-commit-msg",
> +                    git_path("MERGE_MSG"), "merge", NULL, NULL))
> +               abort_commit(remoteheads, NULL);
>         if (0 < option_edit) {
>                 if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
>                         abort_commit(remoteheads, NULL);
> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
> index 5b4b694..bc497bc 100755
> --- a/t/t7505-prepare-commit-msg-hook.sh
> +++ b/t/t7505-prepare-commit-msg-hook.sh
> @@ -167,5 +167,18 @@ test_expect_success 'with failing hook (--no-verify)' '
>
>  '
>
> +test_expect_success 'with failing hook (merge)' '
> +
> +       git checkout -B other HEAD@{1} &&
> +       echo "more" >> file &&
> +       git add file &&
> +       chmod -x $HOOK &&
> +       git commit -m other &&
> +       chmod +x $HOOK &&
> +       git checkout - &&
> +       head=`git rev-parse HEAD` &&

The line above is useless ... Sorry about the noise.

> +       test_must_fail git merge other
> +
> +'
>
>  test_done
> --
> 1.8.1.rc3.27.g3b73c7d.dirty
>

Re: [PATCH] merge: Honor prepare-commit-msg return code

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

> prepare-commit-msg hook is run when committing to prepare the log
> message. If the exit-status is non-zero, the commit should be aborted.

I was scratching my head why you CC'ed Jay, until I dug up 65969d4
(merge: honor prepare-commit-msg hook, 2011-02-14).

> +test_expect_success 'with failing hook (merge)' '
> +
> +	git checkout -B other HEAD@{1} &&
> +	echo "more" >> file &&
> +	git add file &&
> +	chmod -x $HOOK &&

I have a feeling that this will break folks without POSIXPERM
prerequisite.

How about doing it this way instead?  This old test script seems to
want a lot of style clean-ups, but I refrained from doing any in
this fixlet.

Thanks.

 t/t7505-prepare-commit-msg-hook.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index bc497bc..3573751 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -172,11 +172,12 @@ test_expect_success 'with failing hook (merge)' '
 	git checkout -B other HEAD@{1} &&
 	echo "more" >> file &&
 	git add file &&
-	chmod -x $HOOK &&
+	rm -f "$HOOK" &&
 	git commit -m other &&
-	chmod +x $HOOK &&
+	write_script "$HOOK" <<-EOF
+	exit 1
+	EOF
 	git checkout - &&
-	head=`git rev-parse HEAD` &&
 	test_must_fail git merge other
 
 '

Re: [PATCH] merge: Honor prepare-commit-msg return code

[Antoine Pelisse]

>> prepare-commit-msg hook is run when committing to prepare the log
>> message. If the exit-status is non-zero, the commit should be aborted.
>
> I was scratching my head why you CC'ed Jay, until I dug up 65969d4
> (merge: honor prepare-commit-msg hook, 2011-02-14).

I did as suggested in "SubmittingPatches" :)

>> +test_expect_success 'with failing hook (merge)' '
>> +
>> +     git checkout -B other HEAD@{1} &&
>> +     echo "more" >> file &&
>> +     git add file &&
>> +     chmod -x $HOOK &&
>
> I have a feeling that this will break folks without POSIXPERM
> prerequisite.

It felt wrong when I did it, but kept it consistent within the file.
It indeed looks older than other test files I've seen so far but I
don't feel like I know the test general-style-and-policy enough to fix
it myself either.

> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
> index bc497bc..3573751 100755
> --- a/t/t7505-prepare-commit-msg-hook.sh
> +++ b/t/t7505-prepare-commit-msg-hook.sh
> @@ -172,11 +172,12 @@ test_expect_success 'with failing hook (merge)' '
>         git checkout -B other HEAD@{1} &&
>         echo "more" >> file &&
>         git add file &&
> -       chmod -x $HOOK &&
> +       rm -f "$HOOK" &&
>         git commit -m other &&
> -       chmod +x $HOOK &&
> +       write_script "$HOOK" <<-EOF
> +       exit 1
> +       EOF
>         git checkout - &&
> -       head=`git rev-parse HEAD` &&
>         test_must_fail git merge other
>
>  '

What about moving the hook file then ? Not very important to me, just
a suggestion as it would keep the shebang.

Cheers,

Re: [PATCH] merge: Honor prepare-commit-msg return code

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

>>> prepare-commit-msg hook is run when committing to prepare the log
>>> message. If the exit-status is non-zero, the commit should be aborted.
>>
>> I was scratching my head why you CC'ed Jay, until I dug up 65969d4
>> (merge: honor prepare-commit-msg hook, 2011-02-14).
>
> I did as suggested in "SubmittingPatches" :)

Oh, that wasn't meant as a complaint.  I am tempted to rewrite the
log message like so, though:

    65969d4 (merge: honor prepare-commit-msg hook, 2011-02-14) tried to
    make "git commit" and "git merge" consistent, because a merge that
    required user assistance has to be concluded with "git commit", but
    only "git commit" triggered prepare-commit-msg hook.  When it added
    a call to run the prepare-commit-msg hook, however, it forgot to
    check the exit code from the hook like "git commit" does, and ended
    up replacing one inconsistency with another.

>> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
>> index bc497bc..3573751 100755
>> --- a/t/t7505-prepare-commit-msg-hook.sh
>> +++ b/t/t7505-prepare-commit-msg-hook.sh
>> @@ -172,11 +172,12 @@ test_expect_success 'with failing hook (merge)' '
>>         git checkout -B other HEAD@{1} &&
>>         echo "more" >> file &&
>>         git add file &&
>> -       chmod -x $HOOK &&
>> +       rm -f "$HOOK" &&
>>         git commit -m other &&
>> -       chmod +x $HOOK &&
>> +       write_script "$HOOK" <<-EOF
>> +       exit 1
>> +       EOF
>>         git checkout - &&
>> -       head=`git rev-parse HEAD` &&
>>         test_must_fail git merge other
>>
>>  '
>
> What about moving the hook file then ? Not very important to me, just
> a suggestion as it would keep the shebang.

Strictly speaking, the way $HOOK is prepared in the original is
wrong.  The script is always run under "#!/bin/sh" instead of the
shell the user told us to use with $SHELL_PATH.  For a simple one
liner that only exits with 1, it does not matter, though.

Many test scripts got this wrong and that was the reason we later
added write_script helper function to the test suite.

Re: [PATCH] merge: Honor prepare-commit-msg return code

[Antoine Pelisse]

> Oh, that wasn't meant as a complaint.  I am tempted to rewrite the
> log message like so, though:
>
>     65969d4 (merge: honor prepare-commit-msg hook, 2011-02-14) tried to
>     make "git commit" and "git merge" consistent, because a merge that
>     required user assistance has to be concluded with "git commit", but
>     only "git commit" triggered prepare-commit-msg hook.  When it added
>     a call to run the prepare-commit-msg hook, however, it forgot to
>     check the exit code from the hook like "git commit" does, and ended
>     up replacing one inconsistency with another.

That's fine with me

>>> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
>>> index bc497bc..3573751 100755
>>> --- a/t/t7505-prepare-commit-msg-hook.sh
>>> +++ b/t/t7505-prepare-commit-msg-hook.sh
>>> @@ -172,11 +172,12 @@ test_expect_success 'with failing hook (merge)' '
>>>         git checkout -B other HEAD@{1} &&
>>>         echo "more" >> file &&
>>>         git add file &&
>>> -       chmod -x $HOOK &&
>>> +       rm -f "$HOOK" &&
>>>         git commit -m other &&
>>> -       chmod +x $HOOK &&
>>> +       write_script "$HOOK" <<-EOF
>>> +       exit 1
>>> +       EOF
>>>         git checkout - &&
>>> -       head=`git rev-parse HEAD` &&
>>>         test_must_fail git merge other
>>>
>>>  '
>>
>> What about moving the hook file then ? Not very important to me, just
>> a suggestion as it would keep the shebang.
>
> Strictly speaking, the way $HOOK is prepared in the original is
> wrong.  The script is always run under "#!/bin/sh" instead of the
> shell the user told us to use with $SHELL_PATH.  For a simple one
> liner that only exits with 1, it does not matter, though.
>
> Many test scripts got this wrong and that was the reason we later
> added write_script helper function to the test suite.

So let's keep your suggestion and squash the commit.

Thanks,