threads / patch / 56787

patchFix "commit-msg" hook unexpectedly called for "git pull --no-verify"

Subject: [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

## tl;dr

22 messages between Oct 26, 2021 and Nov 1, 2021. Diffs are folded; open one to read it.

replies: 21people: 4as markdown or json

Alex Riesen· Oct 26, 2021, 12:11 UTC · lore

The option is incorrectly 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 <raa.lkml@gmail.com>
---
 builtin/pull.c          |  6 ++++++
 t/t5521-pull-options.sh | 11 +++++++++++
 2 files changed, 17 insertions(+)
Show changes to 2 files +17 −0

builtin/pull.c, t/t5521-pull-options.sh

diff --git a/builtin/pull.c b/builtin/pull.c
index 425950f469..428baea95b 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_no_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, "no-verify", &opt_no_verify, NULL,
+		N_("bypass pre-merge-commit and commit-msg hooks"),
+		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
 	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_no_verify)
+		strvec_push(&args, opt_no_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..0eb1916175 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -225,4 +225,15 @@ 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 &&
+	echo false >dst/.git/hooks/commit-msg &&
+	chmod +x dst/.git/hooks/commit-msg &&
+	test_commit -C src two &&
+	git -C dst pull --no-ff --no-verify
+'
+
 test_done
-- 
2.31.0.30.g60a470ee5c
Jeff King· Oct 26, 2021, 21:16 UTC · re: Alex Riesen · lore

Re: [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

On Tue, Oct 26, 2021 at 02:11:22PM +0200, Alex Riesen wrote:
Show 22 quoted lines
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 425950f469..428baea95b 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_no_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, "no-verify", &opt_no_verify, NULL,
> +		N_("bypass pre-merge-commit and commit-msg hooks"),
> +		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
>  	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
>  		N_("verify that the named commit has a valid GPG signature"),
>  		PARSE_OPT_NOARG),

OK, so we failed to pass through --no-verify, because it got caught as a prefix of --verify-signatures, since the outer parse-options didn't know about it. Makes sense, and I suppose this has been broken since 11b6d17801 (pull: pass git-merge's options to git-merge, 2015-06-14).

I was going to ask whether this should be passing through "verify", and allowing its "no-" variant, but there is no "--verify" in git-merge. Arguably there should be (for consistency and to countermand an earlier --no-verify), but that is outside the scope of your fix (sadly if somebody does change that, they'll have to remember to touch this spot, too, but I don't think it can be helped).

Show 7 quoted lines
> +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 &&
> +	echo false >dst/.git/hooks/commit-msg &&
> +	chmod +x dst/.git/hooks/commit-msg &&

This script without #! should work portably, I think, though we generally prefer using the helper (which also handles the chmod):

  write_script dst/.git/hooks/commit-msg <<-\EOF
  false
  EOF
Other than that nit, this looks good to me.
-Peff
Alex Riesen· Oct 27, 2021, 06:35 UTC · re: Jeff King · lore

[PATCH v2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

The option is incorrectly 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 <raa.lkml@gmail.com>
---
Jeff King, Tue, Oct 26, 2021 23:16:09 +0200:
Show 17 quoted lines
> On Tue, Oct 26, 2021 at 02:11:22PM +0200, Alex Riesen wrote:
> > +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 &&
> > +	echo false >dst/.git/hooks/commit-msg &&
> > +	chmod +x dst/.git/hooks/commit-msg &&
> 
> This script without #! should work portably, I think, though we
> generally prefer using the helper (which also handles the chmod):
> 
>   write_script dst/.git/hooks/commit-msg <<-\EOF
>   false
>   EOF
> 
> Other than that nit, this looks good to me.
Updated. Certainly looks nicer.
 builtin/pull.c          |  6 ++++++
 t/t5521-pull-options.sh | 12 ++++++++++++
 2 files changed, 18 insertions(+)
Show changes to 2 files +18 −0

builtin/pull.c, t/t5521-pull-options.sh

diff --git a/builtin/pull.c b/builtin/pull.c
index 425950f469..428baea95b 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_no_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, "no-verify", &opt_no_verify, NULL,
+		N_("bypass pre-merge-commit and commit-msg hooks"),
+		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
 	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_no_verify)
