From: Junio C Hamano <gitster@pobox•com>
To: Stefan Beller <sbeller@google•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] builtin/merge: honor commit-msg hook for merges
Date: Wed, 06 Sep 2017 06:38:15 +0900 [thread overview]
Message-ID: <xmqqd174bzco.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170905210116.26343-1-sbeller@google.com> (Stefan Beller's message of "Tue, 5 Sep 2017 14:01:16 -0700")
Stefan Beller <sbeller@google•com> 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 <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.
next prev parent reply other threads:[~2017-09-05 21:38 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-05 21:01 [PATCH] builtin/merge: honor commit-msg hook for merges Stefan Beller
2017-09-05 21:38 ` Junio C Hamano [this message]
2017-09-05 23:08 ` [PATCH] parse-options: warn developers on negated options Stefan Beller
2017-09-06 1:52 ` Junio C Hamano
2017-09-06 3:16 ` Junio C Hamano
2017-09-06 21:36 ` Stefan Beller
2017-09-06 23:41 ` Junio C Hamano
2017-09-05 23:29 ` [PATCHv2] builtin/merge: honor commit-msg hook for merges Stefan Beller
2017-09-06 1:57 ` Junio C Hamano
2017-09-06 22:11 ` Stefan Beller
2017-09-06 23:43 ` Junio C Hamano
2017-09-07 22:04 ` [PATCHv3] " Stefan Beller
2017-09-08 1:13 ` Junio C Hamano
2017-09-11 17:12 ` Stefan Beller
2017-09-16 6:22 ` Kaartic Sivaraam
2017-09-20 19:55 ` Stefan Beller
2017-09-21 1:10 ` Junio C Hamano
2017-09-21 20:29 ` [PATCH] Documentation/githooks: mention merge in commit-msg hook Stefan Beller
2017-09-22 1:58 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqd174bzco.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox