| Message ID | 20221119005031.3170699-1-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] am: Allow passing --no-verify flag | expand |
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?
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
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.
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