Web lists-archives.com

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




Stefan Beller <sbeller@xxxxxxxxxx> writes:

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


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

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

Both look sensible.

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