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