{"thread":{"id":"56787","subject":"[PATCH] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","startedAt":"2021-10-26T12:11:36Z","lastAt":"2021-11-01T15:35:03Z","messageCount":22,"participants":["Alex Riesen","Jeff King","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"439639","messageId":"YXfwanz3MynCLDmn@pflmari","threadId":"56787","inReplyTo":null,"subject":"[PATCH] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-26T12:11:22Z","receivedAt":"2021-10-26T12:11:36Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"The option is incorrectly translated to \"--no-verify-signatures\",\nwhich causes the unexpected effect of the hook being called.\nAnd an even more unexpected effect of disabling verification\nof signatures.\n\nThe manual page describes the option to behave same as the similarly\nnamed option of \"git merge\", which seems to be the original intention\nof this option in the \"pull\" command.\n\nSigned-off-by: Alexander Riesen \n---\n builtin/pull.c | 6 ++++++\n t/t5521-pull-options.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 425950f469..428baea95b 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -84,6 +84,7 @@ static char *opt_edit;\n static char *cleanup_arg;\n static char *opt_ff;\n static char *opt_verify_signatures;\n+static char *opt_no_verify;\n static int opt_autostash = -1;\n static int config_autostash;\n static int check_trust_level = 1;\n@@ -160,6 +161,9 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n \t\tN_(\"abort if fast-forward is not possible\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n+\tOPT_PASSTHRU(0, \"no-verify\", &opt_no_verify, NULL,\n+\t\tN_(\"bypass pre-merge-commit and commit-msg hooks\"),\n+\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n@@ -688,6 +692,8 @@ static int run_merge(void)\n \t\tstrvec_pushf(&args, \"--cleanup=%s\", cleanup_arg);\n \tif (opt_ff)\n \t\tstrvec_push(&args, opt_ff);\n+\tif (opt_no_verify)\n+\t\tstrvec_push(&args, opt_no_verify);\n \tif (opt_verify_signatures)\n \t\tstrvec_push(&args, opt_verify_signatures);\n \tstrvec_pushv(&args, opt_strategies.v);\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex db1a381cd9..0eb1916175 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -225,4 +225,15 @@ test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'git pull --no-verify flag passed to merge' '\n+\ttest_when_finished \"rm -fr src dst actual\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\techo false >dst/.git/hooks/commit-msg &&\n+\tchmod +x dst/.git/hooks/commit-msg &&\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --no-ff --no-verify\n+'\n+\n test_done\n-- \n2.31.0.30.g60a470ee5c\n\n"},{"id":"439685","messageId":"YXhwGQOTfD+ypbo8@coredump.intra.peff.net","threadId":"56787","inReplyTo":"YXfwanz3MynCLDmn@pflmari","subject":"Re: [PATCH] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-26T21:16:09Z","receivedAt":"2021-10-26T21:16:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 26, 2021 at 02:11:22PM +0200, Alex Riesen wrote:\n\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 425950f469..428baea95b 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -84,6 +84,7 @@ static char *opt_edit;\n> static char *cleanup_arg;\n> static char *opt_ff;\n> static char *opt_verify_signatures;\n> +static char *opt_no_verify;\n> static int opt_autostash = -1;\n> static int config_autostash;\n> static int check_trust_level = 1;\n> @@ -160,6 +161,9 @@ static struct option pull_options[] = {\n> \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n> \t\tN_(\"abort if fast-forward is not possible\"),\n> \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n> +\tOPT_PASSTHRU(0, \"no-verify\", &opt_no_verify, NULL,\n> +\t\tN_(\"bypass pre-merge-commit and commit-msg hooks\"),\n> +\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n> \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n> \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n> \t\tPARSE_OPT_NOARG),\n\nOK, so we failed to pass through --no-verify, because it got caught as a\nprefix of --verify-signatures, since the outer parse-options didn't know\nabout it. Makes sense, and I suppose this has been broken since\n11b6d17801 (pull: pass git-merge's options to git-merge, 2015-06-14).\n\nI was going to ask whether this should be passing through \"verify\", and\nallowing its \"no-\" variant, but there is no \"--verify\" in git-merge.\nArguably there should be (for consistency and to countermand an earlier\n--no-verify), but that is outside the scope of your fix (sadly if\nsomebody does change that, they'll have to remember to touch this spot,\ntoo, but I don't think it can be helped).\n\n> +test_expect_success 'git pull --no-verify flag passed to merge' '\n> +\ttest_when_finished \"rm -fr src dst actual\" &&\n> +\tgit init src &&\n> +\ttest_commit -C src one &&\n> +\tgit clone src dst &&\n> +\techo false >dst/.git/hooks/commit-msg &&\n> +\tchmod +x dst/.git/hooks/commit-msg &&\n\nThis script without #! should work portably, I think, though we\ngenerally prefer using the helper (which also handles the chmod):\n\n write_script dst/.git/hooks/commit-msg <<-\\EOF\n false\n EOF\n\nOther than that nit, this looks good to me.\n\n-Peff\n"},{"id":"439710","messageId":"YXjzGzcXS/zPgk0W@pflmari","threadId":"56787","inReplyTo":"YXhwGQOTfD+ypbo8@coredump.intra.peff.net","subject":"[PATCH v2] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-27T06:35:07Z","receivedAt":"2021-10-27T06:35:24Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"The option is incorrectly translated to \"--no-verify-signatures\",\nwhich causes the unexpected effect of the hook being called.\nAnd an even more unexpected effect of disabling verification\nof signatures.\n\nThe manual page describes the option to behave same as the similarly\nnamed option of \"git merge\", which seems to be the original intention\nof this option in the \"pull\" command.\n\nSigned-off-by: Alexander Riesen \n---\n\nJeff King, Tue, Oct 26, 2021 23:16:09 +0200:\n> On Tue, Oct 26, 2021 at 02:11:22PM +0200, Alex Riesen wrote:\n> > +test_expect_success 'git pull --no-verify flag passed to merge' '\n> > +\ttest_when_finished \"rm -fr src dst actual\" &&\n> > +\tgit init src &&\n> > +\ttest_commit -C src one &&\n> > +\tgit clone src dst &&\n> > +\techo false >dst/.git/hooks/commit-msg &&\n> > +\tchmod +x dst/.git/hooks/commit-msg &&\n> \n> This script without #! should work portably, I think, though we\n> generally prefer using the helper (which also handles the chmod):\n> \n> write_script dst/.git/hooks/commit-msg <<-\\EOF\n> false\n> EOF\n> \n> Other than that nit, this looks good to me.\n\nUpdated. Certainly looks nicer.\n\n builtin/pull.c | 6 ++++++\n t/t5521-pull-options.sh | 12 ++++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 425950f469..428baea95b 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -84,6 +84,7 @@ static char *opt_edit;\n static char *cleanup_arg;\n static char *opt_ff;\n static char *opt_verify_signatures;\n+static char *opt_no_verify;\n static int opt_autostash = -1;\n static int config_autostash;\n static int check_trust_level = 1;\n@@ -160,6 +161,9 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n \t\tN_(\"abort if fast-forward is not possible\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n+\tOPT_PASSTHRU(0, \"no-verify\", &opt_no_verify, NULL,\n+\t\tN_(\"bypass pre-merge-commit and commit-msg hooks\"),\n+\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n@@ -688,6 +692,8 @@ static int run_merge(void)\n \t\tstrvec_pushf(&args, \"--cleanup=%s\", cleanup_arg);\n \tif (opt_ff)\n \t\tstrvec_push(&args, opt_ff);\n+\tif (opt_no_verify)\n+\t\tstrvec_push(&args, opt_no_verify);\n \tif (opt_verify_signatures)\n \t\tstrvec_push(&args, opt_verify_signatures);\n \tstrvec_pushv(&args, opt_strategies.v);\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex db1a381cd9..7d3a8ae0d3 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -225,4 +225,16 @@ test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'git pull --no-verify flag passed to merge' '\n+\ttest_when_finished \"rm -fr src dst actual\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\twrite_script dst/.git/hooks/commit-msg <<-\\EOF &&\n+\tfalse\n+\tEOF\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --no-ff --no-verify\n+'\n+\n test_done\n-- \n2.33.0.22.g8cd9218530\n\n"},{"id":"439748","messageId":"YXkWlKVVUM9guAhe@coredump.intra.peff.net","threadId":"56787","inReplyTo":"YXjzGzcXS/zPgk0W@pflmari","subject":"Re: [PATCH v2] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-27T09:06:28Z","receivedAt":"2021-10-27T09:06:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 27, 2021 at 08:35:07AM +0200, Alex Riesen wrote:\n\n> The option is incorrectly translated to \"--no-verify-signatures\",\n> which causes the unexpected effect of the hook being called.\n> And an even more unexpected effect of disabling verification\n> of signatures.\n> \n> The manual page describes the option to behave same as the similarly\n> named option of \"git merge\", which seems to be the original intention\n> of this option in the \"pull\" command.\n\nThanks, this looks good to me.\n\n-Peff\n"},{"id":"439770","messageId":"YXlBhmfXl3wFQ5Bj@pflmari","threadId":"56787","inReplyTo":"YXhwGQOTfD+ypbo8@coredump.intra.peff.net","subject":"Re: [PATCH] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-27T12:09:42Z","receivedAt":"2021-10-27T12:10:00Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Jeff King, Tue, Oct 26, 2021 23:16:09 +0200:\n> On Tue, Oct 26, 2021 at 02:11:22PM +0200, Alex Riesen wrote:\n> I was going to ask whether this should be passing through \"verify\", and\n> allowing its \"no-\" variant, but there is no \"--verify\" in git-merge.\n> Arguably there should be (for consistency and to countermand an earlier\n> --no-verify), but that is outside the scope of your fix (sadly if\n> somebody does change that, they'll have to remember to touch this spot,\n> too, but I don't think it can be helped).\n\nThis seems simple enough, though. Like this?\n\n[PATCH] Remove negation from the merge option \"--no-verify\"\n\nThis allows re-enabling hooks disabled by an earlier \"--no-verify\"\nin command-line and makes the interface more consistent.\n---\n Documentation/git-merge.txt | 2 +-\n Documentation/merge-options.txt | 5 +++--\n builtin/merge.c | 12 ++++++------\n builtin/pull.c | 12 ++++++------\n 4 files changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex 3819fadac1..324ae879d2 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]\n-\t[--no-verify] [-s ] [-X ] [-S[]]\n+\t[--[no-]verify] [-s ] [-X ] [-S[]]\n \t[--[no-]allow-unrelated-histories]\n \t[--[no-]rerere-autoupdate] [-m ] [-F ] [...]\n 'git merge' (--continue | --abort | --quit)\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 80d4831662..54cd3b04df 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -112,8 +112,9 @@ option can be used to override --squash.\n +\n With --squash, --commit is not allowed, and will fail.\n \n---no-verify::\n-\tThis option bypasses the pre-merge and commit-msg hooks.\n+--[no-]verify::\n+\tWith `--no-verify`, bypass the pre-merge and commit-msg hooks,\n+\twhich will be run by default.\n \tSee also linkgit:githooks[5].\n \n -s ::\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 9d5359edc2..ab5c221234 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -83,7 +83,7 @@ static int default_to_upstream = 1;\n static int signoff;\n static const char *sign_commit;\n static int autostash;\n-static int no_verify;\n+static int verify = 1;\n \n static struct strategy all_strategy[] = {\n \t{ \"recursive\", DEFAULT_TWOHEAD | NO_TRIVIAL },\n@@ -290,7 +290,7 @@ static struct option builtin_merge_options[] = {\n \tOPT_AUTOSTASH(&autostash),\n \tOPT_BOOL(0, \"overwrite-ignore\", &overwrite_ignore, N_(\"update ignored files (default)\")),\n \tOPT_BOOL(0, \"signoff\", &signoff, N_(\"add Signed-off-by:\")),\n-\tOPT_BOOL(0, \"no-verify\", &no_verify, N_(\"bypass pre-merge-commit and commit-msg hooks\")),\n+\tOPT_BOOL(0, \"verify\", &verify, N_(\"control use of pre-merge-commit and commit-msg hooks\")),\n \tOPT_END()\n };\n \n@@ -822,7 +822,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *index_file = get_index_file();\n \n-\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n+\tif (verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \t/*\n \t * Re-read the index as pre-merge-commit hook could have updated it,\n@@ -858,9 +858,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\t\tabort_commit(remoteheads, NULL);\n \t}\n \n-\tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n-\t\t\t\t\t \"commit-msg\",\n-\t\t\t\t\t git_path_merge_msg(the_repository), NULL))\n+\tif (verify && run_commit_hook(0 < option_edit, get_index_file(),\n+\t\t\t\t \"commit-msg\",\n+\t\t\t\t git_path_merge_msg(the_repository), NULL))\n \t\tabort_commit(remoteheads, NULL);\n \n \tread_merge_msg(&msg);\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 428baea95b..e783da10b2 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -84,7 +84,7 @@ static char *opt_edit;\n static char *cleanup_arg;\n static char *opt_ff;\n static char *opt_verify_signatures;\n-static char *opt_no_verify;\n+static char *opt_verify;\n static int opt_autostash = -1;\n static int config_autostash;\n static int check_trust_level = 1;\n@@ -161,9 +161,9 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n \t\tN_(\"abort if fast-forward is not possible\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n-\tOPT_PASSTHRU(0, \"no-verify\", &opt_no_verify, NULL,\n-\t\tN_(\"bypass pre-merge-commit and commit-msg hooks\"),\n-\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n+\tOPT_PASSTHRU(0, \"verify\", &opt_verify, NULL,\n+\t\tN_(\"control use of pre-merge-commit and commit-msg hooks\"),\n+\t\tPARSE_OPT_NOARG),\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n@@ -692,8 +692,8 @@ static int run_merge(void)\n \t\tstrvec_pushf(&args, \"--cleanup=%s\", cleanup_arg);\n \tif (opt_ff)\n \t\tstrvec_push(&args, opt_ff);\n-\tif (opt_no_verify)\n-\t\tstrvec_push(&args, opt_no_verify);\n+\tif (opt_verify)\n+\t\tstrvec_push(&args, opt_verify);\n \tif (opt_verify_signatures)\n \t\tstrvec_push(&args, opt_verify_signatures);\n \tstrvec_pushv(&args, opt_strategies.v);\n-- \n2.33.0.22.g8cd9218530\n\n\n"},{"id":"439771","messageId":"YXlD5ecNSdeBSMoS@coredump.intra.peff.net","threadId":"56787","inReplyTo":"YXlBhmfXl3wFQ5Bj@pflmari","subject":"Re: [PATCH] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-27T12:19:49Z","receivedAt":"2021-10-27T12:19:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 27, 2021 at 02:09:42PM +0200, Alex Riesen wrote:\n\n> Jeff King, Tue, Oct 26, 2021 23:16:09 +0200:\n> > On Tue, Oct 26, 2021 at 02:11:22PM +0200, Alex Riesen wrote:\n> > I was going to ask whether this should be passing through \"verify\", and\n> > allowing its \"no-\" variant, but there is no \"--verify\" in git-merge.\n> > Arguably there should be (for consistency and to countermand an earlier\n> > --no-verify), but that is outside the scope of your fix (sadly if\n> > somebody does change that, they'll have to remember to touch this spot,\n> > too, but I don't think it can be helped).\n> \n> This seems simple enough, though. Like this?\n> \n> [PATCH] Remove negation from the merge option \"--no-verify\"\n> \n> This allows re-enabling hooks disabled by an earlier \"--no-verify\"\n> in command-line and makes the interface more consistent.\n\nYeah, I don't see any problems in the patch below, and I agree it makes\nthings overall nicer (both the user-facing parts, and not having to see\nthe double-negative \"!no_verify\" in the code).\n\n> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> index 80d4831662..54cd3b04df 100644\n> --- a/Documentation/merge-options.txt\n> +++ b/Documentation/merge-options.txt\n> @@ -112,8 +112,9 @@ option can be used to override --squash.\n> +\n> With --squash, --commit is not allowed, and will fail.\n> \n> ---no-verify::\n> -\tThis option bypasses the pre-merge and commit-msg hooks.\n> +--[no-]verify::\n> +\tWith `--no-verify`, bypass the pre-merge and commit-msg hooks,\n> +\twhich will be run by default.\n\nThis \"which will be run by default\" is a little awkward. Maybe:\n\n By default, pre-merge and commit-msg hooks are run. When `--no-verify`\n is given, these are bypassed.\n\n?\n\n-Peff\n"},{"id":"439774","messageId":"YXlTpzrY7KFqRlno@pflmari","threadId":"56787","inReplyTo":"YXlD5ecNSdeBSMoS@coredump.intra.peff.net","subject":"[PATCH] Remove negation from the merge option \"--no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-27T13:27:03Z","receivedAt":"2021-10-27T13:27:29Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"From: Alex Riesen \n\nThis allows re-enabling hooks disabled by an earlier \"--no-verify\"\nin command-line and makes the interface more consistent.\n\nSigned-off-by: Alex Riesen \n\n---\n\nThis one is on top of \"[PATCH] Fix \"commit-msg\" hook unexpectedly called for\n\"git pull --no-verify\" (http://public-inbox.org/git/YXfwanz3MynCLDmn@pflmari/).\nWhich is a bit awkward. Should I resend as series?\n\nJeff King, Wed, Oct 27, 2021 14:19:49 +0200:\n> On Wed, Oct 27, 2021 at 02:09:42PM +0200, Alex Riesen wrote:\n> > Jeff King, Tue, Oct 26, 2021 23:16:09 +0200:\n> > > On Tue, Oct 26, 2021 at 02:11:22PM +0200, Alex Riesen wrote:\n> > > I was going to ask whether this should be passing through \"verify\", and\n> > > allowing its \"no-\" variant, but there is no \"--verify\" in git-merge.\n> > > Arguably there should be (for consistency and to countermand an earlier\n> > > --no-verify), but that is outside the scope of your fix (sadly if\n> > > somebody does change that, they'll have to remember to touch this spot,\n> > > too, but I don't think it can be helped).\n> > \n> > This seems simple enough, though. Like this?\n> > \n> > [PATCH] Remove negation from the merge option \"--no-verify\"\n> > \n> > This allows re-enabling hooks disabled by an earlier \"--no-verify\"\n> > in command-line and makes the interface more consistent.\n> \n> Yeah, I don't see any problems in the patch below, and I agree it makes\n> things overall nicer (both the user-facing parts, and not having to see\n> the double-negative \"!no_verify\" in the code).\n\nOk, resending it formally.\n\n> > diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> > index 80d4831662..54cd3b04df 100644\n> > --- a/Documentation/merge-options.txt\n> > +++ b/Documentation/merge-options.txt\n> > @@ -112,8 +112,9 @@ option can be used to override --squash.\n> > +\n> > With --squash, --commit is not allowed, and will fail.\n> > \n> > ---no-verify::\n> > -\tThis option bypasses the pre-merge and commit-msg hooks.\n> > +--[no-]verify::\n> > +\tWith `--no-verify`, bypass the pre-merge and commit-msg hooks,\n> > +\twhich will be run by default.\n> \n> This \"which will be run by default\" is a little awkward. Maybe:\n> \n> By default, pre-merge and commit-msg hooks are run. When `--no-verify`\n> is given, these are bypassed.\n> \n> ?\n\nOf course. It certainly reads better like this.\n\n Documentation/git-merge.txt | 2 +-\n Documentation/merge-options.txt | 5 +++--\n builtin/merge.c | 12 ++++++------\n builtin/pull.c | 12 ++++++------\n 4 files changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex 3819fadac1..324ae879d2 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]\n-\t[--no-verify] [-s ] [-X ] [-S[]]\n+\t[--[no-]verify] [-s ] [-X ] [-S[]]\n \t[--[no-]allow-unrelated-histories]\n \t[--[no-]rerere-autoupdate] [-m ] [-F ] [...]\n 'git merge' (--continue | --abort | --quit)\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 80d4831662..f8016b0f7b 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -112,8 +112,9 @@ option can be used to override --squash.\n +\n With --squash, --commit is not allowed, and will fail.\n \n---no-verify::\n-\tThis option bypasses the pre-merge and commit-msg hooks.\n+--[no-]verify::\n+\tBy default, pre-merge and commit-msg hooks are run. When `--no-verify`\n+\tis given, these are bypassed.\n \tSee also linkgit:githooks[5].\n \n -s ::\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 9d5359edc2..ab5c221234 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -83,7 +83,7 @@ static int default_to_upstream = 1;\n static int signoff;\n static const char *sign_commit;\n static int autostash;\n-static int no_verify;\n+static int verify = 1;\n \n static struct strategy all_strategy[] = {\n \t{ \"recursive\", DEFAULT_TWOHEAD | NO_TRIVIAL },\n@@ -290,7 +290,7 @@ static struct option builtin_merge_options[] = {\n \tOPT_AUTOSTASH(&autostash),\n \tOPT_BOOL(0, \"overwrite-ignore\", &overwrite_ignore, N_(\"update ignored files (default)\")),\n \tOPT_BOOL(0, \"signoff\", &signoff, N_(\"add Signed-off-by:\")),\n-\tOPT_BOOL(0, \"no-verify\", &no_verify, N_(\"bypass pre-merge-commit and commit-msg hooks\")),\n+\tOPT_BOOL(0, \"verify\", &verify, N_(\"control use of pre-merge-commit and commit-msg hooks\")),\n \tOPT_END()\n };\n \n@@ -822,7 +822,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *index_file = get_index_file();\n \n-\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n+\tif (verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \t/*\n \t * Re-read the index as pre-merge-commit hook could have updated it,\n@@ -858,9 +858,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\t\tabort_commit(remoteheads, NULL);\n \t}\n \n-\tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n-\t\t\t\t\t \"commit-msg\",\n-\t\t\t\t\t git_path_merge_msg(the_repository), NULL))\n+\tif (verify && run_commit_hook(0 < option_edit, get_index_file(),\n+\t\t\t\t \"commit-msg\",\n+\t\t\t\t git_path_merge_msg(the_repository), NULL))\n \t\tabort_commit(remoteheads, NULL);\n \n \tread_merge_msg(&msg);\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 428baea95b..e783da10b2 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -84,7 +84,7 @@ static char *opt_edit;\n static char *cleanup_arg;\n static char *opt_ff;\n static char *opt_verify_signatures;\n-static char *opt_no_verify;\n+static char *opt_verify;\n static int opt_autostash = -1;\n static int config_autostash;\n static int check_trust_level = 1;\n@@ -161,9 +161,9 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n \t\tN_(\"abort if fast-forward is not possible\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n-\tOPT_PASSTHRU(0, \"no-verify\", &opt_no_verify, NULL,\n-\t\tN_(\"bypass pre-merge-commit and commit-msg hooks\"),\n-\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n+\tOPT_PASSTHRU(0, \"verify\", &opt_verify, NULL,\n+\t\tN_(\"control use of pre-merge-commit and commit-msg hooks\"),\n+\t\tPARSE_OPT_NOARG),\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n@@ -692,8 +692,8 @@ static int run_merge(void)\n \t\tstrvec_pushf(&args, \"--cleanup=%s\", cleanup_arg);\n \tif (opt_ff)\n \t\tstrvec_push(&args, opt_ff);\n-\tif (opt_no_verify)\n-\t\tstrvec_push(&args, opt_no_verify);\n+\tif (opt_verify)\n+\t\tstrvec_push(&args, opt_verify);\n \tif (opt_verify_signatures)\n \t\tstrvec_push(&args, opt_verify_signatures);\n \tstrvec_pushv(&args, opt_strategies.v);\n-- \n2.33.0.22.g8cd9218530\n\n"},{"id":"439826","messageId":"xmqq8ryew7jq.fsf@gitster.g","threadId":"56787","inReplyTo":"YXhwGQOTfD+ypbo8@coredump.intra.peff.net","subject":"Re: [PATCH] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-27T20:12:57Z","receivedAt":"2021-10-27T20:13:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> OK, so we failed to pass through --no-verify, because it got caught as a\n> prefix of --verify-signatures, since the outer parse-options didn't know\n> about it. Makes sense, and I suppose this has been broken since\n> 11b6d17801 (pull: pass git-merge's options to git-merge, 2015-06-14).\n>\n> I was going to ask whether this should be passing through \"verify\", and\n> allowing its \"no-\" variant, but there is no \"--verify\" in git-merge.\n> Arguably there should be (for consistency and to countermand an earlier\n> --no-verify), but that is outside the scope of your fix (sadly if\n> somebody does change that, they'll have to remember to touch this spot,\n> too, but I don't think it can be helped).\n\nWe do not even have \"--verify\" in \"git commit\", because letting the\nhooks to interfere is the default, but if we were designing it\ntoday, we probably would add \"--verify\" to override a \"--no-verify\"\nearlier on the command line, so it is not implausible that people\nwould want to add \"--verify\" to \"git commit\" and \"git merge\" in the\nfuture.\n\nWe can add two hunks, one for builtin/merge.c and another for\nbuiltin/pull.c, to leave a note for future developers and it would\nhelp quite a lot, I would presume.\n\nThanks.\n"},{"id":"439827","messageId":"xmqq4k92w7do.fsf@gitster.g","threadId":"56787","inReplyTo":"YXlTpzrY7KFqRlno@pflmari","subject":"Re: [PATCH] Remove negation from the merge option \"--no-verify\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-27T20:16:35Z","receivedAt":"2021-10-27T20:16:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> From: Alex Riesen \n>\n> This allows re-enabling hooks disabled by an earlier \"--no-verify\"\n> in command-line and makes the interface more consistent.\n>\n> Signed-off-by: Alex Riesen \n>\n> ---\n>\n> This one is on top of \"[PATCH] Fix \"commit-msg\" hook unexpectedly called for\n> \"git pull --no-verify\" (http://public-inbox.org/git/YXfwanz3MynCLDmn@pflmari/).\n> Which is a bit awkward. Should I resend as series?\n\nDon't we need to do this at the root cause command \"git commit\"? It\nis documented to take \"--no-verify\" but not \"--verify\" to countermand\nan earlier \"--no-verify\" on the command line.\n\nAnd yes, I agree that we shouldn't introduce an awkwardness in one\nstep of the series and fix it in another step of the same series.\n"},{"id":"439857","messageId":"YXpFTJTo0pKhM7xG@pflmari","threadId":"56787","inReplyTo":"xmqq4k92w7do.fsf@gitster.g","subject":"Re: [PATCH] Remove negation from the merge option \"--no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-28T06:38:04Z","receivedAt":"2021-10-28T06:38:23Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Wed, Oct 27, 2021 22:16:35 +0200:\n> Alex Riesen writes:\n> \n> > From: Alex Riesen \n> >\n> > This allows re-enabling hooks disabled by an earlier \"--no-verify\"\n> > in command-line and makes the interface more consistent.\n> >\n> > Signed-off-by: Alex Riesen \n> >\n> > ---\n> >\n> > This one is on top of \"[PATCH] Fix \"commit-msg\" hook unexpectedly called for\n> > \"git pull --no-verify\" (http://public-inbox.org/git/YXfwanz3MynCLDmn@pflmari/).\n> > Which is a bit awkward. Should I resend as series?\n> \n> Don't we need to do this at the root cause command \"git commit\"?\n\nThe commit preparing code in builtin/merge.c does not seem to use the code\nfrom builtin/commit.c, so it does not look like a direct cause of that effect\nin \"git merge\". But...\n\n> It is documented to take \"--no-verify\" but not \"--verify\" to countermand an\n> earlier \"--no-verify\" on the command line.\n\nThis particular peculiarity in implementation of \"--[no-]verify\" does look\nlike it has root-caused everything :)\n\n> And yes, I agree that we shouldn't introduce an awkwardness in one\n> step of the series and fix it in another step of the same series.\n\nI think I better resend everything as a single patch then.\n\n"},{"id":"439861","messageId":"YXpZddOixrJDd//s@pflmari","threadId":"56787","inReplyTo":"YXpFTJTo0pKhM7xG@pflmari","subject":"[PATCH] Remove negation from the commit and merge option \"--no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-28T08:04:05Z","receivedAt":"2021-10-28T08:04:21Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"From: Alex Riesen \n\nThis allows re-enabling of the hooks disabled by an earlier \"--no-verify\"\nin command-line and makes the interface more consistent.\n\nIncidentally, this also fixes unexpected calling of the hooks by \"git\npull\" when \"--no-verify\" was specified, where it was incorrectly\ntranslated to \"--no-verify-signatures\". This caused the unexpected\neffect of the hooks being called. And an even more unexpected effect of\ndisabling verification of signatures.\n\nSigned-off-by: Alexander Riesen \n---\nAlex Riesen, Thu, Oct 28, 2021 08:38:04 +0200:\n> Junio C Hamano, Wed, Oct 27, 2021 22:16:35 +0200:\n> > And yes, I agree that we shouldn't introduce an awkwardness in one\n> > step of the series and fix it in another step of the same series.\n> \n> I think I better resend everything as a single patch then.\n> \n\nSomething like this: dengate no-verify in all commit-creating code.\n\nI looked at \"no-verify\" in push and rebase, but they feel different:\nthey create no new commits, and are involved in other workflows.\nPerhaps another time.\n\n Documentation/git-commit.txt | 10 ++++++++--\n Documentation/git-merge.txt | 2 +-\n Documentation/merge-options.txt | 5 +++--\n builtin/commit.c | 22 +++++++++++++++++-----\n builtin/merge.c | 12 ++++++------\n builtin/pull.c | 6 ++++++\n t/t5521-pull-options.sh | 12 ++++++++++++\n t/t7504-commit-msg-hook.sh | 8 ++++++++\n 8 files changed, 61 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex a3baea32ae..ba66209274 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u] [--amend]\n \t [--dry-run] [(-c | -C | --fixup | --squash) ]\n \t [-F | -m ] [--reset-author] [--allow-empty]\n-\t [--allow-empty-message] [--no-verify] [-e] [--author=]\n+\t [--allow-empty-message] [--[no-]verify] [-e] [--author=]\n \t [--date=] [--cleanup=] [--[no-]status]\n \t [-i | -o] [--pathspec-from-file= [--pathspec-file-nul]]\n \t [-S[]] [--] [...]\n@@ -174,7 +174,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n \n -n::\n --no-verify::\n-\tThis option bypasses the pre-commit and commit-msg hooks.\n+\tBy default, pre-merge and commit-msg hooks are run. When one of these\n+\toptions is given, these are bypassed.\n+\tSee also linkgit:githooks[5].\n+\n+--verify::\n+\tThis option re-enables running of the pre-commit and commit-msg hooks\n+\tafter an earlier `-n` or `--no-verify`.\n \tSee also linkgit:githooks[5].\n \n --allow-empty::\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex 3819fadac1..324ae879d2 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]\n-\t[--no-verify] [-s ] [-X ] [-S[]]\n+\t[--[no-]verify] [-s ] [-X ] [-S[]]\n \t[--[no-]allow-unrelated-histories]\n \t[--[no-]rerere-autoupdate] [-m ] [-F ] [...]\n 'git merge' (--continue | --abort | --quit)\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 80d4831662..f8016b0f7b 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -112,8 +112,9 @@ option can be used to override --squash.\n +\n With --squash, --commit is not allowed, and will fail.\n \n---no-verify::\n-\tThis option bypasses the pre-merge and commit-msg hooks.\n+--[no-]verify::\n+\tBy default, pre-merge and commit-msg hooks are run. When `--no-verify`\n+\tis given, these are bypassed.\n \tSee also linkgit:githooks[5].\n \n -s ::\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1dfd799ec5..714722b0cd 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -108,7 +108,7 @@ static char *edit_message, *use_message;\n static char *fixup_message, *squash_message;\n static int all, also, interactive, patch_interactive, only, amend, signoff;\n static int edit_flag = -1; /* unspecified */\n-static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;\n+static int quiet, verbose, verify = 1, allow_empty, dry_run, renew_authorship;\n static int config_commit_verbose = -1; /* unspecified */\n static int no_post_rewrite, allow_empty_message, pathspec_file_nul;\n static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;\n@@ -164,6 +164,16 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int opt_parse_n(const struct option *opt, const char *arg, int unset)\n+{\n+\tint *value = opt->value;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\n+\t*value = 0;\n+\treturn 0;\n+}\n+\n static int opt_parse_rename_score(const struct option *opt, const char *arg, int unset)\n {\n \tconst char **value = opt->value;\n@@ -699,7 +709,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n-\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n+\tif (verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n \t\treturn 0;\n \n \tif (squash_message) {\n@@ -983,7 +993,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (!no_verify && find_hook(\"pre-commit\")) {\n+\tif (verify && find_hook(\"pre-commit\")) {\n \t\t/*\n \t\t * Re-read the index as pre-commit hook could have updated it,\n \t\t * and write it out as a tree. We must do this before we invoke\n@@ -1014,7 +1024,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstrvec_clear(&env);\n \t}\n \n-\tif (!no_verify &&\n+\tif (verify &&\n \t run_commit_hook(use_editor, index_file, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n \t\treturn 0;\n \t}\n@@ -1522,7 +1532,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"interactive\", &interactive, N_(\"interactively add files\")),\n \t\tOPT_BOOL('p', \"patch\", &patch_interactive, N_(\"interactively add changes\")),\n \t\tOPT_BOOL('o', \"only\", &only, N_(\"commit only specified files\")),\n-\t\tOPT_BOOL('n', \"no-verify\", &no_verify, N_(\"bypass pre-commit and commit-msg hooks\")),\n+\t\tOPT_CALLBACK_F('n', NULL, &verify, \"\", N_(\"bypass pre-commit and commit-msg hooks\"),\n+\t\t\t PARSE_OPT_NOARG|PARSE_OPT_NONEG, opt_parse_n),\n+\t\tOPT_BOOL(0, \"verify\", &verify, N_(\"control use of pre-commit and commit-msg hooks\")),\n \t\tOPT_BOOL(0, \"dry-run\", &dry_run, N_(\"show what would be committed\")),\n \t\tOPT_SET_INT(0, \"short\", &status_format, N_(\"show status concisely\"),\n \t\t\t STATUS_FORMAT_SHORT),\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 9d5359edc2..ab5c221234 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -83,7 +83,7 @@ static int default_to_upstream = 1;\n static int signoff;\n static const char *sign_commit;\n static int autostash;\n-static int no_verify;\n+static int verify = 1;\n \n static struct strategy all_strategy[] = {\n \t{ \"recursive\", DEFAULT_TWOHEAD | NO_TRIVIAL },\n@@ -290,7 +290,7 @@ static struct option builtin_merge_options[] = {\n \tOPT_AUTOSTASH(&autostash),\n \tOPT_BOOL(0, \"overwrite-ignore\", &overwrite_ignore, N_(\"update ignored files (default)\")),\n \tOPT_BOOL(0, \"signoff\", &signoff, N_(\"add Signed-off-by:\")),\n-\tOPT_BOOL(0, \"no-verify\", &no_verify, N_(\"bypass pre-merge-commit and commit-msg hooks\")),\n+\tOPT_BOOL(0, \"verify\", &verify, N_(\"control use of pre-merge-commit and commit-msg hooks\")),\n \tOPT_END()\n };\n \n@@ -822,7 +822,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *index_file = get_index_file();\n \n-\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n+\tif (verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \t/*\n \t * Re-read the index as pre-merge-commit hook could have updated it,\n@@ -858,9 +858,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\t\tabort_commit(remoteheads, NULL);\n \t}\n \n-\tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n-\t\t\t\t\t \"commit-msg\",\n-\t\t\t\t\t git_path_merge_msg(the_repository), NULL))\n+\tif (verify && run_commit_hook(0 < option_edit, get_index_file(),\n+\t\t\t\t \"commit-msg\",\n+\t\t\t\t git_path_merge_msg(the_repository), NULL))\n \t\tabort_commit(remoteheads, NULL);\n \n \tread_merge_msg(&msg);\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 425950f469..e783da10b2 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -84,6 +84,7 @@ static char *opt_edit;\n static char *cleanup_arg;\n static char *opt_ff;\n static char *opt_verify_signatures;\n+static char *opt_verify;\n static int opt_autostash = -1;\n static int config_autostash;\n static int check_trust_level = 1;\n@@ -160,6 +161,9 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n \t\tN_(\"abort if fast-forward is not possible\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n+\tOPT_PASSTHRU(0, \"verify\", &opt_verify, NULL,\n+\t\tN_(\"control use of pre-merge-commit and commit-msg hooks\"),\n+\t\tPARSE_OPT_NOARG),\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n@@ -688,6 +692,8 @@ static int run_merge(void)\n \t\tstrvec_pushf(&args, \"--cleanup=%s\", cleanup_arg);\n \tif (opt_ff)\n \t\tstrvec_push(&args, opt_ff);\n+\tif (opt_verify)\n+\t\tstrvec_push(&args, opt_verify);\n \tif (opt_verify_signatures)\n \t\tstrvec_push(&args, opt_verify_signatures);\n \tstrvec_pushv(&args, opt_strategies.v);\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex db1a381cd9..7d3a8ae0d3 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -225,4 +225,16 @@ test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'git pull --no-verify flag passed to merge' '\n+\ttest_when_finished \"rm -fr src dst actual\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\twrite_script dst/.git/hooks/commit-msg <<-\\EOF &&\n+\tfalse\n+\tEOF\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --no-ff --no-verify\n+'\n+\n test_done\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 31b9c6a2c1..166ff5fb26 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '\n \n '\n \n+test_expect_success '-n with failing hook' '\n+\n+\techo \"more\" >> file &&\n+\tgit add file &&\n+\tgit commit -n -m \"more\"\n+\n+'\n+\n test_expect_success '--no-verify with failing hook (editor)' '\n \n \techo \"more stuff\" >> file &&\n-- \n2.33.0.22.g8cd9218530\n\n"},{"id":"439869","messageId":"edca7f6b-e89c-7efa-c6f5-2c3aaaea54f9@gmail.com","threadId":"56787","inReplyTo":"YXpZddOixrJDd//s@pflmari","subject":"Re: [PATCH] Remove negation from the commit and merge option \"--no-verify\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-10-28T13:57:58Z","receivedAt":"2021-10-28T13:58:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 28/10/2021 09:04, Alex Riesen wrote:\n> From: Alex Riesen \n> \n> This allows re-enabling of the hooks disabled by an earlier \"--no-verify\"\n> in command-line and makes the interface more consistent.\n\nThanks for working on this. Since 0f1930c587 (\"parse-options: allow \npositivation of options starting, with no-\", 2012-02-25) merge and \ncommit have accepted \"--verify\" but it is undocumented. The \ndocumentation updates and fix to pull in this patch are very welcome, \nbut I'm not sure we need the other changes. I've left a couple of \ncomments below.\n\n[As an aside we should probably improve the documentation in \nparse-options.h if both Peff and Junio did not know how it handles \n\"--no-foo\" but that is outside the scope of this patch]\n\n> Incidentally, this also fixes unexpected calling of the hooks by \"git\n> pull\" when \"--no-verify\" was specified, where it was incorrectly\n> translated to \"--no-verify-signatures\". This caused the unexpected\n> effect of the hooks being called. And an even more unexpected effect of\n> disabling verification of signatures.\n\nOuch!\n\n> Signed-off-by: Alexander Riesen \n>[...]\n> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\n> index a3baea32ae..ba66209274 100644\n> --- a/Documentation/git-commit.txt\n> +++ b/Documentation/git-commit.txt\n> @@ -11,7 +11,7 @@ SYNOPSIS\n> 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u] [--amend]\n> \t [--dry-run] [(-c | -C | --fixup | --squash) ]\n> \t [-F | -m ] [--reset-author] [--allow-empty]\n> -\t [--allow-empty-message] [--no-verify] [-e] [--author=]\n> +\t [--allow-empty-message] [--[no-]verify] [-e] [--author=]\n\nI think for the synopsis it is fine just to list the most common \noptions. Having --no-verify without the [no-] makes it clear that \n--verify is the default so is not a commonly used option.\n\n> \t [--date=] [--cleanup=] [--[no-]status]\n> \t [-i | -o] [--pathspec-from-file= [--pathspec-file-nul]]\n> \t [-S[]] [--] [...]\n> @@ -174,7 +174,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n> \n> -n::\n> --no-verify::\n> -\tThis option bypasses the pre-commit and commit-msg hooks.\n> +\tBy default, pre-merge and commit-msg hooks are run. When one of these\n\nI think saying \"the pre-merge and commit-msg hooks\" would be clearer as \nyou do below.\n\n> +\toptions is given, these are bypassed.\n> +\tSee also linkgit:githooks[5].\n> +\n> +--verify::\n> +\tThis option re-enables running of the pre-commit and commit-msg hooks\n> +\tafter an earlier `-n` or `--no-verify`.\n> \tSee also linkgit:githooks[5].\n\nSome of the existing documentation describes the \"--no-foo\" option with \n\"--foo\" (e.g --[no-]signoff) but in other places we list the two options \nseparately (e.g. --[no-]edit), I'd lean towards combining them as you \nhave done for the merge documentation but I don't feel strongly about it.\n\n> --allow-empty::\n> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\n> index 3819fadac1..324ae879d2 100644\n> --- a/Documentation/git-merge.txt\n> +++ b/Documentation/git-merge.txt\n> @@ -10,7 +10,7 @@ SYNOPSIS\n> --------\n> [verse]\n> 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]\n> -\t[--no-verify] [-s ] [-X ] [-S[]]\n> +\t[--[no-]verify] [-s ] [-X ] [-S[]]\n\nAgain I'm not sure changing the synopsis makes things clearer.\n\n> \t[--[no-]allow-unrelated-histories]\n> \t[--[no-]rerere-autoupdate] [-m ] [-F ] [...]\n> 'git merge' (--continue | --abort | --quit)\n> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> index 80d4831662..f8016b0f7b 100644\n> --- a/Documentation/merge-options.txt\n> +++ b/Documentation/merge-options.txt\n> @@ -112,8 +112,9 @@ option can be used to override --squash.\n> +\n> With --squash, --commit is not allowed, and will fail.\n> \n> ---no-verify::\n> -\tThis option bypasses the pre-merge and commit-msg hooks.\n> +--[no-]verify::\n> +\tBy default, pre-merge and commit-msg hooks are run. When `--no-verify`\n\nI think \"the pre-merge ...\" would be better here as well.\n\n> +\tis given, these are bypassed.\n> \tSee also linkgit:githooks[5].\n>[...] \n> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\n> index db1a381cd9..7d3a8ae0d3 100755\n> --- a/t/t5521-pull-options.sh\n> +++ b/t/t5521-pull-options.sh\n> @@ -225,4 +225,16 @@ test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '\n> \ttest_must_be_empty actual\n> '\n> \n> +test_expect_success 'git pull --no-verify flag passed to merge' '\n> +\ttest_when_finished \"rm -fr src dst actual\" &&\n> +\tgit init src &&\n> +\ttest_commit -C src one &&\n> +\tgit clone src dst &&\n> +\twrite_script dst/.git/hooks/commit-msg <<-\\EOF &&\n> +\tfalse\n> +\tEOF\n> +\ttest_commit -C src two &&\n> +\tgit -C dst pull --no-ff --no-verify\n> +'\n\nThanks for adding a test\n\n> test_done\n> diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\n> index 31b9c6a2c1..166ff5fb26 100755\n> --- a/t/t7504-commit-msg-hook.sh\n> +++ b/t/t7504-commit-msg-hook.sh\n> @@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '\n> \n> '\n> \n> +test_expect_success '-n with failing hook' '\n> +\n> +\techo \"more\" >> file &&\n> +\tgit add file &&\n> +\tgit commit -n -m \"more\"\n> +\n> +'\n\nIs this to check that \"-n\" works like \"--no-verify\"?\n\nI think it would be very useful to add another test that checks \n\"--verify\" overrides \"--no-verify\".\n\nBest Wishes\n\nPhillip\n"},{"id":"439878","messageId":"YXrFaJXbuSuwfhQ7@pflmari","threadId":"56787","inReplyTo":"edca7f6b-e89c-7efa-c6f5-2c3aaaea54f9@gmail.com","subject":"Re: [PATCH] Remove negation from the commit and merge option \"--no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-28T15:44:40Z","receivedAt":"2021-10-28T15:45:08Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"From: Alex Riesen \n\nThis documents re-enabling of the hooks disabled by an earlier\n\"--no-verify\" in command-line.\n\nSigned-off-by: Alexander Riesen \n---\n\nHi Phillip,\n\nPhillip Wood, Thu, Oct 28, 2021 15:57:58 +0200:\n> On 28/10/2021 09:04, Alex Riesen wrote:\n> > From: Alex Riesen \n> > \n> > This allows re-enabling of the hooks disabled by an earlier \"--no-verify\"\n> > in command-line and makes the interface more consistent.\n> \n> Thanks for working on this. Since 0f1930c587 (\"parse-options: allow\n> positivation of options starting, with no-\", 2012-02-25) merge and commit\n> have accepted \"--verify\" but it is undocumented. The documentation updates\n> and fix to pull in this patch are very welcome, but I'm not sure we need the\n> other changes. I've left a couple of comments below.\n> \n> [As an aside we should probably improve the documentation in parse-options.h\n> if both Peff and Junio did not know how it handles \"--no-foo\" but that is\n> outside the scope of this patch]\n\nInteresting feature. It is unfortunate it was so well hidden. You're right, of\ncourse, and the newly added tests in t7504-commit-msg-hook.sh pass without any\nchanges to the \"builtin/commit.c\".\n\nRemoval of double-negation in the code was an improvement to its readability,\nbut I like small patches more.\n\nAlso, the series has no conflicts with 2.33.0 anymore and the \"git pull\" can\nbe applied independently.\n\n> > diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\n> > index a3baea32ae..ba66209274 100644\n> > --- a/Documentation/git-commit.txt\n> > +++ b/Documentation/git-commit.txt\n> > @@ -11,7 +11,7 @@ SYNOPSIS\n> > 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u] [--amend]\n> > \t [--dry-run] [(-c | -C | --fixup | --squash) ]\n> > \t [-F | -m ] [--reset-author] [--allow-empty]\n> > -\t [--allow-empty-message] [--no-verify] [-e] [--author=]\n> > +\t [--allow-empty-message] [--[no-]verify] [-e] [--author=]\n> \n> I think for the synopsis it is fine just to list the most common options.\n> Having --no-verify without the [no-] makes it clear that --verify is the\n> default so is not a commonly used option.\n\nYep, makes sense.\n\n> > \t [--date=] [--cleanup=] [--[no-]status]\n> > \t [-i | -o] [--pathspec-from-file= [--pathspec-file-nul]]\n> > \t [-S[]] [--] [...]\n> > @@ -174,7 +174,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n> > -n::\n> > --no-verify::\n> > -\tThis option bypasses the pre-commit and commit-msg hooks.\n> > +\tBy default, pre-merge and commit-msg hooks are run. When one of these\n> \n> I think saying \"the pre-merge and commit-msg hooks\" would be clearer as you\n> do below.\n> \n> > +\toptions is given, these are bypassed.\n> > +\tSee also linkgit:githooks[5].\n> > +\n> > +--verify::\n> > +\tThis option re-enables running of the pre-commit and commit-msg hooks\n> > +\tafter an earlier `-n` or `--no-verify`.\n> > \tSee also linkgit:githooks[5].\n> \n> Some of the existing documentation describes the \"--no-foo\" option with\n> \"--foo\" (e.g --[no-]signoff) but in other places we list the two options\n> separately (e.g. --[no-]edit), I'd lean towards combining them as you have\n> done for the merge documentation but I don't feel strongly about it.\n\nHow about this instead:\n\n -n::\n --no-verify::\n By default, pre-commit and commit-msg hooks are run. When one of these\n options is given, the hooks will be bypassed.\n See also linkgit:githooks[5].\n\n --verify::\n This option re-enables running of the pre-commit and commit-msg hooks\n after an earlier `-n` or `--no-verify`.\n\n> > diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\n> > index 3819fadac1..324ae879d2 100644\n> > --- a/Documentation/git-merge.txt\n> > +++ b/Documentation/git-merge.txt\n> > @@ -10,7 +10,7 @@ SYNOPSIS\n> > --------\n> > [verse]\n> > 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]\n> > -\t[--no-verify] [-s ] [-X ] [-S[]]\n> > +\t[--[no-]verify] [-s ] [-X ] [-S[]]\n> \n> Again I'm not sure changing the synopsis makes things clearer.\n\nRemoved.\n\n> > \t[--[no-]allow-unrelated-histories]\n> > \t[--[no-]rerere-autoupdate] [-m ] [-F ] [...]\n> > 'git merge' (--continue | --abort | --quit)\n> > diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> > index 80d4831662..f8016b0f7b 100644\n> > --- a/Documentation/merge-options.txt\n> > +++ b/Documentation/merge-options.txt\n> > @@ -112,8 +112,9 @@ option can be used to override --squash.\n> > +\n> > With --squash, --commit is not allowed, and will fail.\n> > ---no-verify::\n> > -\tThis option bypasses the pre-merge and commit-msg hooks.\n> > +--[no-]verify::\n> > +\tBy default, pre-merge and commit-msg hooks are run. When `--no-verify`\n> \n> I think \"the pre-merge ...\" would be better here as well.\n\nLike this?\n\n --[no-]verify::\n By default, the pre-merge and commit-msg hooks are run.\n\t When `--no-verify` is given, these are bypassed.\n See also linkgit:githooks[5].\n\n> > diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\n> > index 31b9c6a2c1..166ff5fb26 100755\n> > --- a/t/t7504-commit-msg-hook.sh\n> > +++ b/t/t7504-commit-msg-hook.sh\n> > @@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '\n> > '\n> > +test_expect_success '-n with failing hook' '\n> > +\n> > +\techo \"more\" >> file &&\n> > +\tgit add file &&\n> > +\tgit commit -n -m \"more\"\n> > +\n> > +'\n> \n> Is this to check that \"-n\" works like \"--no-verify\"?\n\nFrankly, it was to check that the separate \"-n\" option works as I supposed it\nwould. I never used parse-options before.\n\n> I think it would be very useful to add another test that checks \"--verify\"\n> overrides \"--no-verify\".\n\nReplaced the test with one which has \"-n --verify\".\n\nThanks!\n\n\n Documentation/git-commit.txt | 7 ++++++-\n Documentation/merge-options.txt | 5 +++--\n t/t7504-commit-msg-hook.sh | 8 ++++++++\n 3 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex a3baea32ae..2268787483 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -174,9 +174,14 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n \n -n::\n --no-verify::\n-\tThis option bypasses the pre-commit and commit-msg hooks.\n+\tBy default, pre-commit and commit-msg hooks are run. When one of these\n+\toptions is given, the hooks will be bypassed.\n \tSee also linkgit:githooks[5].\n \n+--verify::\n+\tThis option re-enables running of the pre-commit and commit-msg hooks\n+\tafter an earlier `-n` or `--no-verify`.\n+\n --allow-empty::\n \tUsually recording a commit that has the exact same tree as its\n \tsole parent commit is a mistake, and the command prevents you\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 80d4831662..80267008af 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -112,8 +112,9 @@ option can be used to override --squash.\n +\n With --squash, --commit is not allowed, and will fail.\n \n---no-verify::\n-\tThis option bypasses the pre-merge and commit-msg hooks.\n+--[no-]verify::\n+\tBy default, the pre-merge and commit-msg hooks are run.\n+\tWhen `--no-verify` is given, these are bypassed.\n \tSee also linkgit:githooks[5].\n \n -s ::\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 31b9c6a2c1..67fcc19637 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '\n \n '\n \n+test_expect_success '-n followed by --verify with failing hook' '\n+\n+\techo \"even more\" >> file &&\n+\tgit add file &&\n+\ttest_must_fail git commit -n --verify -m \"even more\"\n+\n+'\n+\n test_expect_success '--no-verify with failing hook (editor)' '\n \n \techo \"more stuff\" >> file &&\n-- \n2.33.0.22.g8cd9218530\n\n"},{"id":"439879","messageId":"YXrFy9I1KPz3IZyp@pflmari","threadId":"56787","inReplyTo":"YXrFaJXbuSuwfhQ7@pflmari","subject":"[PATCH 2/2] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-28T15:46:19Z","receivedAt":"2021-10-28T15:46:38Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"From: Alex Riesen \n\nThe option was incorrectly auto-translated to \"--no-verify-signatures\",\nwhich causes the unexpected effect of the hook being called.\nAnd an even more unexpected effect of disabling verification of signatures.\n\nThe manual page describes the option to behave same as the similarly\nnamed option of \"git merge\", which seems to be the original intention\nof this option in the \"pull\" command.\n\nSigned-off-by: Alexander Riesen \n---\n builtin/pull.c | 6 ++++++\n t/t5521-pull-options.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 30 insertions(+)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 425950f469..e783da10b2 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -84,6 +84,7 @@ static char *opt_edit;\n static char *cleanup_arg;\n static char *opt_ff;\n static char *opt_verify_signatures;\n+static char *opt_verify;\n static int opt_autostash = -1;\n static int config_autostash;\n static int check_trust_level = 1;\n@@ -160,6 +161,9 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n \t\tN_(\"abort if fast-forward is not possible\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n+\tOPT_PASSTHRU(0, \"verify\", &opt_verify, NULL,\n+\t\tN_(\"control use of pre-merge-commit and commit-msg hooks\"),\n+\t\tPARSE_OPT_NOARG),\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n@@ -688,6 +692,8 @@ static int run_merge(void)\n \t\tstrvec_pushf(&args, \"--cleanup=%s\", cleanup_arg);\n \tif (opt_ff)\n \t\tstrvec_push(&args, opt_ff);\n+\tif (opt_verify)\n+\t\tstrvec_push(&args, opt_verify);\n \tif (opt_verify_signatures)\n \t\tstrvec_push(&args, opt_verify_signatures);\n \tstrvec_pushv(&args, opt_strategies.v);\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex db1a381cd9..22cf1b2cf7 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -225,4 +225,28 @@ test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'git pull --no-verify flag passed to merge' '\n+\ttest_when_finished \"rm -fr src dst actual\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\twrite_script dst/.git/hooks/commit-msg <<-\\EOF &&\n+\tfalse\n+\tEOF\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --no-ff --no-verify\n+'\n+\n+test_expect_success 'git pull --no-verify --verify passed to merge' '\n+\ttest_when_finished \"rm -fr src dst actual\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\twrite_script dst/.git/hooks/commit-msg <<-\\EOF &&\n+\tfalse\n+\tEOF\n+\ttest_commit -C src two &&\n+\ttest_must_fail git -C dst pull --no-ff --no-verify --verify\n+'\n+\n test_done\n-- \n2.33.0.22.g8cd9218530\n\n"},{"id":"439880","messageId":"YXrGd9ZF9E+lApZY@pflmari","threadId":"56787","inReplyTo":"YXrFaJXbuSuwfhQ7@pflmari","subject":"Re: [PATCH] Remove negation from the commit and merge option \"--no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-28T15:49:11Z","receivedAt":"2021-10-28T15:49:51Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Subject of this messages was supposed to be\n\n Document positive variant of commit and merge option \"--no-verify\"\n\nBut I hand-edited it beyond all repair :-(\n"},{"id":"439897","messageId":"xmqqv91hrt2y.fsf@gitster.g","threadId":"56787","inReplyTo":"YXrFy9I1KPz3IZyp@pflmari","subject":"Re: [PATCH 2/2] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T16:51:17Z","receivedAt":"2021-10-28T16:51:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> Subject: Re: [PATCH 2/2] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"\n\nPerhaps\n\n Subject: [PATCH] pull: honor --no-verify and do not call the commit-msg hook\n\ninstead.\n\n> From: Alex Riesen \n>\n> The option was incorrectly auto-translated to \"--no-verify-signatures\",\n> which causes the unexpected effect of the hook being called.\n> And an even more unexpected effect of disabling verification of signatures.\n>\n> The manual page describes the option to behave same as the similarly\n> named option of \"git merge\", which seems to be the original intention\n> of this option in the \"pull\" command.\n>\n> Signed-off-by: Alexander Riesen \n> ---\n> builtin/pull.c | 6 ++++++\n> t/t5521-pull-options.sh | 24 ++++++++++++++++++++++++\n> 2 files changed, 30 insertions(+)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 425950f469..e783da10b2 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -84,6 +84,7 @@ static char *opt_edit;\n> static char *cleanup_arg;\n> static char *opt_ff;\n> static char *opt_verify_signatures;\n> +static char *opt_verify;\n> static int opt_autostash = -1;\n> static int config_autostash;\n> static int check_trust_level = 1;\n> @@ -160,6 +161,9 @@ static struct option pull_options[] = {\n> \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n> \t\tN_(\"abort if fast-forward is not possible\"),\n> \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n> +\tOPT_PASSTHRU(0, \"verify\", &opt_verify, NULL,\n> +\t\tN_(\"control use of pre-merge-commit and commit-msg hooks\"),\n> +\t\tPARSE_OPT_NOARG),\n> \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n> \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n> \t\tPARSE_OPT_NOARG),\n> @@ -688,6 +692,8 @@ static int run_merge(void)\n> \t\tstrvec_pushf(&args, \"--cleanup=%s\", cleanup_arg);\n> \tif (opt_ff)\n> \t\tstrvec_push(&args, opt_ff);\n> +\tif (opt_verify)\n> +\t\tstrvec_push(&args, opt_verify);\n> \tif (opt_verify_signatures)\n> \t\tstrvec_push(&args, opt_verify_signatures);\n\nLooks quite straight-forward, especially that now this just mimicks\nhow --[no-]verify-signatures is passed through.\n\nThanks, will queue.\n"},{"id":"439899","messageId":"YXra5UgxtgVubJL/@pflmari","threadId":"56787","inReplyTo":"xmqqv91hrt2y.fsf@gitster.g","subject":"Re: [PATCH 2/2] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-28T17:16:21Z","receivedAt":"2021-10-28T17:16:36Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Thu, Oct 28, 2021 18:51:17 +0200:\n> Alex Riesen writes:\n> \n> > Subject: Re: [PATCH 2/2] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"\n> \n> Perhaps\n> \n> Subject: [PATCH] pull: honor --no-verify and do not call the commit-msg hook\n> \n> instead.\n\nLooks fine from my side. Shall I resend?\n\n> Looks quite straight-forward, especially that now this just mimicks\n> how --[no-]verify-signatures is passed through.\n> \n> Thanks, will queue.\n\nOr did you queue it as is?\n\nRegards,\nAlex\n\n"},{"id":"439927","messageId":"xmqqmtmtq7dy.fsf@gitster.g","threadId":"56787","inReplyTo":"YXra5UgxtgVubJL/@pflmari","subject":"Re: [PATCH 2/2] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T19:25:13Z","receivedAt":"2021-10-28T19:25:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> Junio C Hamano, Thu, Oct 28, 2021 18:51:17 +0200:\n>> Alex Riesen writes:\n>> \n>> > Subject: Re: [PATCH 2/2] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"\n>> \n>> Perhaps\n>> \n>> Subject: [PATCH] pull: honor --no-verify and do not call the commit-msg hook\n>> \n>> instead.\n>\n> Looks fine from my side. Shall I resend?\n\nIf you are OK with the updated text, then I can locally amend.\n"},{"id":"439983","messageId":"YXuWC7zCyVz8s6Yr@pflmari","threadId":"56787","inReplyTo":"xmqqmtmtq7dy.fsf@gitster.g","subject":"Re: [PATCH 2/2] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-29T06:34:51Z","receivedAt":"2021-10-29T06:35:16Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Thu, Oct 28, 2021 21:25:13 +0200:\n> Alex Riesen writes:\n> \n> > Junio C Hamano, Thu, Oct 28, 2021 18:51:17 +0200:\n> >> Alex Riesen writes:\n> >> \n> >> > Subject: Re: [PATCH 2/2] Fix \"commit-msg\" hook unexpectedly called for \"git pull --no-verify\"\n> >> \n> >> Perhaps\n> >> \n> >> Subject: [PATCH] pull: honor --no-verify and do not call the commit-msg hook\n> >> \n> >> instead.\n> >\n> > Looks fine from my side. Shall I resend?\n> \n> If you are OK with the updated text, then I can locally amend.\n\nYes, of course! Thanks!\n"},{"id":"440000","messageId":"7be2fde3-69b2-1da7-bb94-7c181490f626@gmail.com","threadId":"56787","inReplyTo":"YXrFaJXbuSuwfhQ7@pflmari","subject":"Re: [PATCH] Remove negation from the commit and merge option \"--no-verify\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-10-29T13:32:16Z","receivedAt":"2021-10-29T13:32:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 28/10/2021 16:44, Alex Riesen wrote:\n> From: Alex Riesen \n> \n> This documents re-enabling of the hooks disabled by an earlier\n> \"--no-verify\" in command-line.\n> \n> Signed-off-by: Alexander Riesen \n> ---\n> \n> Hi Phillip,\n> \n> Phillip Wood, Thu, Oct 28, 2021 15:57:58 +0200:\n>> On 28/10/2021 09:04, Alex Riesen wrote:\n>>> From: Alex Riesen \n>>>\n>>> This allows re-enabling of the hooks disabled by an earlier \"--no-verify\"\n>>> in command-line and makes the interface more consistent.\n>>\n>> Thanks for working on this. Since 0f1930c587 (\"parse-options: allow\n>> positivation of options starting, with no-\", 2012-02-25) merge and commit\n>> have accepted \"--verify\" but it is undocumented. The documentation updates\n>> and fix to pull in this patch are very welcome, but I'm not sure we need the\n>> other changes. I've left a couple of comments below.\n>>\n>> [As an aside we should probably improve the documentation in parse-options.h\n>> if both Peff and Junio did not know how it handles \"--no-foo\" but that is\n>> outside the scope of this patch]\n> \n> Interesting feature. It is unfortunate it was so well hidden. You're right, of\n> course, and the newly added tests in t7504-commit-msg-hook.sh pass without any\n> changes to the \"builtin/commit.c\".\n> \n> Removal of double-negation in the code was an improvement to its readability,\n> but I like small patches more.\n> \n> Also, the series has no conflicts with 2.33.0 anymore and the \"git pull\" can\n> be applied independently.\n> \n>>> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\n>>> index a3baea32ae..ba66209274 100644\n>>> --- a/Documentation/git-commit.txt\n>>> +++ b/Documentation/git-commit.txt\n>>> @@ -11,7 +11,7 @@ SYNOPSIS\n>>> 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u] [--amend]\n>>> \t [--dry-run] [(-c | -C | --fixup | --squash) ]\n>>> \t [-F | -m ] [--reset-author] [--allow-empty]\n>>> -\t [--allow-empty-message] [--no-verify] [-e] [--author=]\n>>> +\t [--allow-empty-message] [--[no-]verify] [-e] [--author=]\n>>\n>> I think for the synopsis it is fine just to list the most common options.\n>> Having --no-verify without the [no-] makes it clear that --verify is the\n>> default so is not a commonly used option.\n> \n> Yep, makes sense.\n> \n>>> \t [--date=] [--cleanup=] [--[no-]status]\n>>> \t [-i | -o] [--pathspec-from-file= [--pathspec-file-nul]]\n>>> \t [-S[]] [--] [...]\n>>> @@ -174,7 +174,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n>>> -n::\n>>> --no-verify::\n>>> -\tThis option bypasses the pre-commit and commit-msg hooks.\n>>> +\tBy default, pre-merge and commit-msg hooks are run. When one of these\n>>\n>> I think saying \"the pre-merge and commit-msg hooks\" would be clearer as you\n>> do below.\n>>\n>>> +\toptions is given, these are bypassed.\n>>> +\tSee also linkgit:githooks[5].\n>>> +\n>>> +--verify::\n>>> +\tThis option re-enables running of the pre-commit and commit-msg hooks\n>>> +\tafter an earlier `-n` or `--no-verify`.\n>>> \tSee also linkgit:githooks[5].\n>>\n>> Some of the existing documentation describes the \"--no-foo\" option with\n>> \"--foo\" (e.g --[no-]signoff) but in other places we list the two options\n>> separately (e.g. --[no-]edit), I'd lean towards combining them as you have\n>> done for the merge documentation but I don't feel strongly about it.\n> \n> How about this instead:\n> \n> -n::\n> --no-verify::\n> By default, pre-commit and commit-msg hooks are run. When one of these\n> options is given, the hooks will be bypassed.\n> See also linkgit:githooks[5].\n> \n> --verify::\n> This option re-enables running of the pre-commit and commit-msg hooks\n> after an earlier `-n` or `--no-verify`.\n> \n>>> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\n>>> index 3819fadac1..324ae879d2 100644\n>>> --- a/Documentation/git-merge.txt\n>>> +++ b/Documentation/git-merge.txt\n>>> @@ -10,7 +10,7 @@ SYNOPSIS\n>>> --------\n>>> [verse]\n>>> 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]\n>>> -\t[--no-verify] [-s ] [-X ] [-S[]]\n>>> +\t[--[no-]verify] [-s ] [-X ] [-S[]]\n>>\n>> Again I'm not sure changing the synopsis makes things clearer.\n> \n> Removed.\n> \n>>> \t[--[no-]allow-unrelated-histories]\n>>> \t[--[no-]rerere-autoupdate] [-m ] [-F ] [...]\n>>> 'git merge' (--continue | --abort | --quit)\n>>> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n>>> index 80d4831662..f8016b0f7b 100644\n>>> --- a/Documentation/merge-options.txt\n>>> +++ b/Documentation/merge-options.txt\n>>> @@ -112,8 +112,9 @@ option can be used to override --squash.\n>>> +\n>>> With --squash, --commit is not allowed, and will fail.\n>>> ---no-verify::\n>>> -\tThis option bypasses the pre-merge and commit-msg hooks.\n>>> +--[no-]verify::\n>>> +\tBy default, pre-merge and commit-msg hooks are run. When `--no-verify`\n>>\n>> I think \"the pre-merge ...\" would be better here as well.\n> \n> Like this?\n> \n> --[no-]verify::\n> By default, the pre-merge and commit-msg hooks are run.\n> \t When `--no-verify` is given, these are bypassed.\n> See also linkgit:githooks[5].\n> \n>>> diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\n>>> index 31b9c6a2c1..166ff5fb26 100755\n>>> --- a/t/t7504-commit-msg-hook.sh\n>>> +++ b/t/t7504-commit-msg-hook.sh\n>>> @@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '\n>>> '\n>>> +test_expect_success '-n with failing hook' '\n>>> +\n>>> +\techo \"more\" >> file &&\n>>> +\tgit add file &&\n>>> +\tgit commit -n -m \"more\"\n>>> +\n>>> +'\n>>\n>> Is this to check that \"-n\" works like \"--no-verify\"?\n> \n> Frankly, it was to check that the separate \"-n\" option works as I supposed it\n> would. I never used parse-options before.\n> \n>> I think it would be very useful to add another test that checks \"--verify\"\n>> overrides \"--no-verify\".\n> \n> Replaced the test with one which has \"-n --verify\".\n> \n> Thanks!\n> \n> \n> Documentation/git-commit.txt | 7 ++++++-\n> Documentation/merge-options.txt | 5 +++--\n> t/t7504-commit-msg-hook.sh | 8 ++++++++\n> 3 files changed, 17 insertions(+), 3 deletions(-)\n> \n> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\n> index a3baea32ae..2268787483 100644\n> --- a/Documentation/git-commit.txt\n> +++ b/Documentation/git-commit.txt\n> @@ -174,9 +174,14 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n> \n> -n::\n> --no-verify::\n> -\tThis option bypasses the pre-commit and commit-msg hooks.\n> +\tBy default, pre-commit and commit-msg hooks are run. When one of these\n\nAs I suggested yesterday I think this would be better if it kept the \n\"the\" from the original text as you do below for the merge documentation \n- s/default, /&the /\n\n> +\toptions is given, the hooks will be bypassed.\n> \tSee also linkgit:githooks[5]. >\n> +--verify::\n> +\tThis option re-enables running of the pre-commit and commit-msg hooks\n> +\tafter an earlier `-n` or `--no-verify`.\n> +\n> --allow-empty::\n> \tUsually recording a commit that has the exact same tree as its\n> \tsole parent commit is a mistake, and the command prevents you\n> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> index 80d4831662..80267008af 100644\n> --- a/Documentation/merge-options.txt\n> +++ b/Documentation/merge-options.txt\n> @@ -112,8 +112,9 @@ option can be used to override --squash.\n> +\n> With --squash, --commit is not allowed, and will fail.\n> \n> ---no-verify::\n> -\tThis option bypasses the pre-merge and commit-msg hooks.\n> +--[no-]verify::\n> +\tBy default, the pre-merge and commit-msg hooks are run.\n> +\tWhen `--no-verify` is given, these are bypassed.\n> \tSee also linkgit:githooks[5].\n\nThis text looks good. It would be nice to be consistent when documenting \n\"--verify\" and \"--no-verify\" so that documentation for commit and merge \nboth have either a separate entry for each option as you have for commit \nor a shared entry as you have here for merge. I'd be tempted to use this \nform in the commit documentation.\n\n> -s ::\n> diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\n> index 31b9c6a2c1..67fcc19637 100755\n> --- a/t/t7504-commit-msg-hook.sh\n> +++ b/t/t7504-commit-msg-hook.sh\n> @@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '\n> \n> '\n> \n> +test_expect_success '-n followed by --verify with failing hook' '\n> +\n> +\techo \"even more\" >> file &&\n> +\tgit add file &&\n> +\ttest_must_fail git commit -n --verify -m \"even more\"\n> +\n> +'\n\nThanks, having the new test is very helpful.\n\nBest Wishes\n\nPhillip\n"},{"id":"440002","messageId":"YXv7CW4QHQOzFla6@pflmari","threadId":"56787","inReplyTo":"7be2fde3-69b2-1da7-bb94-7c181490f626@gmail.com","subject":"[PATCH] Document positive variant of commit and merge option \"--no-verify\"","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-10-29T13:45:45Z","receivedAt":"2021-10-29T13:46:01Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"From: Alex Riesen \n\nThis documents \"--verify\" option of the commands. It can be used to re-enable\nthe hooks disabled by an earlier \"--no-verify\" in command-line.\n\nSigned-off-by: Alexander Riesen \n---\n\nPhillip Wood, Fri, Oct 29, 2021 15:32:16 +0200:\n> On 28/10/2021 16:44, Alex Riesen wrote:\n> > diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\n> > index a3baea32ae..2268787483 100644\n> > --- a/Documentation/git-commit.txt\n> > +++ b/Documentation/git-commit.txt\n> > @@ -174,9 +174,14 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n> > -n::\n> > --no-verify::\n> > -\tThis option bypasses the pre-commit and commit-msg hooks.\n> > +\tBy default, pre-commit and commit-msg hooks are run. When one of these\n> \n> As I suggested yesterday I think this would be better if it kept the \"the\"\n> from the original text as you do below for the merge documentation -\n> s/default, /&the /\n\nUpdated:\n\n -n::\n --[no-]verify::\n\t By default, the pre-commit and commit-msg hooks are run.\n\t When any of `--no-verify` or `-n` is given, these are bypassed.\n\t See also linkgit:githooks[5].\n\n> > --- a/Documentation/merge-options.txt\n> > +++ b/Documentation/merge-options.txt\n> > @@ -112,8 +112,9 @@ option can be used to override --squash.\n> > +\n> > With --squash, --commit is not allowed, and will fail.\n> > ---no-verify::\n> > -\tThis option bypasses the pre-merge and commit-msg hooks.\n> > +--[no-]verify::\n> > +\tBy default, the pre-merge and commit-msg hooks are run.\n> > +\tWhen `--no-verify` is given, these are bypassed.\n> > \tSee also linkgit:githooks[5].\n> \n> This text looks good. It would be nice to be consistent when documenting\n> \"--verify\" and \"--no-verify\" so that documentation for commit and merge both\n> have either a separate entry for each option as you have for commit or a\n> shared entry as you have here for merge. I'd be tempted to use this form in\n> the commit documentation.\n\nSo I did.\n\nRegards,\nAlex\n\n Documentation/git-commit.txt | 5 +++--\n Documentation/merge-options.txt | 5 +++--\n t/t7504-commit-msg-hook.sh | 8 ++++++++\n 3 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex a3baea32ae..b27a4c4c34 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -173,8 +173,9 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n \t(see http://developercertificate.org/ for more information).\n \n -n::\n---no-verify::\n-\tThis option bypasses the pre-commit and commit-msg hooks.\n+--[no-]verify::\n+\tBy default, the pre-commit and commit-msg hooks are run.\n+\tWhen any of `--no-verify` or `-n` is given, these are bypassed.\n \tSee also linkgit:githooks[5].\n \n --allow-empty::\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 80d4831662..80267008af 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -112,8 +112,9 @@ option can be used to override --squash.\n +\n With --squash, --commit is not allowed, and will fail.\n \n---no-verify::\n-\tThis option bypasses the pre-merge and commit-msg hooks.\n+--[no-]verify::\n+\tBy default, the pre-merge and commit-msg hooks are run.\n+\tWhen `--no-verify` is given, these are bypassed.\n \tSee also linkgit:githooks[5].\n \n -s ::\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 31b9c6a2c1..67fcc19637 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '\n \n '\n \n+test_expect_success '-n followed by --verify with failing hook' '\n+\n+\techo \"even more\" >> file &&\n+\tgit add file &&\n+\ttest_must_fail git commit -n --verify -m \"even more\"\n+\n+'\n+\n test_expect_success '--no-verify with failing hook (editor)' '\n \n \techo \"more stuff\" >> file &&\n-- \n2.33.0.22.g8cd9218530\n\n"},{"id":"440221","messageId":"918977fe-4e87-8a2d-c337-6b57b83ae8dc@gmail.com","threadId":"56787","inReplyTo":"YXv7CW4QHQOzFla6@pflmari","subject":"Re: [PATCH] Document positive variant of commit and merge option \"--no-verify\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-11-01T15:34:57Z","receivedAt":"2021-11-01T15:35:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 29/10/2021 14:45, Alex Riesen wrote:\n> From: Alex Riesen \n> \n> This documents \"--verify\" option of the commands. It can be used to re-enable\n> the hooks disabled by an earlier \"--no-verify\" in command-line.\n> \n> Signed-off-by: Alexander Riesen \n> ---\n\nThis version looks good, thanks for documenting these options\n\nBest Wishes\n\nPhillip\n\n> Phillip Wood, Fri, Oct 29, 2021 15:32:16 +0200:\n>> On 28/10/2021 16:44, Alex Riesen wrote:\n>>> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\n>>> index a3baea32ae..2268787483 100644\n>>> --- a/Documentation/git-commit.txt\n>>> +++ b/Documentation/git-commit.txt\n>>> @@ -174,9 +174,14 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n>>> -n::\n>>> --no-verify::\n>>> -\tThis option bypasses the pre-commit and commit-msg hooks.\n>>> +\tBy default, pre-commit and commit-msg hooks are run. When one of these\n>>\n>> As I suggested yesterday I think this would be better if it kept the \"the\"\n>> from the original text as you do below for the merge documentation -\n>> s/default, /&the /\n> \n> Updated:\n> \n> -n::\n> --[no-]verify::\n> \t By default, the pre-commit and commit-msg hooks are run.\n> \t When any of `--no-verify` or `-n` is given, these are bypassed.\n> \t See also linkgit:githooks[5].\n> \n>>> --- a/Documentation/merge-options.txt\n>>> +++ b/Documentation/merge-options.txt\n>>> @@ -112,8 +112,9 @@ option can be used to override --squash.\n>>> +\n>>> With --squash, --commit is not allowed, and will fail.\n>>> ---no-verify::\n>>> -\tThis option bypasses the pre-merge and commit-msg hooks.\n>>> +--[no-]verify::\n>>> +\tBy default, the pre-merge and commit-msg hooks are run.\n>>> +\tWhen `--no-verify` is given, these are bypassed.\n>>> \tSee also linkgit:githooks[5].\n>>\n>> This text looks good. It would be nice to be consistent when documenting\n>> \"--verify\" and \"--no-verify\" so that documentation for commit and merge both\n>> have either a separate entry for each option as you have for commit or a\n>> shared entry as you have here for merge. I'd be tempted to use this form in\n>> the commit documentation.\n> \n> So I did.\n> \n> Regards,\n> Alex\n> \n> Documentation/git-commit.txt | 5 +++--\n> Documentation/merge-options.txt | 5 +++--\n> t/t7504-commit-msg-hook.sh | 8 ++++++++\n> 3 files changed, 14 insertions(+), 4 deletions(-)\n> \n> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\n> index a3baea32ae..b27a4c4c34 100644\n> --- a/Documentation/git-commit.txt\n> +++ b/Documentation/git-commit.txt\n> @@ -173,8 +173,9 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.\n> \t(see http://developercertificate.org/ for more information).\n> \n> -n::\n> ---no-verify::\n> -\tThis option bypasses the pre-commit and commit-msg hooks.\n> +--[no-]verify::\n> +\tBy default, the pre-commit and commit-msg hooks are run.\n> +\tWhen any of `--no-verify` or `-n` is given, these are bypassed.\n> \tSee also linkgit:githooks[5].\n> \n> --allow-empty::\n> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> index 80d4831662..80267008af 100644\n> --- a/Documentation/merge-options.txt\n> +++ b/Documentation/merge-options.txt\n> @@ -112,8 +112,9 @@ option can be used to override --squash.\n> +\n> With --squash, --commit is not allowed, and will fail.\n> \n> ---no-verify::\n> -\tThis option bypasses the pre-merge and commit-msg hooks.\n> +--[no-]verify::\n> +\tBy default, the pre-merge and commit-msg hooks are run.\n> +\tWhen `--no-verify` is given, these are bypassed.\n> \tSee also linkgit:githooks[5].\n> \n> -s ::\n> diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\n> index 31b9c6a2c1..67fcc19637 100755\n> --- a/t/t7504-commit-msg-hook.sh\n> +++ b/t/t7504-commit-msg-hook.sh\n> @@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' '\n> \n> '\n> \n> +test_expect_success '-n followed by --verify with failing hook' '\n> +\n> +\techo \"even more\" >> file &&\n> +\tgit add file &&\n> +\ttest_must_fail git commit -n --verify -m \"even more\"\n> +\n> +'\n> +\n> test_expect_success '--no-verify with failing hook (editor)' '\n> \n> \techo \"more stuff\" >> file &&\n> \n"}]}