[PATCH v2] am: Allow passing --no-verify flag

[PATCH v2] am: Allow passing --no-verify flag

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The git-am --no-verify flag is analogous to the same flag passed to
git-commit. It bypasses the pre-applypatch and applypatch-msg hooks
if they are enabled.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- add test to verify that the new option works

 Documentation/git-am.txt |  8 +++++-
 builtin/am.c             | 11 ++++++--
 t/t4154-am-noverify.sh   | 60 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 4 deletions(-)
 create mode 100755 t/t4154-am-noverify.sh

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 326276e51ce5..0c1dfb3c98b4 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -9,7 +9,7 @@ git-am - Apply a series of patches from a mailbox
 SYNOPSIS
 --------
 [verse]
-'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]
+'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8] [--no-verify]
 	 [--[no-]3way] [--interactive] [--committer-date-is-author-date]
 	 [--ignore-date] [--ignore-space-change | --ignore-whitespace]
 	 [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]
@@ -138,6 +138,12 @@ include::rerere-options.txt[]
 --interactive::
 	Run interactively.
 
+-n::
+--no-verify::
+	By default, the pre-applypatch and applypatch-msg hooks are run.
+	When any of `--no-verify` or `-n` is given, these are bypassed.
+	See also linkgit:githooks[5].
+
 --committer-date-is-author-date::
 	By default the command records the date from the e-mail
 	message as the commit author date, and uses the time of
