{"thread":{"id":"58875","subject":"[PATCH v4] am: Allow passing --no-verify flag","startedAt":"2022-11-30T17:28:47Z","lastAt":"2022-12-25T11:26:09Z","messageCount":7,"participants":["Thierry Reding","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"468259","messageId":"20221130172833.2662751-1-thierry.reding@gmail.com","threadId":"58875","inReplyTo":null,"subject":"[PATCH v4] am: Allow passing --no-verify flag","fromName":"Thierry Reding","fromEmail":"thierry.reding@gmail.com","sentAt":"2022-11-30T17:28:33Z","receivedAt":"2022-11-30T17:28:47Z","isPatch":true,"sender":{"key":"thierry.reding@gmail.com","avatar":null},"body":"From: Thierry Reding <treding@nvidia.com>\n\nThe git-am --no-verify flag is analogous to the same flag passed to\ngit-commit. It bypasses the pre-applypatch and applypatch-msg hooks\nif they are enabled.\n\nSigned-off-by: Thierry Reding <treding@nvidia.com>\n---\nChanges in v4:\n- check that scripts haven't run and that commits have been applied\n\nChanges in v3:\n- move tests to existing t/t4150-am\n\nChanges in v2:\n- add test to verify that the new option works\n\n Documentation/git-am.txt |  8 +++++++-\n builtin/am.c             | 11 ++++++++---\n t/t4150-am.sh            | 32 ++++++++++++++++++++++++++++++++\n 3 files changed, 47 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex 326276e51ce5..0c1dfb3c98b4 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -9,7 +9,7 @@ git-am - Apply a series of patches from a mailbox\n SYNOPSIS\n --------\n [verse]\n-'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]\n+'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8] [--no-verify]\n \t [--[no-]3way] [--interactive] [--committer-date-is-author-date]\n \t [--ignore-date] [--ignore-space-change | --ignore-whitespace]\n \t [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]\n@@ -138,6 +138,12 @@ include::rerere-options.txt[]\n --interactive::\n \tRun interactively.\n \n+-n::\n+--no-verify::\n+\tBy default, the pre-applypatch and applypatch-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 --committer-date-is-author-date::\n \tBy default the command records the date from the e-mail\n \tmessage as the commit author date, and uses the time of\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 30c9b3a9cd72..dfe8104f570d 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -117,6 +117,7 @@ struct am_state {\n \n \t/* various operating modes and command line options */\n \tint interactive;\n+\tint no_verify;\n \tint threeway;\n \tint quiet;\n \tint signoff; /* enum signoff_type */\n@@ -472,10 +473,12 @@ static void am_destroy(const struct am_state *state)\n  */\n static int run_applypatch_msg_hook(struct am_state *state)\n {\n-\tint ret;\n+\tint ret = 0;\n \n \tassert(state->msg);\n-\tret = run_hooks_l(\"applypatch-msg\", am_path(state, \"final-commit\"), NULL);\n+\n+\tif (!state->no_verify)\n+\t\tret = run_hooks_l(\"applypatch-msg\", am_path(state, \"final-commit\"), NULL);\n \n \tif (!ret) {\n \t\tFREE_AND_NULL(state->msg);\n@@ -1640,7 +1643,7 @@ static void do_commit(const struct am_state *state)\n \tconst char *reflog_msg, *author, *committer = NULL;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tif (run_hooks(\"pre-applypatch\"))\n+\tif (!state->no_verify && run_hooks(\"pre-applypatch\"))\n \t\texit(1);\n \n \tif (write_cache_as_tree(&tree, 0, NULL))\n@@ -2330,6 +2333,8 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_BOOL('i', \"interactive\", &state.interactive,\n \t\t\tN_(\"run interactively\")),\n+\t\tOPT_BOOL('n', \"no-verify\", &state.no_verify,\n+\t\t\tN_(\"bypass pre-applypatch and applypatch-msg hooks\")),\n \t\tOPT_HIDDEN_BOOL('b', \"binary\", &binary,\n \t\t\tN_(\"historical option -- no-op\")),\n \t\tOPT_BOOL('3', \"3way\", &state.threeway,\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex cdad4b688078..d77c4dcefeb7 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -345,6 +345,22 @@ test_expect_success 'am with failing applypatch-msg hook' '\n \ttest_cmp_rev first HEAD\n '\n \n+test_expect_success 'am with failing applypatch-msg hook (no verify)' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout first &&\n+\ttest_hook applypatch-msg <<-\\EOF &&\n+\techo hook-message >\"$1\"\n+\texit 1\n+\tEOF\n+\tgit am --no-verify patch1 &&\n+\ttest_path_is_missing .git/rebase-apply &&\n+\tgit diff --exit-code second &&\n+\ttest_cmp_rev second HEAD &&\n+\tgit log -1 --format=format:%B >actual &&\n+\ttest_cmp msg actual\n+'\n+\n test_expect_success 'am with pre-applypatch hook' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n@@ -374,6 +390,22 @@ test_expect_success 'am with failing pre-applypatch hook' '\n \ttest_cmp_rev first HEAD\n '\n \n+test_expect_success 'am with failing pre-applypatch hook (no verify)' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout first &&\n+\ttouch empty-file &&\n+\ttest_hook pre-applypatch <<-\\EOF &&\n+\trm empty-file\n+\texit 1\n+\tEOF\n+\tgit am --no-verify patch1 &&\n+\ttest_path_is_missing .git/rebase-apply &&\n+\tgit diff --exit-code second &&\n+\ttest_cmp_rev second HEAD &&\n+\ttest_path_is_file empty-file\n+'\n+\n test_expect_success 'am with post-applypatch hook' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n-- \n2.38.1\n\n"},{"id":"468296","messageId":"xmqqr0xkt07x.fsf@gitster.g","threadId":"58875","inReplyTo":"20221130172833.2662751-1-thierry.reding@gmail.com","subject":"Re: [PATCH v4] am: Allow passing --no-verify flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-01T00:51:46Z","receivedAt":"2022-12-01T00:52:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thierry Reding <thierry.reding@gmail.com> writes:\n\n> +\t\tOPT_BOOL('n', \"no-verify\", &state.no_verify,\n> +\t\t\tN_(\"bypass pre-applypatch and applypatch-msg hooks\")),\n\nI think parse_options machinery is smart enough to do the right to\nallow \"git am --no-verify --verify\" a way to express \"last one wins,\nturn the .no_verify bit off to countermand an earlier --no-verify\",\nso this looks good.\n\n> diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n> index cdad4b688078..d77c4dcefeb7 100755\n> --- a/t/t4150-am.sh\n> +++ b/t/t4150-am.sh\n> @@ -345,6 +345,22 @@ test_expect_success 'am with failing applypatch-msg hook' '\n>  \ttest_cmp_rev first HEAD\n>  '\n>  \n> +test_expect_success 'am with failing applypatch-msg hook (no verify)' '\n> +\trm -fr .git/rebase-apply &&\n> +\tgit reset --hard &&\n> +\tgit checkout first &&\n> +\ttest_hook applypatch-msg <<-\\EOF &&\n> +\techo hook-message >\"$1\"\n> +\texit 1\n> +\tEOF\n> +\tgit am --no-verify patch1 &&\n> +\ttest_path_is_missing .git/rebase-apply &&\n> +\tgit diff --exit-code second &&\n> +\ttest_cmp_rev second HEAD &&\n\nThis somewhat raised my eyebrows, as the condition will not\ngenerally hold.\n\nI am guessing this was merely copied and pasted from the previous\ntest piece before this one, which uses \"test_cmp_rev first HEAD\" and\nits use of test_cmp_rev is more sensible, as it is making sure that\n\"we started with 'first' commit at HEAD, 'am' should not have done\nanything by failing, so expect HEAD is still at 'first'\".\n\nThe reason to expect HEAD to be 'second' here is very different.  We\nstart from 'first', 'am' should have applied the patch as-is while\npretending that the wallclock is frozen (i.e. even the committer\ntimestamp should be the same as the original commit), so expect HEAD\nis pointing at the exact same commit recreated by 'am'\".  That is a\nbit unrealistic expectation outside the context of the tests.\n\nIf we added test_commit or test_tick anywhere before this step in an\nunrelated test, it likely will break this expectation; in other\nwords, I think the use of \"test_cmp_rev second\" here makes it\nunnecessarily brittle.\n\n> +\tgit log -1 --format=format:%B >actual &&\n> +\ttest_cmp msg actual\n> +'\n\nOther than that, this test looks good.\n\n> @@ -374,6 +390,22 @@ test_expect_success 'am with failing pre-applypatch hook' '\n>  \ttest_cmp_rev first HEAD\n>  '\n>  \n> +test_expect_success 'am with failing pre-applypatch hook (no verify)' '\n> +\trm -fr .git/rebase-apply &&\n> +\tgit reset --hard &&\n> +\tgit checkout first &&\n> +\ttouch empty-file &&\n> +\ttest_hook pre-applypatch <<-\\EOF &&\n> +\trm empty-file\n> +\texit 1\n> +\tEOF\n> +\tgit am --no-verify patch1 &&\n> +\ttest_path_is_missing .git/rebase-apply &&\n> +\tgit diff --exit-code second &&\n> +\ttest_cmp_rev second HEAD &&\n> +\ttest_path_is_file empty-file\n> +'\n\nThis \"pre\" stuff, unlike the other hook, does not affect the\nresulting commit, so you invent an extra file \"empty-file\" as a\nmarker and remove it from the hook, so that the absense of the file\ncan be used as a signal that the hook did run.  We could do it the\nother way around (i.e. make sure the marker file does not exist\nbefore running \"git am\", arrange the marker to be created by running\nthe hook, and ensure that the marker does not exist after running\n\"git am\"), and either way is fine.\n\nThe same comment as the previous one applies to \"test_cmp_rev second\"\ncheck.  I think removing them would make the tests better.\n\nOh, also I keep forgetting but\n\n> Subject: Re: [PATCH v4] am: Allow passing --no-verify flag\n\nour convention is not capitalize \"Allow\" here.\n\nThanks.\n"},{"id":"468297","messageId":"xmqqlenst021.fsf@gitster.g","threadId":"58875","inReplyTo":"xmqqr0xkt07x.fsf@gitster.g","subject":"Re: [PATCH v4] am: Allow passing --no-verify flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-01T00:55:18Z","receivedAt":"2022-12-01T00:55:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...\n> The same comment as the previous one applies to \"test_cmp_rev second\"\n> check.  I think removing them would make the tests better.\n\nI will queue this on top for now.  Thanks.\n\n t/t4150-am.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git i/t/t4150-am.sh w/t/t4150-am.sh\nindex d77c4dcefe..50b257e43f 100755\n--- i/t/t4150-am.sh\n+++ w/t/t4150-am.sh\n@@ -356,7 +356,6 @@ test_expect_success 'am with failing applypatch-msg hook (no verify)' '\n \tgit am --no-verify patch1 &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit diff --exit-code second &&\n-\ttest_cmp_rev second HEAD &&\n \tgit log -1 --format=format:%B >actual &&\n \ttest_cmp msg actual\n '\n@@ -401,9 +400,10 @@ test_expect_success 'am with failing pre-applypatch hook (no verify)' '\n \tEOF\n \tgit am --no-verify patch1 &&\n \ttest_path_is_missing .git/rebase-apply &&\n+\ttest_path_is_file empty-file &&\n \tgit diff --exit-code second &&\n-\ttest_cmp_rev second HEAD &&\n-\ttest_path_is_file empty-file\n+\tgit log -1 --format=format:%B >actual &&\n+\ttest_cmp msg actual\n '\n \n test_expect_success 'am with post-applypatch hook' '\n"},{"id":"469427","messageId":"Y6LP+fCMmP33T6Cx@orome","threadId":"58875","inReplyTo":"xmqqlenst021.fsf@gitster.g","subject":"Re: [PATCH v4] am: Allow passing --no-verify flag","fromName":"Thierry Reding","fromEmail":"thierry.reding@gmail.com","sentAt":"2022-12-21T09:20:57Z","receivedAt":"2022-12-21T09:21:09Z","isPatch":true,"sender":{"key":"thierry.reding@gmail.com","avatar":null},"body":"On Thu, Dec 01, 2022 at 09:55:18AM +0900, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > ...\n> > The same comment as the previous one applies to \"test_cmp_rev second\"\n> > check.  I think removing them would make the tests better.\n> \n> I will queue this on top for now.  Thanks.\n\nJust to clarify: do you want me to send out another version with your\nchanges on top or should I consider this done?\n\nThierry\n\n> \n>  t/t4150-am.sh | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git i/t/t4150-am.sh w/t/t4150-am.sh\n> index d77c4dcefe..50b257e43f 100755\n> --- i/t/t4150-am.sh\n> +++ w/t/t4150-am.sh\n> @@ -356,7 +356,6 @@ test_expect_success 'am with failing applypatch-msg hook (no verify)' '\n>  \tgit am --no-verify patch1 &&\n>  \ttest_path_is_missing .git/rebase-apply &&\n>  \tgit diff --exit-code second &&\n> -\ttest_cmp_rev second HEAD &&\n>  \tgit log -1 --format=format:%B >actual &&\n>  \ttest_cmp msg actual\n>  '\n> @@ -401,9 +400,10 @@ test_expect_success 'am with failing pre-applypatch hook (no verify)' '\n>  \tEOF\n>  \tgit am --no-verify patch1 &&\n>  \ttest_path_is_missing .git/rebase-apply &&\n> +\ttest_path_is_file empty-file &&\n>  \tgit diff --exit-code second &&\n> -\ttest_cmp_rev second HEAD &&\n> -\ttest_path_is_file empty-file\n> +\tgit log -1 --format=format:%B >actual &&\n> +\ttest_cmp msg actual\n>  '\n>  \n>  test_expect_success 'am with post-applypatch hook' '\n"},{"id":"469428","messageId":"xmqqtu1prt2j.fsf@gitster.g","threadId":"58875","inReplyTo":"Y6LP+fCMmP33T6Cx@orome","subject":"Re: [PATCH v4] am: Allow passing --no-verify flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-21T09:42:44Z","receivedAt":"2022-12-21T09:42:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thierry Reding <thierry.reding@gmail.com> writes:\n\n> On Thu, Dec 01, 2022 at 09:55:18AM +0900, Junio C Hamano wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>> \n>> > ...\n>> > The same comment as the previous one applies to \"test_cmp_rev second\"\n>> > check.  I think removing them would make the tests better.\n>> \n>> I will queue this on top for now.  Thanks.\n>\n> Just to clarify: do you want me to send out another version with your\n> changes on top or should I consider this done?\n\nIt is up to you.\n\n * If you agree with the \"fix-up\" done with the extra commit (marked\n   as \"SQUASH???\") on top and there is no other changes you want to\n   make, you can tell me to squash it into your patch.\n\n * If you don't and have better idea, you can send out an updated\n   version, and you have my permission to include any (or all) parts\n   of the changes I gave in the message you are responding to.\n\n * Or if you make a very good argument and convince me why your\n   original without my fix-up is better, I may change my mind and\n   drop the changes, keeping your original alone.\n\nOf course, folks on the list may have other suggestions and review\ncomments, which may change the picture again ;-)\n\nIn any case, thanks for contributing!\n\n"},{"id":"469442","messageId":"Y6NApq8pFcqkvlFg@orome","threadId":"58875","inReplyTo":"xmqqtu1prt2j.fsf@gitster.g","subject":"Re: [PATCH v4] am: Allow passing --no-verify flag","fromName":"Thierry Reding","fromEmail":"thierry.reding@gmail.com","sentAt":"2022-12-21T17:21:42Z","receivedAt":"2022-12-21T17:22:55Z","isPatch":true,"sender":{"key":"thierry.reding@gmail.com","avatar":null},"body":"On Wed, Dec 21, 2022 at 06:42:44PM +0900, Junio C Hamano wrote:\n> Thierry Reding <thierry.reding@gmail.com> writes:\n> \n> > On Thu, Dec 01, 2022 at 09:55:18AM +0900, Junio C Hamano wrote:\n> >> Junio C Hamano <gitster@pobox.com> writes:\n> >> \n> >> > ...\n> >> > The same comment as the previous one applies to \"test_cmp_rev second\"\n> >> > check.  I think removing them would make the tests better.\n> >> \n> >> I will queue this on top for now.  Thanks.\n> >\n> > Just to clarify: do you want me to send out another version with your\n> > changes on top or should I consider this done?\n> \n> It is up to you.\n> \n>  * If you agree with the \"fix-up\" done with the extra commit (marked\n>    as \"SQUASH???\") on top and there is no other changes you want to\n>    make, you can tell me to squash it into your patch.\n> \n>  * If you don't and have better idea, you can send out an updated\n>    version, and you have my permission to include any (or all) parts\n>    of the changes I gave in the message you are responding to.\n> \n>  * Or if you make a very good argument and convince me why your\n>    original without my fix-up is better, I may change my mind and\n>    drop the changes, keeping your original alone.\n> \n> Of course, folks on the list may have other suggestions and review\n> comments, which may change the picture again ;-)\n> \n> In any case, thanks for contributing!\n\nYour changes look fine. I applied them to my local tree and everything\nstill works as expected, so feel free to squash those into the commit.\n\nThanks,\nThierry\n"},{"id":"469520","messageId":"xmqqo7rrohbr.fsf@gitster.g","threadId":"58875","inReplyTo":"Y6NApq8pFcqkvlFg@orome","subject":"Re: [PATCH v4] am: Allow passing --no-verify flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-25T11:26:00Z","receivedAt":"2022-12-25T11:26:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thierry Reding <thierry.reding@gmail.com> writes:\n\n> Your changes look fine. I applied them to my local tree and everything\n> still works as expected, so feel free to squash those into the commit.\n\nThanks, will do.\n"}]}