diff mbox series

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

Message ID YXfwanz3MynCLDmn@pflmari (mailing list archive)
State Superseded
Headers show
Series Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" | expand

Commit Message

Alex Riesen Oct. 26, 2021, 12:11 p.m. UTC
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(+)

Comments

Jeff King Oct. 26, 2021, 9:16 p.m. UTC | #1
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
Alex Riesen Oct. 27, 2021, 12:09 p.m. UTC | #2
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);
Jeff King Oct. 27, 2021, 12:19 p.m. UTC | #3
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
Junio C Hamano Oct. 27, 2021, 8:12 p.m. UTC | #4
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.
diff mbox series

Patch

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