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

[Junio C Hamano <gitster@pobox.com>]

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?