{"thread":{"id":"58818","subject":"[PATCH v2] am: Allow passing --no-verify flag","startedAt":"2022-11-19T01:43:27Z","lastAt":"2022-11-27T01:27:37Z","messageCount":4,"participants":["Thierry Reding","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"467548","messageId":"20221119005031.3170699-1-thierry.reding@gmail.com","threadId":"58818","inReplyTo":null,"subject":"[PATCH v2] am: Allow passing --no-verify flag","fromName":"Thierry Reding","fromEmail":"thierry.reding@gmail.com","sentAt":"2022-11-19T00:50:31Z","receivedAt":"2022-11-19T01:43:27Z","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 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/t4154-am-noverify.sh   | 60 ++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 75 insertions(+), 4 deletions(-)\n create mode 100755 t/t4154-am-noverify.sh\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 20aea0d2487b..26ad8a468dc4 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@@ -2329,6 +2332,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/t4154-am-noverify.sh b/t/t4154-am-noverify.sh\nnew file mode 100755\nindex 000000000000..fbf45998243f\n--- /dev/null\n+++ b/t/t4154-am-noverify.sh\n@@ -0,0 +1,60 @@\n+#!/bin/sh\n+\n+test_description='git am --no-verify'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo \"root\" >file &&\n+\tgit add file &&\n+\tgit commit -m \"zeroth\" &&\n+\tgit branch side-success &&\n+\tgit branch side-failure &&\n+\techo \"foo\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"first\" &&\n+\tgit format-patch --stdout HEAD^ >patch1\n+'\n+\n+setup_success_hook () {\n+\ttest_when_finished \"rm -f actual_hooks expected_hooks\" &&\n+\techo \"$1\" >expected_hooks &&\n+\ttest_hook \"$1\" <<-EOF\n+\techo $1 >>actual_hooks\n+\tEOF\n+}\n+\n+test_expect_success '--no-verify with succeeding hook' '\n+\tsetup_success_hook \"pre-applypatch\" &&\n+\tsetup_success_hook \"applypatch-msg\" &&\n+\tgit checkout side-success &&\n+\tgit am --no-verify patch1\n+'\n+\n+setup_failing_hook () {\n+\ttest_when_finished \"rm -f actual_hooks\" &&\n+\ttest_hook \"$1\" <<-EOF\n+\techo $1-failing-hook >>actual_hooks\n+\texit 1\n+\tEOF\n+}\n+\n+test_expect_failure 'with failing hook' '\n+\ttest_when_finished \"git am --abort\" &&\n+\tsetup_failing_hook \"pre-applypatch\" &&\n+\tsetup_failing_hook \"applypatch-msg\" &&\n+\tgit checkout side-failure &&\n+\tgit am patch1\n+'\n+\n+test_expect_success '--no-verify with failing hook' '\n+\tsetup_success_hook \"pre-applypatch\" &&\n+\tsetup_success_hook \"applypatch-msg\" &&\n+\tgit checkout side-failure &&\n+\tgit am --no-verify patch1\n+'\n+\n+test_done\n-- \n2.38.1\n\n"},{"id":"467658","messageId":"xmqqilj8yir1.fsf@gitster.g","threadId":"58818","inReplyTo":"20221119005031.3170699-1-thierry.reding@gmail.com","subject":"Re: [PATCH v2] am: Allow passing --no-verify flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-21T05:33:06Z","receivedAt":"2022-11-21T05:33:13Z","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> From: Thierry Reding <treding@nvidia.com>\n>\n> The git-am --no-verify flag is analogous to the same flag passed to\n> git-commit. It bypasses the pre-applypatch and applypatch-msg hooks\n> if they are enabled.\n>\n> Signed-off-by: Thierry Reding <treding@nvidia.com>\n> ---\n> Changes in v2:\n> - add test to verify that the new option works\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\nI think the goal of this topic is to allow bypassing the checks made\nby these two hooks (and possibly future ones that validate the input\nto \"am\"), and there are at least two possible implementations to\nachieve that goal.  You can still run the hook and ignore its\nfailure exit, or you can skip running the hook and pretend as if\nhook succeeded.\n\nAs it is documented that applypatch-msg is allowed to edit the\nmessage file to normalize the message, between the two, running the\nhook (to allow the hook to automatically edit the message) but\nignoring its failure would be a more intuitive approach to \"bypass\"\nthe check.  If the option were called --no-hook or --bypass-hooks\nthen it would be a different story, though.\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\nAnd it seems that this took a less intuitive avenue of bypassing the\nhook completely.  I am not 100% convinced that this is the better\nchoice (but I am not convinced it is the worse one, either).\n\n> diff --git a/t/t4154-am-noverify.sh b/t/t4154-am-noverify.sh\n> new file mode 100755\n> index 000000000000..fbf45998243f\n> --- /dev/null\n> +++ b/t/t4154-am-noverify.sh\n> @@ -0,0 +1,60 @@\n> +#!/bin/sh\n> +\n\nIt is surprising, and I am not enthused to see, that this needs an\nentirely new script.\n\nDon't we already have a script or two to test \"am\", among which the\ninvocation of hooks is already tested?\n"},{"id":"468006","messageId":"Y4D+UU6l3YN9r9Xn@orome","threadId":"58818","inReplyTo":"xmqqilj8yir1.fsf@gitster.g","subject":"Re: [PATCH v2] am: Allow passing --no-verify flag","fromName":"Thierry Reding","fromEmail":"thierry.reding@gmail.com","sentAt":"2022-11-25T17:41:37Z","receivedAt":"2022-11-25T17:41:44Z","isPatch":true,"sender":{"key":"thierry.reding@gmail.com","avatar":null},"body":"On Mon, Nov 21, 2022 at 02:33:06PM +0900, Junio C Hamano wrote:\n> Thierry Reding <thierry.reding@gmail.com> writes:\n> \n> > From: Thierry Reding <treding@nvidia.com>\n> >\n> > The git-am --no-verify flag is analogous to the same flag passed to\n> > git-commit. It bypasses the pre-applypatch and applypatch-msg hooks\n> > if they are enabled.\n> >\n> > Signed-off-by: Thierry Reding <treding@nvidia.com>\n> > ---\n> > Changes in v2:\n> > - add test to verify that the new option works\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> I think the goal of this topic is to allow bypassing the checks made\n> by these two hooks (and possibly future ones that validate the input\n> to \"am\"), and there are at least two possible implementations to\n> achieve that goal.  You can still run the hook and ignore its\n> failure exit, or you can skip running the hook and pretend as if\n> hook succeeded.\n> \n> As it is documented that applypatch-msg is allowed to edit the\n> message file to normalize the message, between the two, running the\n> hook (to allow the hook to automatically edit the message) but\n> ignoring its failure would be a more intuitive approach to \"bypass\"\n> the check.  If the option were called --no-hook or --bypass-hooks\n> then it would be a different story, though.\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> And it seems that this took a less intuitive avenue of bypassing the\n> hook completely.  I am not 100% convinced that this is the better\n> choice (but I am not convinced it is the worse one, either).\n\nThinking a bit more about this, if we let applypatch-msg run but ignore\nfailures and continue on to commit the result, wouldn't that potentially\nallow committing garbage? I'm thinking about cases where applypatch-msg\nmay attempt to normalize the message and fails badly, leaving a partial\ncommit message or none at all.\n\nThe primary use-case where I'd like to use this new option for git am is\nwhen the pre-applypatch hook fails and that has less of the risks\nassociated with applypatch-msg, so perhaps --no-verify should only apply\nto pre-applypatch?\n\n> \n> > diff --git a/t/t4154-am-noverify.sh b/t/t4154-am-noverify.sh\n> > new file mode 100755\n> > index 000000000000..fbf45998243f\n> > --- /dev/null\n> > +++ b/t/t4154-am-noverify.sh\n> > @@ -0,0 +1,60 @@\n> > +#!/bin/sh\n> > +\n> \n> It is surprising, and I am not enthused to see, that this needs an\n> entirely new script.\n> \n> Don't we already have a script or two to test \"am\", among which the\n> invocation of hooks is already tested?\n\nI can move the tests to the corresponding sections in t/t4150-am.sh.\n\nThierry\n"},{"id":"468032","messageId":"xmqqv8n19ofx.fsf@gitster.g","threadId":"58818","inReplyTo":"Y4D+UU6l3YN9r9Xn@orome","subject":"Re: [PATCH v2] am: Allow passing --no-verify flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-27T01:27:30Z","receivedAt":"2022-11-27T01:27:37Z","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> Thinking a bit more about this, if we let applypatch-msg run but ignore\n> failures and continue on to commit the result, wouldn't that potentially\n> allow committing garbage? I'm thinking about cases where applypatch-msg\n> may attempt to normalize the message and fails badly, leaving a partial\n> commit message or none at all.\n\nThat is a very reasonable way to think about this.  So I am sold on\nyour approach of not running the hook at all.\n\nThanks.\n"}]}