Stefan Beller <sbel...@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 <sbel...@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.
