Re: [PATCH v2] am: Allow passing --no-verify flag
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 21, 2022, 05:33 UTC
- Message-ID
- <xmqqilj8yir1.fsf@gitster.g>
- In-Reply-To
- <20221119005031.3170699-1-thierry.reding@gmail.com>
Thierry Reding <thierry.reding@gmail.com> writes:
Show 10 quoted lines
> 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
Show 5 quoted lines
> +-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.
Show 5 quoted lines
> 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).
Show 8 quoted lines
> 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?