From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges
Date: Fri, 08 Sep 2017 10:13:29 +0900 [thread overview]
Message-ID: <xmqqr2vi0z7q.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170907220429.31312-1-sbeller@google.com> (Stefan Beller's message of "Thu, 7 Sep 2017 15:04:29 -0700")
Stefan Beller <sbeller@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.
next prev parent reply other threads:[~2017-09-08 1:13 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
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 [this message]
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqr2vi0z7q.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).