Re: [PATCH] Remove negation from the merge option "--no-verify"

Alex Riesen [off-list ref] writes:

From: Alex Riesen <redacted>

This allows re-enabling hooks disabled by an earlier "--no-verify"
in command-line and makes the interface more consistent.

Signed-off-by: Alex Riesen <redacted>

---

This one is on top of "[PATCH] Fix "commit-msg" hook unexpectedly called for
"git pull --no-verify" (http://public-inbox.org/git/YXfwanz3MynCLDmn@pflmari/).
Which is a bit awkward. Should I resend as series?

Don't we need to do this at the root cause command "git commit"?  It
is documented to take "--no-verify" but not "--verify" to countermand
an earlier "--no-verify" on the command line.

And yes, I agree that we shouldn't introduce an awkwardness in one
step of the series and fix it in another step of the same series.

Junio C Hamano, Wed, Oct 27, 2021 22:16:35 +0200:

Alex Riesen [off-list ref] writes:

quoted

From: Alex Riesen <redacted>

This allows re-enabling hooks disabled by an earlier "--no-verify"
in command-line and makes the interface more consistent.

Signed-off-by: Alex Riesen <redacted>

---

This one is on top of "[PATCH] Fix "commit-msg" hook unexpectedly called for
"git pull --no-verify" (http://public-inbox.org/git/YXfwanz3MynCLDmn@pflmari/).
Which is a bit awkward. Should I resend as series?

Don't we need to do this at the root cause command "git commit"?

The commit preparing code in builtin/merge.c does not seem to use the code
from builtin/commit.c, so it does not look like a direct cause of that effect
in "git merge". But...

It is documented to take "--no-verify" but not "--verify" to countermand an
earlier "--no-verify" on the command line.

This particular peculiarity in implementation of "--[no-]verify" does look
like it has root-caused everything :)

And yes, I agree that we shouldn't introduce an awkwardness in one
step of the series and fix it in another step of the same series.

I think I better resend everything as a single patch then.

From: Alex Riesen <redacted>

This allows re-enabling of the hooks disabled by an earlier "--no-verify"
in command-line and makes the interface more consistent.

Incidentally, this also fixes unexpected calling of the hooks by "git
pull" when "--no-verify" was specified, where it was incorrectly
translated to "--no-verify-signatures". This caused the unexpected
effect of the hooks being called. And an even more unexpected effect of
disabling verification of signatures.

Signed-off-by: Alexander Riesen <redacted>
---
Alex Riesen, Thu, Oct 28, 2021 08:38:04 +0200:

Junio C Hamano, Wed, Oct 27, 2021 22:16:35 +0200:

quoted

And yes, I agree that we shouldn't introduce an awkwardness in one
step of the series and fix it in another step of the same series.

I think I better resend everything as a single patch then.

Something like this: dengate no-verify in all commit-creating code.

I looked at "no-verify" in push and rebase, but they feel different:
they create no new commits, and are involved in other workflows.
Perhaps another time.

 Documentation/git-commit.txt    | 10 ++++++++--
 Documentation/git-merge.txt     |  2 +-
 Documentation/merge-options.txt |  5 +++--
 builtin/commit.c                | 22 +++++++++++++++++-----
 builtin/merge.c                 | 12 ++++++------
 builtin/pull.c                  |  6 ++++++
 t/t5521-pull-options.sh         | 12 ++++++++++++
 t/t7504-commit-msg-hook.sh      |  8 ++++++++
 8 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index a3baea32ae..ba66209274 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt

@@ -11,7 +11,7 @@ SYNOPSIS
 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]
 	   [--dry-run] [(-c | -C | --fixup | --squash) <commit>]
 	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
-	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
+	   [--allow-empty-message] [--[no-]verify] [-e] [--author=<author>]
 	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
 	   [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]]
 	   [-S[<keyid>]] [--] [<pathspec>...]

@@ -174,7 +174,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
 
 -n::
 --no-verify::
-	This option bypasses the pre-commit and commit-msg hooks.
+	By default, pre-merge and commit-msg hooks are run. When one of these
+	options is given, these are bypassed.
+	See also linkgit:githooks[5].
+
+--verify::
+	This option re-enables running of the pre-commit and commit-msg hooks
+	after an earlier `-n` or `--no-verify`.
 	See also linkgit:githooks[5].
 
 --allow-empty::

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 3819fadac1..324ae879d2 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt

