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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 5, 2017, 21:38 UTC
Message-ID: <xmqqd174bzco.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170905210116.26343-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 6 quoted lines

> 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.

Show 23 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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", &overwrite_ignore, N_("update ignored files (default)")),
>  	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
> +	OPT_BOOL(0, "no-verify", &no_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.