Re: [PATCH] builtin/merge: honor commit-msg hook for merges

[Junio C Hamano]

Stefan Beller  writes:

> Similar to 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14)
> merge should also honor the commit-msg hook; the reason is the same as
> in that commit: When a merge is stopped due to conflicts or --no-commit,
> the subsequent commit calls the commit-msg hook.  However, it is not
> called after a clean merge. Fix this inconsistency by invoking the hook
> after clean merges as well.

The above reads better without "; the reason is the same as in that
commit"---"Similar to", combined with the clean and concise
explanation after the colon you have, sufficiently justifies why
this is a good change.  

Excellent job spotting the precedent and making it consistent ;-).

> This change is motivated by Gerrits commit-msg hook to install a change-id

s/Gerrits/Gerrit's/ perhaps?

> trailer into the commit message. Without such a change id, Gerrit refuses

I do not live in Gerrit land and I do not know which one is the more
preferred one, but be consistent between "change-id" and "change
id".

> to accept (merge) commits by default, such that the inconsistency of
> (not) running commit-msg hook between commit and merge leads to confusion
> and might block people from getting their work done.

Yup.  Nicely explained.

I didn't check how "merge --continue" is implemented, but we need to
behave exactly the same way over there, too.  Making sure that it is
a case in t7504 may be a good idea, in addition to the test you
added.

> Signed-off-by: Stefan Beller 
> ---
>  builtin/merge.c|  8 
>  t/t7504-commit-msg-hook.sh | 45 +
>  2 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 7df3fe3927..087efd560d 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -73,6 +73,7 @@ static int show_progress = -1;
>  static int default_to_upstream = 1;
>  static int signoff;
>  static const char *sign_commit;
> +static int no_verify;
>  
>  static struct strategy all_strategy[] = {
>   { "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
> @@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {
> N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>   OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
> files (default)")),
>   OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
> + OPT_BOOL(0, "no-verify", _verify, N_("bypass pre-commit and 
> commit-msg hooks")),

This allows "--no-no-verify", which may want to be eventually
addressed (either by changing the code not to accept, or declaring
that it is an intended behaviour); I do not offhand know for sure but I
strong suspect "commit" shares the same issue, in which case this
patch is perfectly fine and addressing "--no-no-verify" should be
done for both of them in a separate follow-up topic.  #leftoverbits

Thanks.  I'll be online starting today, but please expect slow
responses for a few days as there is significant backlog.

[PATCH] builtin/merge: honor commit-msg hook for merges

[Stefan Beller]

Similar to 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14)
merge should also honor the commit-msg hook; the reason is the same as
in that commit: When a merge is stopped due to conflicts or --no-commit,
the subsequent commit calls the commit-msg hook.  However, it is not
called after a clean merge. Fix this inconsistency by invoking the hook
after clean merges as well.

This change is motivated by Gerrits commit-msg hook to install a change-id
trailer into the commit message. Without such a change id, Gerrit refuses
to accept (merge) commits by default, such that the inconsistency of
(not) running commit-msg hook between commit and merge leads to confusion
and might block people from getting their work done.

Signed-off-by: Stefan Beller 
---
 builtin/merge.c|  8 
 t/t7504-commit-msg-hook.sh | 45 +
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 7df3fe3927..087efd560d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -73,6 +73,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
+static int no_verify;
 
 static struct strategy all_strategy[] = {
{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {
  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
files (default)")),
OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
+   OPT_BOOL(0, "no-verify", _verify, N_("bypass pre-commit and 
commit-msg hooks")),
OPT_END()
 };
 
@@ -780,6 +782,12 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
if (launch_editor(git_path_merge_msg(), NULL, NULL))
abort_commit(remoteheads, NULL);
}
+
+   if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
+ "commit-msg",
+ git_path_merge_msg(), NULL))
+   abort_commit(remoteheads, NULL);
+
read_merge_msg();
strbuf_stripspace(, 0 < option_edit);
if (!msg.len)
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 88d4cda299..1cd54af3cc 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -101,6 +101,10 @@ cat > "$HOOK" <> file &&
@@ -135,6 +139,32 @@ test_expect_success '--no-verify with failing hook 
(editor)' '
 
 '
 
+test_expect_success 'merge fails with failing hook' '
+
+   test_when_finished "git branch -D newbranch" &&
+   test_when_finished "git checkout -f master" &&
+   git checkout --orphan newbranch &&
+   : >file2 &&
+   git add file2 &&
+   git commit --no-verify file2 -m in-side-branch &&
+   test_must_fail git merge --allow-unrelated-histories master &&
+   commit_msg_is "in-side-branch" # HEAD before merge
+
+'
+
+test_expect_success 'merge bypasses failing hook with --no-verify' '
+
+   test_when_finished "git branch -D newbranch" &&
+   test_when_finished "git checkout -f master" &&
+   git checkout --orphan newbranch &&
+   : >file2 &&
+   git add file2 &&
+   git commit --no-verify file2 -m in-side-branch &&
+   git merge --no-verify --allow-unrelated-histories master &&
+   commit_msg_is "Merge branch '\''master'\'' into newbranch"
+'
+
+
 chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '
 
@@ -178,10 +208,6 @@ exit 0
 EOF
 chmod +x "$HOOK"
 
-commit_msg_is () {
-   test "$(git log --pretty=format:%s%b -1)" = "$1"
-}
-
 test_expect_success 'hook edits commit message' '
 
echo "additional" >> file &&
@@ -217,7 +243,17 @@ test_expect_success "hook doesn't edit commit message 
(editor)" '
echo "more plus" > FAKE_MSG &&
GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit --no-verify &&
commit_msg_is "more plus"
+'
 
+test_expect_success 'hook called in git-merge picks up commit message' '
+   test_when_finished "git branch -D newbranch" &&
+   test_when_finished "git checkout -f master" &&
+   git checkout --orphan newbranch &&
+   : >file2 &&
+   git add file2 &&
+   git commit --no-verify file2 -m in-side-branch &&
+   git merge --allow-unrelated-histories master &&
+   commit_msg_is "new message"
 '
 
 # set up fake editor to replace `pick` by `reword`
@@ -237,4 +273,5 @@ test_expect_success 'hook is called for reword during 
`rebase -i`' '
 
 '
 
+
 test_done
-- 
2.14.0.rc0.3.g6c2e499285