* [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" @ 2021-10-26 12:11 Alex Riesen 2021-10-26 21:16 ` Jeff King 0 siblings, 1 reply; 22+ messages in thread From: Alex Riesen @ 2021-10-26 12:11 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano, Jeff King 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" 2021-10-26 12:11 [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" Alex Riesen @ 2021-10-26 21:16 ` Jeff King 2021-10-27 6:35 ` [PATCH v2] " Alex Riesen ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Jeff King @ 2021-10-26 21:16 UTC (permalink / raw) To: Alex Riesen; +Cc: Git List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" 2021-10-26 21:16 ` Jeff King @ 2021-10-27 6:35 ` Alex Riesen 2021-10-27 9:06 ` Jeff King 2021-10-27 12:09 ` [PATCH] " Alex Riesen 2021-10-27 20:12 ` [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" Junio C Hamano 2 siblings, 1 reply; 22+ messages in thread From: Alex Riesen @ 2021-10-27 6:35 UTC (permalink / raw) To: Jeff King; +Cc: Git List, Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" 2021-10-27 6:35 ` [PATCH v2] " Alex Riesen @ 2021-10-27 9:06 ` Jeff King 0 siblings, 0 replies; 22+ messages in thread From: Jeff King @ 2021-10-27 9:06 UTC (permalink / raw) To: Alex Riesen; +Cc: Git List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" 2021-10-26 21:16 ` Jeff King 2021-10-27 6:35 ` [PATCH v2] " Alex Riesen @ 2021-10-27 12:09 ` Alex Riesen 2021-10-27 12:19 ` Jeff King 2021-10-27 20:12 ` [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" Junio C Hamano 2 siblings, 1 reply; 22+ messages in thread From: Alex Riesen @ 2021-10-27 12:09 UTC (permalink / raw) To: Jeff King; +Cc: Git List, Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" 2021-10-27 12:09 ` [PATCH] " Alex Riesen @ 2021-10-27 12:19 ` Jeff King 2021-10-27 13:27 ` [PATCH] Remove negation from the merge option "--no-verify" Alex Riesen 0 siblings, 1 reply; 22+ messages in thread From: Jeff King @ 2021-10-27 12:19 UTC (permalink / raw) To: Alex Riesen; +Cc: Git List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] Remove negation from the merge option "--no-verify" 2021-10-27 12:19 ` Jeff King @ 2021-10-27 13:27 ` Alex Riesen 2021-10-27 20:16 ` Junio C Hamano 0 siblings, 1 reply; 22+ messages in thread From: Alex Riesen @ 2021-10-27 13:27 UTC (permalink / raw) To: Jeff King; +Cc: Git List, Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] Remove negation from the merge option "--no-verify" 2021-10-27 13:27 ` [PATCH] Remove negation from the merge option "--no-verify" Alex Riesen @ 2021-10-27 20:16 ` Junio C Hamano 2021-10-28 6:38 ` Alex Riesen 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2021-10-27 20:16 UTC (permalink / raw) To: Alex Riesen; +Cc: Jeff King, Git List 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Remove negation from the merge option "--no-verify" 2021-10-27 20:16 ` Junio C Hamano @ 2021-10-28 6:38 ` Alex Riesen 2021-10-28 8:04 ` [PATCH] Remove negation from the commit and " Alex Riesen 0 siblings, 1 reply; 22+ messages in thread From: Alex Riesen @ 2021-10-28 6:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Git List 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] Remove negation from the commit and merge option "--no-verify" 2021-10-28 6:38 ` Alex Riesen @ 2021-10-28 8:04 ` Alex Riesen 2021-10-28 13:57 ` Phillip Wood 0 siblings, 1 reply; 22+ messages in thread From: Alex Riesen @ 2021-10-28 8:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Git List 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] Remove negation from the commit and merge option "--no-verify" 2021-10-28 8:04 ` [PATCH] Remove negation from the commit and " Alex Riesen @ 2021-10-28 13:57 ` Phillip Wood 2021-10-28 15:44 ` Alex Riesen 0 siblings, 1 reply; 22+ messages in thread From: Phillip Wood @ 2021-10-28 13:57 UTC (permalink / raw) To: Alex Riesen, Junio C Hamano; +Cc: Jeff King, Git List 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Remove negation from the commit and merge option "--no-verify" 2021-10-28 13:57 ` Phillip Wood @ 2021-10-28 15:44 ` Alex Riesen 2021-10-28 15:46 ` [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" Alex Riesen ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Alex Riesen @ 2021-10-28 15:44 UTC (permalink / raw) To: phillip.wood; +Cc: Junio C Hamano, Jeff King, Git List 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" 2021-10-28 15:44 ` Alex Riesen @ 2021-10-28 15:46 ` Alex Riesen 2021-10-28 16:51 ` Junio C Hamano 2021-10-28 15:49 ` [PATCH] Remove negation from the commit and merge option "--no-verify" Alex Riesen 2021-10-29 13:32 ` Phillip Wood 2 siblings, 1 reply; 22+ messages in thread From: Alex Riesen @ 2021-10-28 15:46 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano, Jeff King, Phillip Wood 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" 2021-10-28 15:46 ` [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" Alex Riesen @ 2021-10-28 16:51 ` Junio C Hamano 2021-10-28 17:16 ` Alex Riesen 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2021-10-28 16:51 UTC (permalink / raw) To: Alex Riesen; +Cc: Git List, Jeff King, Phillip Wood 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" 2021-10-28 16:51 ` Junio C Hamano @ 2021-10-28 17:16 ` Alex Riesen 2021-10-28 19:25 ` Junio C Hamano 0 siblings, 1 reply; 22+ messages in thread From: Alex Riesen @ 2021-10-28 17:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Jeff King, Phillip Wood 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" 2021-10-28 17:16 ` Alex Riesen @ 2021-10-28 19:25 ` Junio C Hamano 2021-10-29 6:34 ` Alex Riesen 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2021-10-28 19:25 UTC (permalink / raw) To: Alex Riesen; +Cc: Git List, Jeff King, Phillip Wood 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" 2021-10-28 19:25 ` Junio C Hamano @ 2021-10-29 6:34 ` Alex Riesen 0 siblings, 0 replies; 22+ messages in thread From: Alex Riesen @ 2021-10-29 6:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Jeff King, Phillip Wood 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! ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Remove negation from the commit and merge option "--no-verify" 2021-10-28 15:44 ` Alex Riesen 2021-10-28 15:46 ` [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" Alex Riesen @ 2021-10-28 15:49 ` Alex Riesen 2021-10-29 13:32 ` Phillip Wood 2 siblings, 0 replies; 22+ messages in thread From: Alex Riesen @ 2021-10-28 15:49 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano, Jeff King, Phillip Wood 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 :-( ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Remove negation from the commit and merge option "--no-verify" 2021-10-28 15:44 ` Alex Riesen 2021-10-28 15:46 ` [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" Alex Riesen 2021-10-28 15:49 ` [PATCH] Remove negation from the commit and merge option "--no-verify" Alex Riesen @ 2021-10-29 13:32 ` Phillip Wood 2021-10-29 13:45 ` [PATCH] Document positive variant of " Alex Riesen 2 siblings, 1 reply; 22+ messages in thread From: Phillip Wood @ 2021-10-29 13:32 UTC (permalink / raw) To: Alex Riesen, phillip.wood; +Cc: Junio C Hamano, Jeff King, Git List 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] Document positive variant of commit and merge option "--no-verify" 2021-10-29 13:32 ` Phillip Wood @ 2021-10-29 13:45 ` Alex Riesen 2021-11-01 15:34 ` Phillip Wood 0 siblings, 1 reply; 22+ messages in thread From: Alex Riesen @ 2021-10-29 13:45 UTC (permalink / raw) To: Phillip Wood; +Cc: Junio C Hamano, Jeff King, Git List 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] Document positive variant of commit and merge option "--no-verify" 2021-10-29 13:45 ` [PATCH] Document positive variant of " Alex Riesen @ 2021-11-01 15:34 ` Phillip Wood 0 siblings, 0 replies; 22+ messages in thread From: Phillip Wood @ 2021-11-01 15:34 UTC (permalink / raw) To: Alex Riesen, Phillip Wood; +Cc: Junio C Hamano, Jeff King, Git List 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 && > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" 2021-10-26 21:16 ` Jeff King 2021-10-27 6:35 ` [PATCH v2] " Alex Riesen 2021-10-27 12:09 ` [PATCH] " Alex Riesen @ 2021-10-27 20:12 ` Junio C Hamano 2 siblings, 0 replies; 22+ messages in thread From: Junio C Hamano @ 2021-10-27 20:12 UTC (permalink / raw) To: Jeff King; +Cc: Alex Riesen, Git List 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-11-01 15:35 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-26 12:11 [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" Alex Riesen 2021-10-26 21:16 ` Jeff King 2021-10-27 6:35 ` [PATCH v2] " Alex Riesen 2021-10-27 9:06 ` Jeff King 2021-10-27 12:09 ` [PATCH] " Alex Riesen 2021-10-27 12:19 ` Jeff King 2021-10-27 13:27 ` [PATCH] Remove negation from the merge option "--no-verify" Alex Riesen 2021-10-27 20:16 ` Junio C Hamano 2021-10-28 6:38 ` Alex Riesen 2021-10-28 8:04 ` [PATCH] Remove negation from the commit and " Alex Riesen 2021-10-28 13:57 ` Phillip Wood 2021-10-28 15:44 ` Alex Riesen 2021-10-28 15:46 ` [PATCH 2/2] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" Alex Riesen 2021-10-28 16:51 ` Junio C Hamano 2021-10-28 17:16 ` Alex Riesen 2021-10-28 19:25 ` Junio C Hamano 2021-10-29 6:34 ` Alex Riesen 2021-10-28 15:49 ` [PATCH] Remove negation from the commit and merge option "--no-verify" Alex Riesen 2021-10-29 13:32 ` Phillip Wood 2021-10-29 13:45 ` [PATCH] Document positive variant of " Alex Riesen 2021-11-01 15:34 ` Phillip Wood 2021-10-27 20:12 ` [PATCH] Fix "commit-msg" hook unexpectedly called for "git pull --no-verify" Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.