| Message ID | YXfwanz3MynCLDmn@pflmari (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" | expand |
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
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);
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
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 --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
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(+)