@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
-	[--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
+	[--[no-]verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
 	[--[no-]allow-unrelated-histories]
 	[--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
 'git merge' (--continue | --abort | --quit)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 80d4831662..f8016b0f7b 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt

@@ -112,8 +112,9 @@ option can be used to override --squash.
 +
 With --squash, --commit is not allowed, and will fail.
 
---no-verify::
-	This option bypasses the pre-merge and commit-msg hooks.
+--[no-]verify::
+	By default, pre-merge and commit-msg hooks are run. When `--no-verify`
+	is given, these are bypassed.
 	See also linkgit:githooks[5].
 
 -s <strategy>::

diff --git a/builtin/commit.c b/builtin/commit.c
index 1dfd799ec5..714722b0cd 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -108,7 +108,7 @@ static char *edit_message, *use_message;
 static char *fixup_message, *squash_message;
 static int all, also, interactive, patch_interactive, only, amend, signoff;
 static int edit_flag = -1; /* unspecified */
-static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
+static int quiet, verbose, verify = 1, allow_empty, dry_run, renew_authorship;
 static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;

@@ -164,6 +164,16 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static int opt_parse_n(const struct option *opt, const char *arg, int unset)
+{
+	int *value = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+
+	*value = 0;
+	return 0;
+}
+
 static int opt_parse_rename_score(const struct option *opt, const char *arg, int unset)
 {
 	const char **value = opt->value;

@@ -699,7 +709,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
+	if (verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
 		return 0;
 
 	if (squash_message) {

@@ -983,7 +993,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (!no_verify && find_hook("pre-commit")) {
+	if (verify && find_hook("pre-commit")) {
 		/*
 		 * Re-read the index as pre-commit hook could have updated it,
 		 * and write it out as a tree.  We must do this before we invoke

@@ -1014,7 +1024,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		strvec_clear(&env);
 	}
 
-	if (!no_verify &&
+	if (verify &&
 	    run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) {
 		return 0;
 	}

@@ -1522,7 +1532,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "interactive", &interactive, N_("interactively add files")),
 		OPT_BOOL('p', "patch", &patch_interactive, N_("interactively add changes")),
 		OPT_BOOL('o', "only", &only, N_("commit only specified files")),
-		OPT_BOOL('n', "no-verify", &no_verify, N_("bypass pre-commit and commit-msg hooks")),
+		OPT_CALLBACK_F('n', NULL, &verify, "", N_("bypass pre-commit and commit-msg hooks"),
+			       PARSE_OPT_NOARG|PARSE_OPT_NONEG, opt_parse_n),
+		OPT_BOOL(0, "verify", &verify, N_("control use of pre-commit and commit-msg hooks")),
 		OPT_BOOL(0, "dry-run", &dry_run, N_("show what would be committed")),
 		OPT_SET_INT(0, "short", &status_format, N_("show status concisely"),
 			    STATUS_FORMAT_SHORT),

diff --git a/builtin/merge.c b/builtin/merge.c
index 9d5359edc2..ab5c221234 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -83,7 +83,7 @@ static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
 static int autostash;
-static int no_verify;
+static int verify = 1;
 
 static struct strategy all_strategy[] = {
 	{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },

@@ -290,7 +290,7 @@ static struct option builtin_merge_options[] = {
 	OPT_AUTOSTASH(&autostash),
 	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, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")),
+	OPT_BOOL(0, "verify", &verify, N_("control use of pre-merge-commit and commit-msg hooks")),
 	OPT_END()
 };
 

@@ -822,7 +822,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	const char *index_file = get_index_file();
 
-	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+	if (verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
 		abort_commit(remoteheads, NULL);
 	/*
 	 * Re-read the index as pre-merge-commit hook could have updated it,

@@ -858,9 +858,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 			abort_commit(remoteheads, NULL);
 	}
 
-	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
-					  "commit-msg",
-					  git_path_merge_msg(the_repository), NULL))
+	if (verify && run_commit_hook(0 < option_edit, get_index_file(),
+				      "commit-msg",
+				      git_path_merge_msg(the_repository), NULL))
 		abort_commit(remoteheads, NULL);
 
 	read_merge_msg(&msg);

diff --git a/builtin/pull.c b/builtin/pull.c
index 425950f469..e783da10b2 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -84,6 +84,7 @@ static char *opt_edit;
 static char *cleanup_arg;
 static char *opt_ff;
 static char *opt_verify_signatures;
+static char *opt_verify;
 static int opt_autostash = -1;
 static int config_autostash;
 static int check_trust_level = 1;

@@ -160,6 +161,9 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
 		N_("abort if fast-forward is not possible"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+	OPT_PASSTHRU(0, "verify", &opt_verify, NULL,
+		N_("control use of pre-merge-commit and commit-msg hooks"),
+		PARSE_OPT_NOARG),
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),

@@ -688,6 +692,8 @@ static int run_merge(void)
 		strvec_pushf(&args, "--cleanup=%s", cleanup_arg);
 	if (opt_ff)
 		strvec_push(&args, opt_ff);
+	if (opt_verify)
+		strvec_push(&args, opt_verify);
 	if (opt_verify_signatures)
 		strvec_push(&args, opt_verify_signatures);
 	strvec_pushv(&args, opt_strategies.v);

diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index db1a381cd9..7d3a8ae0d3 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh

@@ -225,4 +225,16 @@ test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'git pull --no-verify flag passed to merge' '
+	test_when_finished "rm -fr src dst actual" &&
+	git init src &&
+	test_commit -C src one &&
+	git clone src dst &&
+	write_script dst/.git/hooks/commit-msg <<-\EOF &&
+	false
+	EOF
+	test_commit -C src two &&
+	git -C dst pull --no-ff --no-verify
+'
+
 test_done

diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 31b9c6a2c1..166ff5fb26 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh

@@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '
 
 '
 
+test_expect_success '-n with failing hook' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit -n -m "more"
+
+'
+
 test_expect_success '--no-verify with failing hook (editor)' '
 
 	echo "more stuff" >> file &&

-- 
2.33.0.22.g8cd9218530

Hi Alex

On 28/10/2021 09:04, Alex Riesen wrote:

From: Alex Riesen <redacted>

This allows re-enabling of the hooks disabled by an earlier "--no-verify"
in command-line and makes the interface more consistent.

Thanks for working on this. Since 0f1930c587 ("parse-options: allow 
positivation of options starting, with no-", 2012-02-25) merge and 
commit have accepted "--verify" but it is undocumented. The 
documentation updates and fix to pull in this patch are very welcome, 
but I'm not sure we need the other changes. I've left a couple of 
comments below.

[As an aside we should probably improve the documentation in 
parse-options.h if both Peff and Junio did not know how it handles 
"--no-foo" but that is outside the scope of this patch]

Incidentally, this also fixes unexpected calling of the hooks by "git
pull" when "--no-verify" was specified, where it was incorrectly
translated to "--no-verify-signatures". This caused the unexpected
effect of the hooks being called. And an even more unexpected effect of
disabling verification of signatures.

Ouch!

I think for the synopsis it is fine just to list the most common 
options. Having --no-verify without the [no-] makes it clear that 
--verify is the default so is not a commonly used option.

I think saying "the pre-merge and commit-msg hooks" would be clearer as 
you do below.

+	options is given, these are bypassed.
+	See also linkgit:githooks[5].
+
+--verify::
+	This option re-enables running of the pre-commit and commit-msg hooks
+	after an earlier `-n` or `--no-verify`.
  	See also linkgit:githooks[5].

Some of the existing documentation describes the "--no-foo" option with 
"--foo" (e.g --[no-]signoff) but in other places we list the two options 
separately (e.g. --[no-]edit), I'd lean towards combining them as you 
have done for the merge documentation but I don't feel strongly about it.

Again I'm not sure changing the synopsis makes things clearer.

I think "the pre-merge ..." would be better here as well.

Thanks for adding a test

Is this to check that "-n" works like "--no-verify"?

I think it would be very useful to add another test that checks 
"--verify" overrides "--no-verify".

Best Wishes

Phillip

From: Alex Riesen <redacted>

This documents re-enabling of the hooks disabled by an earlier
"--no-verify" in command-line.

Signed-off-by: Alexander Riesen <redacted>
---

Hi Phillip,

Phillip Wood, Thu, Oct 28, 2021 15:57:58 +0200:

On 28/10/2021 09:04, Alex Riesen wrote:

quoted

From: Alex Riesen <redacted>

This allows re-enabling of the hooks disabled by an earlier "--no-verify"
in command-line and makes the interface more consistent.

Thanks for working on this. Since 0f1930c587 ("parse-options: allow
positivation of options starting, with no-", 2012-02-25) merge and commit
have accepted "--verify" but it is undocumented. The documentation updates
and fix to pull in this patch are very welcome, but I'm not sure we need the
other changes. I've left a couple of comments below.

[As an aside we should probably improve the documentation in parse-options.h
if both Peff and Junio did not know how it handles "--no-foo" but that is
outside the scope of this patch]

Interesting feature. It is unfortunate it was so well hidden. You're right, of
course, and the newly added tests in t7504-commit-msg-hook.sh pass without any
changes to the "builtin/commit.c".

Removal of double-negation in the code was an improvement to its readability,
but I like small patches more.

Also, the series has no conflicts with 2.33.0 anymore and the "git pull" can
be applied independently.

quoted

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index a3baea32ae..ba66209274 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt

@@ -11,7 +11,7 @@ SYNOPSIS
  'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]
  	   [--dry-run] [(-c | -C | --fixup | --squash) <commit>]
  	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
-	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
+	   [--allow-empty-message] [--[no-]verify] [-e] [--author=<author>]

I think for the synopsis it is fine just to list the most common options.
Having --no-verify without the [no-] makes it clear that --verify is the
default so is not a commonly used option.

Yep, makes sense.

quoted

  	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
  	   [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]]
  	   [-S[<keyid>]] [--] [<pathspec>...]

@@ -174,7 +174,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
  -n::
  --no-verify::
-	This option bypasses the pre-commit and commit-msg hooks.
+	By default, pre-merge and commit-msg hooks are run. When one of these

I think saying "the pre-merge and commit-msg hooks" would be clearer as you
do below.

quoted

+	options is given, these are bypassed.
+	See also linkgit:githooks[5].
+
+--verify::
+	This option re-enables running of the pre-commit and commit-msg hooks
+	after an earlier `-n` or `--no-verify`.
  	See also linkgit:githooks[5].

Some of the existing documentation describes the "--no-foo" option with
"--foo" (e.g --[no-]signoff) but in other places we list the two options
separately (e.g. --[no-]edit), I'd lean towards combining them as you have
done for the merge documentation but I don't feel strongly about it.

How about this instead:

  -n::
  --no-verify::
          By default, pre-commit and commit-msg hooks are run. When one of these
          options is given, the hooks will be bypassed.
          See also linkgit:githooks[5].

  --verify::
          This option re-enables running of the pre-commit and commit-msg hooks
          after an earlier `-n` or `--no-verify`.

quoted

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 3819fadac1..324ae879d2 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt

@@ -10,7 +10,7 @@ SYNOPSIS
  --------
  [verse]
  'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
-	[--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
+	[--[no-]verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]

Again I'm not sure changing the synopsis makes things clearer.

Removed.

quoted

  	[--[no-]allow-unrelated-histories]
  	[--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
  'git merge' (--continue | --abort | --quit)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 80d4831662..f8016b0f7b 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt

@@ -112,8 +112,9 @@ option can be used to override --squash.
  +
  With --squash, --commit is not allowed, and will fail.
---no-verify::
-	This option bypasses the pre-merge and commit-msg hooks.
+--[no-]verify::
+	By default, pre-merge and commit-msg hooks are run. When `--no-verify`

I think "the pre-merge ..." would be better here as well.

Like this?

  --[no-]verify::
          By default, the pre-merge and commit-msg hooks are run.
	  When `--no-verify` is given, these are bypassed.
          See also linkgit:githooks[5].

quoted

diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 31b9c6a2c1..166ff5fb26 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh

@@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '
  '
+test_expect_success '-n with failing hook' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit -n -m "more"
+
+'

Is this to check that "-n" works like "--no-verify"?

Frankly, it was to check that the separate "-n" option works as I supposed it
would. I never used parse-options before.

I think it would be very useful to add another test that checks "--verify"
overrides "--no-verify".

Replaced the test with one which has "-n --verify".

Thanks!


 Documentation/git-commit.txt    | 7 ++++++-
 Documentation/merge-options.txt | 5 +++--
 t/t7504-commit-msg-hook.sh      | 8 ++++++++
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index a3baea32ae..2268787483 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt

@@ -174,9 +174,14 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
 
 -n::
 --no-verify::
-	This option bypasses the pre-commit and commit-msg hooks.
+	By default, pre-commit and commit-msg hooks are run. When one of these
+	options is given, the hooks will be bypassed.
 	See also linkgit:githooks[5].
 
+--verify::
+	This option re-enables running of the pre-commit and commit-msg hooks
+	after an earlier `-n` or `--no-verify`.
+
 --allow-empty::
 	Usually recording a commit that has the exact same tree as its
 	sole parent commit is a mistake, and the command prevents you

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 80d4831662..80267008af 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt

@@ -112,8 +112,9 @@ option can be used to override --squash.
 +
 With --squash, --commit is not allowed, and will fail.
 
---no-verify::
-	This option bypasses the pre-merge and commit-msg hooks.
+--[no-]verify::
+	By default, the pre-merge and commit-msg hooks are run.
+	When `--no-verify` is given, these are bypassed.
 	See also linkgit:githooks[5].
 
 -s <strategy>::

diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 31b9c6a2c1..67fcc19637 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh

@@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '
 
 '
 
+test_expect_success '-n followed by --verify with failing hook' '
+
+	echo "even more" >> file &&
+	git add file &&
+	test_must_fail git commit -n --verify -m "even more"
+
+'
+
 test_expect_success '--no-verify with failing hook (editor)' '
 
 	echo "more stuff" >> file &&

-- 
2.33.0.22.g8cd9218530

From: Alex Riesen <redacted>

The option was incorrectly auto-translated to "--no-verify-signatures",
which causes the unexpected effect of the hook being called.
And an even more unexpected effect of disabling verification of signatures.

The manual page describes the option to behave same as the similarly
named option of "git merge", which seems to be the original intention
of this option in the "pull" command.

Signed-off-by: Alexander Riesen <redacted>
---
 builtin/pull.c          |  6 ++++++
 t/t5521-pull-options.sh | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 425950f469..e783da10b2 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -84,6 +84,7 @@ static char *opt_edit;
 static char *cleanup_arg;
 static char *opt_ff;
 static char *opt_verify_signatures;
+static char *opt_verify;
 static int opt_autostash = -1;
 static int config_autostash;
 static int check_trust_level = 1;

@@ -160,6 +161,9 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
 		N_("abort if fast-forward is not possible"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+	OPT_PASSTHRU(0, "verify", &opt_verify, NULL,
+		N_("control use of pre-merge-commit and commit-msg hooks"),
+		PARSE_OPT_NOARG),
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),

@@ -688,6 +692,8 @@ static int run_merge(void)
 		strvec_pushf(&args, "--cleanup=%s", cleanup_arg);
 	if (opt_ff)
 		strvec_push(&args, opt_ff);
+	if (opt_verify)
+		strvec_push(&args, opt_verify);
 	if (opt_verify_signatures)
 		strvec_push(&args, opt_verify_signatures);
 	strvec_pushv(&args, opt_strategies.v);

diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index db1a381cd9..22cf1b2cf7 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh

@@ -225,4 +225,28 @@ test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'git pull --no-verify flag passed to merge' '
+	test_when_finished "rm -fr src dst actual" &&
+	git init src &&
+	test_commit -C src one &&
+	git clone src dst &&
+	write_script dst/.git/hooks/commit-msg <<-\EOF &&
+	false
+	EOF
+	test_commit -C src two &&
+	git -C dst pull --no-ff --no-verify
+'
+
+test_expect_success 'git pull --no-verify --verify passed to merge' '
+	test_when_finished "rm -fr src dst actual" &&
+	git init src &&
+	test_commit -C src one &&
+	git clone src dst &&
+	write_script dst/.git/hooks/commit-msg <<-\EOF &&
+	false
+	EOF
+	test_commit -C src two &&
+	test_must_fail git -C dst pull --no-ff --no-verify --verify
+'
+
 test_done

-- 
2.33.0.22.g8cd9218530

Subject of this messages was supposed to be

    Document positive variant of commit and merge option "--no-verify"

But I hand-edited it beyond all repair :-(

Hi Alex

On 28/10/2021 16:44, Alex Riesen wrote:

As I suggested yesterday I think this would be better if it kept the 
"the" from the original text as you do below for the merge documentation 
- s/default, /&the /

This text looks good. It would be nice to be consistent when documenting 
"--verify" and "--no-verify" so that documentation for commit and merge 
both have either a separate entry for each option as you have for commit 
or a shared entry as you have here for merge. I'd be tempted to use this 
form in the commit documentation.

Thanks, having the new test is very helpful.

Best Wishes

Phillip

From: Alex Riesen <redacted>

This documents "--verify" option of the commands. It can be used to re-enable
the hooks disabled by an earlier "--no-verify" in command-line.

Signed-off-by: Alexander Riesen <redacted>
---

Phillip Wood, Fri, Oct 29, 2021 15:32:16 +0200:

On 28/10/2021 16:44, Alex Riesen wrote:

quoted

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index a3baea32ae..2268787483 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt

@@ -174,9 +174,14 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
  -n::
  --no-verify::
-	This option bypasses the pre-commit and commit-msg hooks.
+	By default, pre-commit and commit-msg hooks are run. When one of these

As I suggested yesterday I think this would be better if it kept the "the"
from the original text as you do below for the merge documentation -
s/default, /&the /

Updated:

    -n::
    --[no-]verify::
	    By default, the pre-commit and commit-msg hooks are run.
	    When any of `--no-verify` or `-n` is given, these are bypassed.
	    See also linkgit:githooks[5].

quoted

--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt

@@ -112,8 +112,9 @@ option can be used to override --squash.
  +
  With --squash, --commit is not allowed, and will fail.
---no-verify::
-	This option bypasses the pre-merge and commit-msg hooks.
+--[no-]verify::
+	By default, the pre-merge and commit-msg hooks are run.
+	When `--no-verify` is given, these are bypassed.
  	See also linkgit:githooks[5].

This text looks good. It would be nice to be consistent when documenting
"--verify" and "--no-verify" so that documentation for commit and merge both
have either a separate entry for each option as you have for commit or a
shared entry as you have here for merge. I'd be tempted to use this form in
the commit documentation.

So I did.

Regards,
Alex

 Documentation/git-commit.txt    | 5 +++--
 Documentation/merge-options.txt | 5 +++--
 t/t7504-commit-msg-hook.sh      | 8 ++++++++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index a3baea32ae..b27a4c4c34 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt

@@ -173,8 +173,9 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
 	(see http://developercertificate.org/ for more information).
 
 -n::
---no-verify::
-	This option bypasses the pre-commit and commit-msg hooks.
+--[no-]verify::
+	By default, the pre-commit and commit-msg hooks are run.
+	When any of `--no-verify` or `-n` is given, these are bypassed.
 	See also linkgit:githooks[5].
 
 --allow-empty::

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 80d4831662..80267008af 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt

@@ -112,8 +112,9 @@ option can be used to override --squash.
 +
 With --squash, --commit is not allowed, and will fail.
 
---no-verify::
-	This option bypasses the pre-merge and commit-msg hooks.
+--[no-]verify::
+	By default, the pre-merge and commit-msg hooks are run.
+	When `--no-verify` is given, these are bypassed.
 	See also linkgit:githooks[5].
 
 -s <strategy>::

diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 31b9c6a2c1..67fcc19637 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh

@@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '
 
 '
 
+test_expect_success '-n followed by --verify with failing hook' '
+
+	echo "even more" >> file &&
+	git add file &&
+	test_must_fail git commit -n --verify -m "even more"
+
+'
+
 test_expect_success '--no-verify with failing hook (editor)' '
 
 	echo "more stuff" >> file &&

-- 
2.33.0.22.g8cd9218530

Hi Alex

On 29/10/2021 14:45, Alex Riesen wrote:

From: Alex Riesen <redacted>

This documents "--verify" option of the commands. It can be used to re-enable
the hooks disabled by an earlier "--no-verify" in command-line.

Signed-off-by: Alexander Riesen <redacted>
---

This version looks good, thanks for documenting these options

Best Wishes

Phillip