diff --git a/builtin/am.c b/builtin/am.c
index 20aea0d2487b..26ad8a468dc4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -117,6 +117,7 @@ struct am_state {
 
 	/* various operating modes and command line options */
 	int interactive;
+	int no_verify;
 	int threeway;
 	int quiet;
 	int signoff; /* enum signoff_type */
@@ -472,10 +473,12 @@ static void am_destroy(const struct am_state *state)
  */
 static int run_applypatch_msg_hook(struct am_state *state)
 {
-	int ret;
+	int ret = 0;
 
 	assert(state->msg);
-	ret = run_hooks_l("applypatch-msg", am_path(state, "final-commit"), NULL);
+
+	if (!state->no_verify)
+		ret = run_hooks_l("applypatch-msg", am_path(state, "final-commit"), NULL);
 
 	if (!ret) {
 		FREE_AND_NULL(state->msg);
@@ -1640,7 +1643,7 @@ static void do_commit(const struct am_state *state)
 	const char *reflog_msg, *author, *committer = NULL;
 	struct strbuf sb = STRBUF_INIT;
 
-	if (run_hooks("pre-applypatch"))
+	if (!state->no_verify && run_hooks("pre-applypatch"))
 		exit(1);
 
 	if (write_cache_as_tree(&tree, 0, NULL))
@@ -2329,6 +2332,8 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL('i', "interactive", &state.interactive,
 			N_("run interactively")),
+		OPT_BOOL('n', "no-verify", &state.no_verify,
+			N_("bypass pre-applypatch and applypatch-msg hooks")),
 		OPT_HIDDEN_BOOL('b', "binary", &binary,
 			N_("historical option -- no-op")),
 		OPT_BOOL('3', "3way", &state.threeway,
diff --git a/t/t4154-am-noverify.sh b/t/t4154-am-noverify.sh
new file mode 100755
index 000000000000..fbf45998243f
--- /dev/null
+++ b/t/t4154-am-noverify.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='git am --no-verify'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo "root" >file &&
+	git add file &&
+	git commit -m "zeroth" &&
+	git branch side-success &&
+	git branch side-failure &&
+	echo "foo" >>file &&
+	git add file &&
+	git commit -m "first" &&
+	git format-patch --stdout HEAD^ >patch1
+'
+
+setup_success_hook () {
+	test_when_finished "rm -f actual_hooks expected_hooks" &&
+	echo "$1" >expected_hooks &&
+	test_hook "$1" <<-EOF
+	echo $1 >>actual_hooks
+	EOF
+}
+
+test_expect_success '--no-verify with succeeding hook' '
+	setup_success_hook "pre-applypatch" &&
+	setup_success_hook "applypatch-msg" &&
+	git checkout side-success &&
+	git am --no-verify patch1
+'
+
+setup_failing_hook () {
+	test_when_finished "rm -f actual_hooks" &&
+	test_hook "$1" <<-EOF
+	echo $1-failing-hook >>actual_hooks
+	exit 1
+	EOF
+}
+
+test_expect_failure 'with failing hook' '
+	test_when_finished "git am --abort" &&
+	setup_failing_hook "pre-applypatch" &&
+	setup_failing_hook "applypatch-msg" &&
+	git checkout side-failure &&
+	git am patch1
+'
+
+test_expect_success '--no-verify with failing hook' '
+	setup_success_hook "pre-applypatch" &&
+	setup_success_hook "applypatch-msg" &&
+	git checkout side-failure &&
+	git am --no-verify patch1
+'
+
+test_done
-- 
2.38.1

Re: [PATCH v2] am: Allow passing --no-verify flag

[Junio C Hamano]

Thierry Reding <thierry.reding@gmail.com> writes:

> From: Thierry Reding <treding@nvidia.com>
>
> The git-am --no-verify flag is analogous to the same flag passed to
> git-commit. It bypasses the pre-applypatch and applypatch-msg hooks
> if they are enabled.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - add test to verify that the new option works

> +-n::
> +--no-verify::
> +	By default, the pre-applypatch and applypatch-msg hooks are run.
> +	When any of `--no-verify` or `-n` is given, these are bypassed.
> +	See also linkgit:githooks[5].

I think the goal of this topic is to allow bypassing the checks made
by these two hooks (and possibly future ones that validate the input
to "am"), and there are at least two possible implementations to
achieve that goal.  You can still run the hook and ignore its
failure exit, or you can skip running the hook and pretend as if
hook succeeded.

As it is documented that applypatch-msg is allowed to edit the
message file to normalize the message, between the two, running the
hook (to allow the hook to automatically edit the message) but
ignoring its failure would be a more intuitive approach to "bypass"
the check.  If the option were called --no-hook or --bypass-hooks
then it would be a different story, though.

>  	assert(state->msg);
> -	ret = run_hooks_l("applypatch-msg", am_path(state, "final-commit"), NULL);
> +
> +	if (!state->no_verify)
> +		ret = run_hooks_l("applypatch-msg", am_path(state, "final-commit"), NULL);

And it seems that this took a less intuitive avenue of bypassing the
hook completely.  I am not 100% convinced that this is the better
choice (but I am not convinced it is the worse one, either).

> diff --git a/t/t4154-am-noverify.sh b/t/t4154-am-noverify.sh
> new file mode 100755
> index 000000000000..fbf45998243f
> --- /dev/null
> +++ b/t/t4154-am-noverify.sh
> @@ -0,0 +1,60 @@
> +#!/bin/sh
> +

It is surprising, and I am not enthused to see, that this needs an
entirely new script.

Don't we already have a script or two to test "am", among which the
invocation of hooks is already tested?

Re: [PATCH v2] am: Allow passing --no-verify flag

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3003 bytes --]

On Mon, Nov 21, 2022 at 02:33:06PM +0900, Junio C Hamano wrote:
> Thierry Reding <thierry.reding@gmail.com> writes:
> 
> > From: Thierry Reding <treding@nvidia.com>
> >
> > The git-am --no-verify flag is analogous to the same flag passed to
> > git-commit. It bypasses the pre-applypatch and applypatch-msg hooks
> > if they are enabled.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v2:
> > - add test to verify that the new option works
> 
> > +-n::
> > +--no-verify::
> > +	By default, the pre-applypatch and applypatch-msg hooks are run.
> > +	When any of `--no-verify` or `-n` is given, these are bypassed.
> > +	See also linkgit:githooks[5].
> 
> I think the goal of this topic is to allow bypassing the checks made
> by these two hooks (and possibly future ones that validate the input
> to "am"), and there are at least two possible implementations to
> achieve that goal.  You can still run the hook and ignore its
> failure exit, or you can skip running the hook and pretend as if
> hook succeeded.
> 
> As it is documented that applypatch-msg is allowed to edit the
> message file to normalize the message, between the two, running the
> hook (to allow the hook to automatically edit the message) but
> ignoring its failure would be a more intuitive approach to "bypass"
> the check.  If the option were called --no-hook or --bypass-hooks
> then it would be a different story, though.
> 
> >  	assert(state->msg);
> > -	ret = run_hooks_l("applypatch-msg", am_path(state, "final-commit"), NULL);
> > +
> > +	if (!state->no_verify)
> > +		ret = run_hooks_l("applypatch-msg", am_path(state, "final-commit"), NULL);
> 
> And it seems that this took a less intuitive avenue of bypassing the
> hook completely.  I am not 100% convinced that this is the better
> choice (but I am not convinced it is the worse one, either).

Thinking a bit more about this, if we let applypatch-msg run but ignore
failures and continue on to commit the result, wouldn't that potentially
allow committing garbage? I'm thinking about cases where applypatch-msg
may attempt to normalize the message and fails badly, leaving a partial
commit message or none at all.

The primary use-case where I'd like to use this new option for git am is
when the pre-applypatch hook fails and that has less of the risks
associated with applypatch-msg, so perhaps --no-verify should only apply
to pre-applypatch?

> 
> > diff --git a/t/t4154-am-noverify.sh b/t/t4154-am-noverify.sh
> > new file mode 100755
> > index 000000000000..fbf45998243f
> > --- /dev/null
> > +++ b/t/t4154-am-noverify.sh
> > @@ -0,0 +1,60 @@
> > +#!/bin/sh
> > +
> 
> It is surprising, and I am not enthused to see, that this needs an
> entirely new script.
> 
> Don't we already have a script or two to test "am", among which the
> invocation of hooks is already tested?

I can move the tests to the corresponding sections in t/t4150-am.sh.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] am: Allow passing --no-verify flag

[Junio C Hamano]

Thierry Reding <thierry.reding@gmail.com> writes:

> Thinking a bit more about this, if we let applypatch-msg run but ignore
> failures and continue on to commit the result, wouldn't that potentially
> allow committing garbage? I'm thinking about cases where applypatch-msg
> may attempt to normalize the message and fails badly, leaving a partial
> commit message or none at all.

That is a very reasonable way to think about this.  So I am sold on
your approach of not running the hook at all.

Thanks.