+		strvec_push(&args, opt_no_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
-- 
2.33.0.22.g8cd9218530
Jeff King· Oct 27, 2021, 09:06 UTC · re: Alex Riesen · lore

Re: [PATCH v2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

On Wed, Oct 27, 2021 at 08:35:07AM +0200, Alex Riesen wrote:
Show 8 quoted lines
> The option is incorrectly 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.
Thanks, this looks good to me.
-Peff
Alex Riesen· Oct 27, 2021, 12:09 UTC · re: Jeff King · lore

Re: [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

Jeff King, Tue, Oct 26, 2021 23:16:09 +0200:
Show 7 quoted lines
> On Tue, Oct 26, 2021 at 02:11:22PM +0200, Alex Riesen wrote:
> I was going to ask whether this should be passing through "verify", and
> allowing its "no-" variant, but there is no "--verify" in git-merge.
> Arguably there should be (for consistency and to countermand an earlier
> --no-verify), but that is outside the scope of your fix (sadly if
> somebody does change that, they'll have to remember to touch this spot,
> too, but I don't think it can be helped).
This seems simple enough, though. Like this?
[PATCH] Remove negation from the merge option "--no-verify"
This allows re-enabling hooks disabled by an earlier "--no-verify"
in command-line and makes the interface more consistent.
---
 Documentation/git-merge.txt     |  2 +-
 Documentation/merge-options.txt |  5 +++--
 builtin/merge.c                 | 12 ++++++------
 builtin/pull.c                  | 12 ++++++------
 4 files changed, 16 insertions(+), 15 deletions(-)
Show changes to 4 files +16 −14

Documentation/git-merge.txt, Documentation/merge-options.txt, builtin/merge.c, builtin/pull.c

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..54cd3b04df 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::
+	With `--no-verify`, bypass the pre-merge and commit-msg hooks,
+	which will be run by default.
 	See also linkgit:githooks[5].
 
 -s <strategy>::
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 428baea95b..e783da10b2 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -84,7 +84,7 @@ static char *opt_edit;
 static char *cleanup_arg;
 static char *opt_ff;
 static char *opt_verify_signatures;
-static char *opt_no_verify;
+static char *opt_verify;
 static int opt_autostash = -1;
 static int config_autostash;
 static int check_trust_level = 1;
@@ -161,9 +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, "no-verify", &opt_no_verify, NULL,
-		N_("bypass pre-merge-commit and commit-msg hooks"),
-		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),
@@ -692,8 +692,8 @@ static int run_merge(void)
 		strvec_pushf(&args, "--cleanup=%s", cleanup_arg);
 	if (opt_ff)
 		strvec_push(&args, opt_ff);
-	if (opt_no_verify)
-		strvec_push(&args, opt_no_verify);
+	if (opt_verify)
+		strvec_push(&args, opt_verify);
 	if (opt_verify_signatures)
 		strvec_push(&args, opt_verify_signatures);
 	strvec_pushv(&args, opt_strategies.v);
-- 
2.33.0.22.g8cd9218530
Jeff King· Oct 27, 2021, 12:19 UTC · re: Alex Riesen · lore

Re: [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

On Wed, Oct 27, 2021 at 02:09:42PM +0200, Alex Riesen wrote:
Show 15 quoted lines
> Jeff King, Tue, Oct 26, 2021 23:16:09 +0200:
> > On Tue, Oct 26, 2021 at 02:11:22PM +0200, Alex Riesen wrote:
> > I was going to ask whether this should be passing through "verify", and
> > allowing its "no-" variant, but there is no "--verify" in git-merge.
> > Arguably there should be (for consistency and to countermand an earlier
> > --no-verify), but that is outside the scope of your fix (sadly if
> > somebody does change that, they'll have to remember to touch this spot,
> > too, but I don't think it can be helped).
> 
> This seems simple enough, though. Like this?
> 
> [PATCH] Remove negation from the merge option "--no-verify"
> 
> This allows re-enabling hooks disabled by an earlier "--no-verify"
> in command-line and makes the interface more consistent.

Yeah, I don't see any problems in the patch below, and I agree it makes things overall nicer (both the user-facing parts, and not having to see the double-negative "!no_verify" in the code).

Show 13 quoted lines
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 80d4831662..54cd3b04df 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::
> +	With `--no-verify`, bypass the pre-merge and commit-msg hooks,
> +	which will be run by default.
This "which will be run by default" is a little awkward. Maybe:
  By default, pre-merge and commit-msg hooks are run. When `--no-verify`
  is given, these are bypassed.
?
-Peff
Alex Riesen· Oct 27, 2021, 13:27 UTC · re: Jeff King · lore

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

From: Alex Riesen <raa.lkml@gmail.com>

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 <raa.lkml@gmail.com>
---

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?

Jeff King, Wed, Oct 27, 2021 14:19:49 +0200:
Show 20 quoted lines
> On Wed, Oct 27, 2021 at 02:09:42PM +0200, Alex Riesen wrote:
> > Jeff King, Tue, Oct 26, 2021 23:16:09 +0200:
> > > On Tue, Oct 26, 2021 at 02:11:22PM +0200, Alex Riesen wrote:
> > > I was going to ask whether this should be passing through "verify", and
> > > allowing its "no-" variant, but there is no "--verify" in git-merge.
> > > Arguably there should be (for consistency and to countermand an earlier
> > > --no-verify), but that is outside the scope of your fix (sadly if
> > > somebody does change that, they'll have to remember to touch this spot,
> > > too, but I don't think it can be helped).
> > 
> > This seems simple enough, though. Like this?
> > 
> > [PATCH] Remove negation from the merge option "--no-verify"
> > 
> > This allows re-enabling hooks disabled by an earlier "--no-verify"
> > in command-line and makes the interface more consistent.
> 
> Yeah, I don't see any problems in the patch below, and I agree it makes
> things overall nicer (both the user-facing parts, and not having to see
> the double-negative "!no_verify" in the code).
Ok, resending it formally.
Show 20 quoted lines
> > diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> > index 80d4831662..54cd3b04df 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::
> > +	With `--no-verify`, bypass the pre-merge and commit-msg hooks,
> > +	which will be run by default.
> 
> This "which will be run by default" is a little awkward. Maybe:
> 
>   By default, pre-merge and commit-msg hooks are run. When `--no-verify`
>   is given, these are bypassed.
> 
> ?
Of course. It certainly reads better like this.
 Documentation/git-merge.txt     |  2 +-
 Documentation/merge-options.txt |  5 +++--
 builtin/merge.c                 | 12 ++++++------
 builtin/pull.c                  | 12 ++++++------
 4 files changed, 16 insertions(+), 15 deletions(-)
Show changes to 4 files +16 −14

Documentation/git-merge.txt, Documentation/merge-options.txt, builtin/merge.c, builtin/pull.c

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/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 428baea95b..e783da10b2 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -84,7 +84,7 @@ static char *opt_edit;
 static char *cleanup_arg;
 static char *opt_ff;
 static char *opt_verify_signatures;
-static char *opt_no_verify;
+static char *opt_verify;
 static int opt_autostash = -1;
 static int config_autostash;
 static int check_trust_level = 1;
@@ -161,9 +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, "no-verify", &opt_no_verify, NULL,
-		N_("bypass pre-merge-commit and commit-msg hooks"),
-		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),
@@ -692,8 +692,8 @@ static int run_merge(void)
 		strvec_pushf(&args, "--cleanup=%s", cleanup_arg);
 	if (opt_ff)
 		strvec_push(&args, opt_ff);
-	if (opt_no_verify)
-		strvec_push(&args, opt_no_verify);
+	if (opt_verify)
+		strvec_push(&args, opt_verify);
 	if (opt_verify_signatures)
 		strvec_push(&args, opt_verify_signatures);
 	strvec_pushv(&args, opt_strategies.v);
-- 
2.33.0.22.g8cd9218530
Junio C Hamano· Oct 27, 2021, 20:16 UTC · re: Alex Riesen · lore

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

Alex Riesen <alexander.riesen@cetitec.com> writes:
Show 12 quoted lines
> From: Alex Riesen <raa.lkml@gmail.com>
>
> 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 <raa.lkml@gmail.com>
>
> ---
>
> 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.

Alex Riesen· Oct 28, 2021, 06:38 UTC · re: Junio C Hamano · lore

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

Junio C Hamano, Wed, Oct 27, 2021 22:16:35 +0200:
Show 16 quoted lines
> Alex Riesen <alexander.riesen@cetitec.com> writes:
> 
> > From: Alex Riesen <raa.lkml@gmail.com>
> >
> > 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 <raa.lkml@gmail.com>
> >
> > ---
> >
> > 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.
Alex Riesen· Oct 28, 2021, 08:04 UTC · re: Alex Riesen · lore

[PATCH] Remove negation from the commit and merge option "--no-verify"

From: Alex Riesen <raa.lkml@gmail.com>

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 <raa.lkml@gmail.com>
---
Alex Riesen, Thu, Oct 28, 2021 08:38:04 +0200:
Show 6 quoted lines
> Junio C Hamano, Wed, Oct 27, 2021 22:16:35 +0200:
> > 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(-)
Show changes to 8 files +61 −15

Documentation/git-commit.txt, Documentation/git-merge.txt, Documentation/merge-options.txt, builtin/commit.c, builtin/merge.c, builtin/pull.c, t/t5521-pull-options.sh, t/t7504-commit-msg-hook.sh

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
Phillip Wood· Oct 28, 2021, 13:57 UTC · re: Alex Riesen · lore

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

Hi Alex
On 28/10/2021 09:04, Alex Riesen wrote:
> From: Alex Riesen <raa.lkml@gmail.com>
> 
> 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]

Show 5 quoted lines
> 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!
Show 12 quoted lines
> Signed-off-by: Alexander Riesen <raa.lkml@gmail.com>
>[...]
> 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.

Show 9 quoted lines
>   	   [--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.

Show 7 quoted lines
> +	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.

Show 11 quoted lines
>   --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>]]
Again I'm not sure changing the synopsis makes things clearer.
Show 15 quoted lines
>   	[--[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.
Show 22 quoted lines
> +	is given, these are bypassed.
>   	See also linkgit:githooks[5].
>[...]   
> 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
> +'
Thanks for adding a test
Show 16 quoted lines
>   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"
> +
> +'
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
Alex Riesen· Oct 28, 2021, 15:44 UTC · re: Phillip Wood · lore

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

From: Alex Riesen <raa.lkml@gmail.com>

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

Signed-off-by: Alexander Riesen <raa.lkml@gmail.com>
---
Hi Phillip,
Phillip Wood, Thu, Oct 28, 2021 15:57:58 +0200:
Show 15 quoted lines
> On 28/10/2021 09:04, Alex Riesen wrote:
> > From: Alex Riesen <raa.lkml@gmail.com>
> > 
> > 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.

Show 14 quoted lines
> > 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.
Show 24 quoted lines
> >   	   [--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.
> 
> > +	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`.
Show 12 quoted lines
> > 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.
Show 16 quoted lines
> >   	[--[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].
Show 15 quoted lines
> > 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(-)
Show changes to 3 files +17 −2

Documentation/git-commit.txt, Documentation/merge-options.txt, t/t7504-commit-msg-hook.sh

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
Alex Riesen· Oct 28, 2021, 15:46 UTC · re: Alex Riesen · lore

[PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

From: Alex Riesen <raa.lkml@gmail.com>

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 <raa.lkml@gmail.com>
---
 builtin/pull.c          |  6 ++++++
 t/t5521-pull-options.sh | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+)
Show changes to 2 files +30 −0

builtin/pull.c, t/t5521-pull-options.sh

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
Junio C Hamano· Oct 28, 2021, 16:51 UTC · re: Alex Riesen · lore

Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

Alex Riesen <alexander.riesen@cetitec.com> writes:
> Subject: Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"
Perhaps
    Subject: [PATCH] pull: honor --no-verify and do not call the commit-msg hook
instead.
Show 46 quoted lines
> From: Alex Riesen <raa.lkml@gmail.com>
>
> 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 <raa.lkml@gmail.com>
> ---
>  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);

Looks quite straight-forward, especially that now this just mimicks how --[no-]verify-signatures is passed through.

Thanks, will queue.
Alex Riesen· Oct 28, 2021, 17:16 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

Junio C Hamano, Thu, Oct 28, 2021 18:51:17 +0200:
Show 9 quoted lines
> Alex Riesen <alexander.riesen@cetitec.com> writes:
> 
> > Subject: Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"
> 
> Perhaps
> 
>     Subject: [PATCH] pull: honor --no-verify and do not call the commit-msg hook
> 
> instead.
Looks fine from my side. Shall I resend?
> Looks quite straight-forward, especially that now this just mimicks
> how --[no-]verify-signatures is passed through.
> 
> Thanks, will queue.
Or did you queue it as is?

Regards, Alex

Junio C Hamano· Oct 28, 2021, 19:25 UTC · re: Alex Riesen · lore

Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

Alex Riesen <alexander.riesen@cetitec.com> writes:
Show 12 quoted lines
> Junio C Hamano, Thu, Oct 28, 2021 18:51:17 +0200:
>> Alex Riesen <alexander.riesen@cetitec.com> writes:
>> 
>> > Subject: Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"
>> 
>> Perhaps
>> 
>>     Subject: [PATCH] pull: honor --no-verify and do not call the commit-msg hook
>> 
>> instead.
>
> Looks fine from my side. Shall I resend?
If you are OK with the updated text, then I can locally amend.
Alex Riesen· Oct 29, 2021, 06:34 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

Junio C Hamano, Thu, Oct 28, 2021 21:25:13 +0200:
Show 16 quoted lines
> Alex Riesen <alexander.riesen@cetitec.com> writes:
> 
> > Junio C Hamano, Thu, Oct 28, 2021 18:51:17 +0200:
> >> Alex Riesen <alexander.riesen@cetitec.com> writes:
> >> 
> >> > Subject: Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"
> >> 
> >> Perhaps
> >> 
> >>     Subject: [PATCH] pull: honor --no-verify and do not call the commit-msg hook
> >> 
> >> instead.
> >
> > Looks fine from my side. Shall I resend?
> 
> If you are OK with the updated text, then I can locally amend.
Yes, of course! Thanks!
Alex Riesen· Oct 28, 2021, 15:49 UTC · re: Alex Riesen · lore

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

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 :-(
Phillip Wood· Oct 29, 2021, 13:32 UTC · re: Alex Riesen · lore

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

Hi Alex
On 28/10/2021 16:44, Alex Riesen wrote:
Show 172 quoted lines
> From: Alex Riesen <raa.lkml@gmail.com>
> 
> This documents re-enabling of the hooks disabled by an earlier
> "--no-verify" in command-line.
> 
> Signed-off-by: Alexander Riesen <raa.lkml@gmail.com>
> ---
> 
> Hi Phillip,
> 
> Phillip Wood, Thu, Oct 28, 2021 15:57:58 +0200:
>> On 28/10/2021 09:04, Alex Riesen wrote:
>>> From: Alex Riesen <raa.lkml@gmail.com>
>>>
>>> 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.
> 
>>> 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.
> 
>>>    	   [--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.
>>
>>> +	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`.
> 
>>> 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.
> 
>>>    	[--[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].
> 
>>> 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
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 /
Show 23 quoted lines
> +	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].

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.

Show 16 quoted lines
>   -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"
> +
> +'
Thanks, having the new test is very helpful.
Best Wishes
Phillip
Alex Riesen· Oct 29, 2021, 13:45 UTC · re: Phillip Wood · lore

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

From: Alex Riesen <raa.lkml@gmail.com>

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 <raa.lkml@gmail.com>
---
Phillip Wood, Fri, Oct 29, 2021 15:32:16 +0200:
Show 14 quoted lines
> On 28/10/2021 16:44, Alex Riesen wrote:
> > 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].
Show 17 quoted lines
> > --- 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(-)
Show changes to 3 files +14 −2

Documentation/git-commit.txt, Documentation/merge-options.txt, t/t7504-commit-msg-hook.sh

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
Phillip Wood· Nov 1, 2021, 15:34 UTC · re: Alex Riesen · lore

Re: [PATCH] Document positive variant of commit and merge option "--no-verify"

Hi Alex
On 29/10/2021 14:45, Alex Riesen wrote:
Show 7 quoted lines
> From: Alex Riesen <raa.lkml@gmail.com>
> 
> 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 <raa.lkml@gmail.com>
> ---
This version looks good, thanks for documenting these options
Best Wishes
Phillip
Show 104 quoted lines
> Phillip Wood, Fri, Oct 29, 2021 15:32:16 +0200:
>> On 28/10/2021 16:44, Alex Riesen wrote:
>>> 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].
> 
>>> --- 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 &&
> 
Junio C Hamano· Oct 27, 2021, 20:12 UTC · re: Jeff King · lore

Re: [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify"

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> OK, so we failed to pass through --no-verify, because it got caught as a
> prefix of --verify-signatures, since the outer parse-options didn't know
> about it. Makes sense, and I suppose this has been broken since
> 11b6d17801 (pull: pass git-merge's options to git-merge, 2015-06-14).
>
> I was going to ask whether this should be passing through "verify", and
> allowing its "no-" variant, but there is no "--verify" in git-merge.
> Arguably there should be (for consistency and to countermand an earlier
> --no-verify), but that is outside the scope of your fix (sadly if
> somebody does change that, they'll have to remember to touch this spot,
> too, but I don't think it can be helped).

We do not even have "--verify" in "git commit", because letting the hooks to interfere is the default, but if we were designing it today, we probably would add "--verify" to override a "--no-verify" earlier on the command line, so it is not implausible that people would want to add "--verify" to "git commit" and "git merge" in the future.

We can add two hunks, one for builtin/merge.c and another for builtin/pull.c, to leave a note for future developers and it would help quite a lot, I would presume.

Thanks.

← back to recent threads