git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
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.

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