{"thread":{"id":"58865","subject":"[PATCH v3] am: Allow passing --no-verify flag","startedAt":"2022-11-28T18:02:23Z","lastAt":"2022-11-29T10:03:49Z","messageCount":5,"participants":["Thierry Reding","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"468115","messageId":"20221128174825.1510407-1-thierry.reding@gmail.com","threadId":"58865","inReplyTo":null,"subject":"[PATCH v3] am: Allow passing --no-verify flag","fromName":"Thierry Reding","fromEmail":"thierry.reding@gmail.com","sentAt":"2022-11-28T17:48:25Z","receivedAt":"2022-11-28T18:02:23Z","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 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            | 20 ++++++++++++++++++++\n 3 files changed, 35 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..84e6bebeca7a 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -345,6 +345,16 @@ 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+\texit 1\n+\tEOF\n+\tgit am --no-verify patch1\n+'\n+\n test_expect_success 'am with pre-applypatch hook' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n@@ -374,6 +384,16 @@ 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+\ttest_hook pre-applypatch <<-\\EOF &&\n+\texit 1\n+\tEOF\n+\tgit am --no-verify patch1\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":"468129","messageId":"Y4U/OhHSg85UV+5L@nand.local","threadId":"58865","inReplyTo":"20221128174825.1510407-1-thierry.reding@gmail.com","subject":"Re: [PATCH v3] am: Allow passing --no-verify flag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-28T23:07:38Z","receivedAt":"2022-11-28T23:07:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Thierry,\n\nThis is looking pretty good. Thanks for pushing out a couple of new\nrounds. I took a look at the discussion and have some thoughts below,\nbut they are minor.\n\nThis will likely not be in the upcoming v2.39.0, since Junio has already\ncut and pushed the first release candidate tag. But you should feel free\nto keep working on it in the meantime and we can revisit it post-2.39.\n\nOn Mon, Nov 28, 2022 at 06:48:25PM +0100, Thierry Reding wrote:\n> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\n> index 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\nSpelling this as \"[--no-verify]\" and not \"[--[no-]verify]\" is right,\nsince the option itself is called \"--no-verify\" (and thus has the\nOPT_NONEG flag bit set), and \"--verify\" does not exist. Good.\n\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\nShould we be more explicit here, given the discussion in the earlier\nrounds? IOW, instead of saying that they are \"bypassed\" (which might\nlead the reader to believe that they are run and their output/exit code\nignored), should we say that they are \"not run\" and be clearer?\n\nThe (elided) implementation below looks good to me.\n\n> diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n> index cdad4b688078..84e6bebeca7a 100755\n> --- a/t/t4150-am.sh\n> +++ b/t/t4150-am.sh\n> @@ -345,6 +345,16 @@ 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> +\texit 1\n> +\tEOF\n> +\tgit am --no-verify patch1\n\nShould we verify that the patch was applied with the expected message?\n\n> +'\n> +\n>  test_expect_success 'am with pre-applypatch hook' '\n>  \trm -fr .git/rebase-apply &&\n>  \tgit reset --hard &&\n> @@ -374,6 +384,16 @@ 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> +\ttest_hook pre-applypatch <<-\\EOF &&\n> +\texit 1\n> +\tEOF\n> +\tgit am --no-verify patch1\n\nDitto here.\n\nThanks,\nTaylor\n"},{"id":"468141","messageId":"xmqqv8my4kvu.fsf@gitster.g","threadId":"58865","inReplyTo":"20221128174825.1510407-1-thierry.reding@gmail.com","subject":"Re: [PATCH v3] am: Allow passing --no-verify flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-29T01:20:05Z","receivedAt":"2022-11-29T01:20:21Z","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> +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> +\texit 1\n> +\tEOF\n> +\tgit am --no-verify patch1\n> +'\n> +\n>  test_expect_success 'am with pre-applypatch hook' '\n>  \trm -fr .git/rebase-apply &&\n>  \tgit reset --hard &&\n> @@ -374,6 +384,16 @@ 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> +\ttest_hook pre-applypatch <<-\\EOF &&\n> +\texit 1\n> +\tEOF\n> +\tgit am --no-verify patch1\n> +'\n> +\n>  test_expect_success 'am with post-applypatch hook' '\n>  \trm -fr .git/rebase-apply &&\n>  \tgit reset --hard &&\n\nThese two tests will still pass if you change the implementation to\nrun the hook and simply ignore its exit status, but I recall you\nmaking a good argument against that alternative implementation ,in\ncomparison to \"not running the hook at all\".\n\nI think these tests should make sure that the hooks did not even\nrun.  Perhaps by creating a marker file before running \"git am\",\nadding a \"rm\" that marker file in the hook, and making sure that\nthe marker file still exists after \"git am\" returns, or something\nlike that.\n"},{"id":"468162","messageId":"Y4XYzPhZLhmoGzZT@orome","threadId":"58865","inReplyTo":"Y4U/OhHSg85UV+5L@nand.local","subject":"Re: [PATCH v3] am: Allow passing --no-verify flag","fromName":"Thierry Reding","fromEmail":"thierry.reding@gmail.com","sentAt":"2022-11-29T10:02:52Z","receivedAt":"2022-11-29T10:03:03Z","isPatch":true,"sender":{"key":"thierry.reding@gmail.com","avatar":null},"body":"On Mon, Nov 28, 2022 at 06:07:38PM -0500, Taylor Blau wrote:\n> Hi Thierry,\n> \n> This is looking pretty good. Thanks for pushing out a couple of new\n> rounds. I took a look at the discussion and have some thoughts below,\n> but they are minor.\n> \n> This will likely not be in the upcoming v2.39.0, since Junio has already\n> cut and pushed the first release candidate tag. But you should feel free\n> to keep working on it in the meantime and we can revisit it post-2.39.\n\nThat's fine. No rush. I've had local workarounds for this for a very\nlong time, so I can be patient for a little longer. =)\n\n> On Mon, Nov 28, 2022 at 06:48:25PM +0100, Thierry Reding wrote:\n> > diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\n> > index 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> \n> Spelling this as \"[--no-verify]\" and not \"[--[no-]verify]\" is right,\n> since the option itself is called \"--no-verify\" (and thus has the\n> OPT_NONEG flag bit set), and \"--verify\" does not exist. Good.\n> \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> \n> Should we be more explicit here, given the discussion in the earlier\n> rounds? IOW, instead of saying that they are \"bypassed\" (which might\n> lead the reader to believe that they are run and their output/exit code\n> ignored), should we say that they are \"not run\" and be clearer?\n\nI adopted the wording as for git commit and since the behavior is\nexactly the same, it seemed best to stay consistent with that. My\ninterpretation of \"bypassed\" is equivalent to \"unused\" or \"ignored\",\nthough \"ignored\" in this case relating to their existence rather than\nreturn value.\n\nEither wording is fine with me, so if you feel strongly I can reword\nthis. In that case it might be worth updating the git commit\ndocumentation as well to avoid the same ambiguity.\n\n> \n> The (elided) implementation below looks good to me.\n> \n> > diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n> > index cdad4b688078..84e6bebeca7a 100755\n> > --- a/t/t4150-am.sh\n> > +++ b/t/t4150-am.sh\n> > @@ -345,6 +345,16 @@ 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> > +\texit 1\n> > +\tEOF\n> > +\tgit am --no-verify patch1\n> \n> Should we verify that the patch was applied with the expected message?\n\nYes, that's a good point. I've updated both tests to check that the\npatch has been applied and the scripts haven't been run.\n\nThanks,\nThierry\n"},{"id":"468163","messageId":"Y4XY/tUT+vKOrabX@orome","threadId":"58865","inReplyTo":"xmqqv8my4kvu.fsf@gitster.g","subject":"Re: [PATCH v3] am: Allow passing --no-verify flag","fromName":"Thierry Reding","fromEmail":"thierry.reding@gmail.com","sentAt":"2022-11-29T10:03:42Z","receivedAt":"2022-11-29T10:03:49Z","isPatch":true,"sender":{"key":"thierry.reding@gmail.com","avatar":null},"body":"On Tue, Nov 29, 2022 at 10:20:05AM +0900, Junio C Hamano wrote:\n> Thierry Reding <thierry.reding@gmail.com> writes:\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> > +\texit 1\n> > +\tEOF\n> > +\tgit am --no-verify patch1\n> > +'\n> > +\n> >  test_expect_success 'am with pre-applypatch hook' '\n> >  \trm -fr .git/rebase-apply &&\n> >  \tgit reset --hard &&\n> > @@ -374,6 +384,16 @@ 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> > +\ttest_hook pre-applypatch <<-\\EOF &&\n> > +\texit 1\n> > +\tEOF\n> > +\tgit am --no-verify patch1\n> > +'\n> > +\n> >  test_expect_success 'am with post-applypatch hook' '\n> >  \trm -fr .git/rebase-apply &&\n> >  \tgit reset --hard &&\n> \n> These two tests will still pass if you change the implementation to\n> run the hook and simply ignore its exit status, but I recall you\n> making a good argument against that alternative implementation ,in\n> comparison to \"not running the hook at all\".\n> \n> I think these tests should make sure that the hooks did not even\n> run.  Perhaps by creating a marker file before running \"git am\",\n> adding a \"rm\" that marker file in the hook, and making sure that\n> the marker file still exists after \"git am\" returns, or something\n> like that.\n\nAll good points. I've updated both tests to check that patches have been\napplied correctly and that the scripts haven't been run.\n\nThierry\n"}]}