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

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

[Alex Riesen]

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(+)

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

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

[Jeff King]

On Tue, Oct 26, 2021 at 02:11:22PM +0200, Alex Riesen wrote:

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

> +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

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

[Alex Riesen]

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:
> 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(+)

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

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

[Jeff King]

On Wed, Oct 27, 2021 at 08:35:07AM +0200, Alex Riesen wrote:

> 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

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

[Alex Riesen]

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

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

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

[Jeff King]

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

> 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

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

[Alex Riesen]

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

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Junio C Hamano]

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"?  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.

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

[Alex Riesen]

Junio C Hamano, Wed, Oct 27, 2021 22:16:35 +0200:
> 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.

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

[Alex Riesen]

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:
> 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(-)

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

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

[Phillip Wood]

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]

> 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!

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

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

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

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

> +	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

>   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

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

[Alex Riesen]

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
+	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

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

[Alex Riesen]

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

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

[Alex Riesen]

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 :-(

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

[Junio C Hamano]

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.

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

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

[Alex Riesen]

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?

> 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

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

[Junio C Hamano]

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.

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

[Alex Riesen]

Junio C Hamano, Thu, Oct 28, 2021 21:25:13 +0200:
> 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!

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

[Phillip Wood]

Hi Alex

On 28/10/2021 16:44, Alex Riesen wrote:
> 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 /

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

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

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

[Alex Riesen]

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:
> 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 &&
-- 
2.33.0.22.g8cd9218530

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

[Phillip Wood]

Hi Alex

On 29/10/2021 14:45, Alex Riesen wrote:
> 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

> 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 &&
>