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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2017, 01:13 UTC
Message-ID: <xmqqr2vi0z7q.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170907220429.31312-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 5 quoted lines

> .... The --no-verify option however is not remembered across invocations
> of git-merge. Originally the author assumed an alternative in which the
> 'git merge --continue' command accepts the --no-verify flag, but that
> opens up the discussion which flags are allows to the continued merge
> command and which must be given in the first invocation.

This leaves a reader (me) wondering what the final conclusion was, after the author assumed something and thought about alternatives. I am guessing that your final decision was not to remember "--no-verify" so a user who started "merge --no-verify" that stopped in the middle must say "merge --continue --no-verify" or "commit --no-verify" to conclude the merge? Or you added some mechanism to remember the fact that no-verify was given so that "merge --continue" will read from there, ignoring "merge --continue --verify" from the command line? Not just the above part of the log message confusing, but there is no update to the documentation, and we shouldn't expect end-users to find out what ought to happen by reading t7504 X-<.

The new test in t7504 tells me that you remember --[no-]verify from the initial invocation and use the same when --continue is given; it is unclear how that remembered one interacts with --[no-]verify that is given when --continue is given. It is not documented, tested and explained in the log message. I would expect that the command line trumps what was given in the initial invocation.

Show 11 quoted lines

> +static int verify_msg = 1;
>  
>  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, "verify", &verify_msg, N_("verify commit-msg hook")),
>  	OPT_END()
>  };

I suspect that the previous iteration gives a much better end-user experience when "git merge -h" is used. This will give the impression that the user MUST say "merge --verify" if the user wants to verify commit-msg hook (whatever that means), but because the option defaults to true, that is not what happens. The user instead must say "merge --no-verify" if the verification is unwanted.

"git commit -h" explains

    --no-verify        bypass pre-commit and commit-msg hooks

and I think that is the way how we want to explain this option in "git merge" too. Normally it is not bypassed, and the user can ask with "--no-verify". Thanks to René's change in 2012, the option definition you had in the previous one will make --[no-]verify accepted just fine.

Show 7 quoted lines

> +test_expect_success 'merge fails with failing hook' '
> + ...
> +'
> +
> +test_expect_success 'merge bypasses failing hook with --no-verify' '
> + ...
> +'

Both look sensible.

Show 18 quoted lines

> +test_expect_failure 'merge --continue remembers --no-verify' '
> +	test_when_finished "git branch -D newbranch" &&
> +	test_when_finished "git checkout -f master" &&
> +	git checkout master &&
> +	echo a >file2 &&
> +	git add file2 &&
> +	git commit --no-verify -m "add file2 to master" &&
> +	git checkout -b newbranch master^ &&
> +	echo b >file2 &&
> +	git add file2 &&
> +	git commit --no-verify file2 -m in-side-branch &&
> +	git merge --no-verify -m not-rewritten-by-hook master &&
> +	# resolve conflict:
> +	echo c >file2 &&
> +	git add file2 &&
> +	git merge --continue &&
> +	commit_msg_is not-rewritten-by-hook
>  '

OK. What should happen when the last "merge --continue" was given "--verify" at the same time? A similar test whose title is "--no-verify remembered by merge --continue can be overriden" may be a good thing to follow this one, perhaps?

Thanks.