# [PATCH] merge: Honor prepare-commit-msg return code 6 messages from 2013-01-02 to 2013-01-03. Participants: Antoine Pelisse, Junio C Hamano. Thread: https://gitlist.dev/t/32511 ## Antoine Pelisse, 2013-01-02 18:42 Subject: [PATCH] merge: Honor prepare-commit-msg return code Message-ID: <1357152170-5511-1-git-send-email-apelisse@gmail.com> URL: https://gitlist.dev/e/1357152170-5511-1-git-send-email-apelisse%40gmail.com ``` 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 --- 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 ``` ## Antoine Pelisse, 2013-01-02 19:05 Subject: Re: [PATCH] merge: Honor prepare-commit-msg return code Message-ID: URL: https://gitlist.dev/e/CALWbr2wCe4riywfZ6X3JhsHPvZZQJn8bkdtOakCQK%2BzJr-5iew%40mail.gmail.com In-Reply-To: <1357152170-5511-1-git-send-email-apelisse@gmail.com> ``` On Wed, Jan 2, 2013 at 7:42 PM, Antoine Pelisse 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 > --- > 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 > ``` ## Junio C Hamano, 2013-01-02 20:40 Subject: Re: [PATCH] merge: Honor prepare-commit-msg return code Message-ID: <7v623f2uam.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v623f2uam.fsf%40alter.siamese.dyndns.org In-Reply-To: <1357152170-5511-1-git-send-email-apelisse@gmail.com> ``` Antoine Pelisse 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 ' ``` ## Antoine Pelisse, 2013-01-02 21:02 Subject: Re: [PATCH] merge: Honor prepare-commit-msg return code Message-ID: URL: https://gitlist.dev/e/CALWbr2wWjwUnHFq1icMRuW%3DvjQDhTO1e_chffqUvDWY5za1Kiw%40mail.gmail.com In-Reply-To: <7v623f2uam.fsf@alter.siamese.dyndns.org> ``` >> 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, ``` ## Junio C Hamano, 2013-01-02 21:21 Subject: Re: [PATCH] merge: Honor prepare-commit-msg return code Message-ID: <7vwqvv1dta.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwqvv1dta.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Antoine Pelisse 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. ``` ## Antoine Pelisse, 2013-01-03 17:33 Subject: Re: [PATCH] merge: Honor prepare-commit-msg return code Message-ID: URL: https://gitlist.dev/e/CALWbr2zAWrGBSvMFOYMzhFnPejv6_uU3LH6dfOwimXs_1v1eMg%40mail.gmail.com In-Reply-To: <7vwqvv1dta.fsf@alter.siamese.dyndns.org> ``` > 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, ```