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

[PATCH v4] 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 v4:
- check that scripts haven't run and that commits have been applied

Changes in v3:
- move tests to existing t/t4150-am

Changes in v2:
- add test to verify that the new option works

 Documentation/git-am.txt |  8 +++++++-
 builtin/am.c             | 11 ++++++++---
 t/t4150-am.sh            | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 4 deletions(-)

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 30c9b3a9cd72..dfe8104f570d 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))
@@ -2330,6 +2333,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/t4150-am.sh b/t/t4150-am.sh
index cdad4b688078..d77c4dcefeb7 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -345,6 +345,22 @@ test_expect_success 'am with failing applypatch-msg hook' '
 	test_cmp_rev first HEAD
 '
 
+test_expect_success 'am with failing applypatch-msg hook (no verify)' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
+	git checkout first &&
+	test_hook applypatch-msg <<-\EOF &&
+	echo hook-message >"$1"
+	exit 1
+	EOF
+	git am --no-verify patch1 &&
+	test_path_is_missing .git/rebase-apply &&
+	git diff --exit-code second &&
+	test_cmp_rev second HEAD &&
+	git log -1 --format=format:%B >actual &&
+	test_cmp msg actual
+'
+
 test_expect_success 'am with pre-applypatch hook' '
 	rm -fr .git/rebase-apply &&
 	git reset --hard &&
@@ -374,6 +390,22 @@ test_expect_success 'am with failing pre-applypatch hook' '
 	test_cmp_rev first HEAD
 '
 
+test_expect_success 'am with failing pre-applypatch hook (no verify)' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
+	git checkout first &&
+	touch empty-file &&
+	test_hook pre-applypatch <<-\EOF &&
+	rm empty-file
+	exit 1
+	EOF
+	git am --no-verify patch1 &&
+	test_path_is_missing .git/rebase-apply &&
+	git diff --exit-code second &&
+	test_cmp_rev second HEAD &&
+	test_path_is_file empty-file
+'
+
 test_expect_success 'am with post-applypatch hook' '
 	rm -fr .git/rebase-apply &&
 	git reset --hard &&
-- 
2.38.1

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

[Junio C Hamano]

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

> +		OPT_BOOL('n', "no-verify", &state.no_verify,
> +			N_("bypass pre-applypatch and applypatch-msg hooks")),

I think parse_options machinery is smart enough to do the right to
allow "git am --no-verify --verify" a way to express "last one wins,
turn the .no_verify bit off to countermand an earlier --no-verify",
so this looks good.

> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index cdad4b688078..d77c4dcefeb7 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -345,6 +345,22 @@ test_expect_success 'am with failing applypatch-msg hook' '
>  	test_cmp_rev first HEAD
>  '
>  
> +test_expect_success 'am with failing applypatch-msg hook (no verify)' '
> +	rm -fr .git/rebase-apply &&
> +	git reset --hard &&
> +	git checkout first &&
> +	test_hook applypatch-msg <<-\EOF &&
> +	echo hook-message >"$1"
> +	exit 1
> +	EOF
> +	git am --no-verify patch1 &&
> +	test_path_is_missing .git/rebase-apply &&
> +	git diff --exit-code second &&
> +	test_cmp_rev second HEAD &&

This somewhat raised my eyebrows, as the condition will not
generally hold.

I am guessing this was merely copied and pasted from the previous
test piece before this one, which uses "test_cmp_rev first HEAD" and
its use of test_cmp_rev is more sensible, as it is making sure that
"we started with 'first' commit at HEAD, 'am' should not have done
anything by failing, so expect HEAD is still at 'first'".

The reason to expect HEAD to be 'second' here is very different.  We
start from 'first', 'am' should have applied the patch as-is while
pretending that the wallclock is frozen (i.e. even the committer
timestamp should be the same as the original commit), so expect HEAD
is pointing at the exact same commit recreated by 'am'".  That is a
bit unrealistic expectation outside the context of the tests.

If we added test_commit or test_tick anywhere before this step in an
unrelated test, it likely will break this expectation; in other
words, I think the use of "test_cmp_rev second" here makes it
unnecessarily brittle.

> +	git log -1 --format=format:%B >actual &&
> +	test_cmp msg actual
> +'

Other than that, this test looks good.

> @@ -374,6 +390,22 @@ test_expect_success 'am with failing pre-applypatch hook' '
>  	test_cmp_rev first HEAD
>  '
>  
> +test_expect_success 'am with failing pre-applypatch hook (no verify)' '
> +	rm -fr .git/rebase-apply &&
> +	git reset --hard &&
> +	git checkout first &&
> +	touch empty-file &&
> +	test_hook pre-applypatch <<-\EOF &&
> +	rm empty-file
> +	exit 1
> +	EOF
> +	git am --no-verify patch1 &&
> +	test_path_is_missing .git/rebase-apply &&
> +	git diff --exit-code second &&
> +	test_cmp_rev second HEAD &&
> +	test_path_is_file empty-file
> +'

This "pre" stuff, unlike the other hook, does not affect the
resulting commit, so you invent an extra file "empty-file" as a
marker and remove it from the hook, so that the absense of the file
can be used as a signal that the hook did run.  We could do it the
other way around (i.e. make sure the marker file does not exist
before running "git am", arrange the marker to be created by running
the hook, and ensure that the marker does not exist after running
"git am"), and either way is fine.

The same comment as the previous one applies to "test_cmp_rev second"
check.  I think removing them would make the tests better.

Oh, also I keep forgetting but

> Subject: Re: [PATCH v4] am: Allow passing --no-verify flag

our convention is not capitalize "Allow" here.

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...
> The same comment as the previous one applies to "test_cmp_rev second"
> check.  I think removing them would make the tests better.

I will queue this on top for now.  Thanks.

 t/t4150-am.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git i/t/t4150-am.sh w/t/t4150-am.sh
index d77c4dcefe..50b257e43f 100755
--- i/t/t4150-am.sh
+++ w/t/t4150-am.sh
@@ -356,7 +356,6 @@ test_expect_success 'am with failing applypatch-msg hook (no verify)' '
 	git am --no-verify patch1 &&
 	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code second &&
-	test_cmp_rev second HEAD &&
 	git log -1 --format=format:%B >actual &&
 	test_cmp msg actual
 '
@@ -401,9 +400,10 @@ test_expect_success 'am with failing pre-applypatch hook (no verify)' '
 	EOF
 	git am --no-verify patch1 &&
 	test_path_is_missing .git/rebase-apply &&
+	test_path_is_file empty-file &&
 	git diff --exit-code second &&
-	test_cmp_rev second HEAD &&
-	test_path_is_file empty-file
+	git log -1 --format=format:%B >actual &&
+	test_cmp msg actual
 '
 
 test_expect_success 'am with post-applypatch hook' '

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

[Thierry Reding]

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

On Thu, Dec 01, 2022 at 09:55:18AM +0900, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > ...
> > The same comment as the previous one applies to "test_cmp_rev second"
> > check.  I think removing them would make the tests better.
> 
> I will queue this on top for now.  Thanks.

Just to clarify: do you want me to send out another version with your
changes on top or should I consider this done?

Thierry

> 
>  t/t4150-am.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git i/t/t4150-am.sh w/t/t4150-am.sh
> index d77c4dcefe..50b257e43f 100755
> --- i/t/t4150-am.sh
> +++ w/t/t4150-am.sh
> @@ -356,7 +356,6 @@ test_expect_success 'am with failing applypatch-msg hook (no verify)' '
>  	git am --no-verify patch1 &&
>  	test_path_is_missing .git/rebase-apply &&
>  	git diff --exit-code second &&
> -	test_cmp_rev second HEAD &&
>  	git log -1 --format=format:%B >actual &&
>  	test_cmp msg actual
>  '
> @@ -401,9 +400,10 @@ test_expect_success 'am with failing pre-applypatch hook (no verify)' '
>  	EOF
>  	git am --no-verify patch1 &&
>  	test_path_is_missing .git/rebase-apply &&
> +	test_path_is_file empty-file &&
>  	git diff --exit-code second &&
> -	test_cmp_rev second HEAD &&
> -	test_path_is_file empty-file
> +	git log -1 --format=format:%B >actual &&
> +	test_cmp msg actual
>  '
>  
>  test_expect_success 'am with post-applypatch hook' '

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

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

[Junio C Hamano]

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

> On Thu, Dec 01, 2022 at 09:55:18AM +0900, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > ...
>> > The same comment as the previous one applies to "test_cmp_rev second"
>> > check.  I think removing them would make the tests better.
>> 
>> I will queue this on top for now.  Thanks.
>
> Just to clarify: do you want me to send out another version with your
> changes on top or should I consider this done?

It is up to you.

 * If you agree with the "fix-up" done with the extra commit (marked
   as "SQUASH???") on top and there is no other changes you want to
   make, you can tell me to squash it into your patch.

 * If you don't and have better idea, you can send out an updated
   version, and you have my permission to include any (or all) parts
   of the changes I gave in the message you are responding to.

 * Or if you make a very good argument and convince me why your
   original without my fix-up is better, I may change my mind and
   drop the changes, keeping your original alone.

Of course, folks on the list may have other suggestions and review
comments, which may change the picture again ;-)

In any case, thanks for contributing!

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

[Thierry Reding]

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

On Wed, Dec 21, 2022 at 06:42:44PM +0900, Junio C Hamano wrote:
> Thierry Reding <thierry.reding@gmail.com> writes:
> 
> > On Thu, Dec 01, 2022 at 09:55:18AM +0900, Junio C Hamano wrote:
> >> Junio C Hamano <gitster@pobox.com> writes:
> >> 
> >> > ...
> >> > The same comment as the previous one applies to "test_cmp_rev second"
> >> > check.  I think removing them would make the tests better.
> >> 
> >> I will queue this on top for now.  Thanks.
> >
> > Just to clarify: do you want me to send out another version with your
> > changes on top or should I consider this done?
> 
> It is up to you.
> 
>  * If you agree with the "fix-up" done with the extra commit (marked
>    as "SQUASH???") on top and there is no other changes you want to
>    make, you can tell me to squash it into your patch.
> 
>  * If you don't and have better idea, you can send out an updated
>    version, and you have my permission to include any (or all) parts
>    of the changes I gave in the message you are responding to.
> 
>  * Or if you make a very good argument and convince me why your
>    original without my fix-up is better, I may change my mind and
>    drop the changes, keeping your original alone.
> 
> Of course, folks on the list may have other suggestions and review
> comments, which may change the picture again ;-)
> 
> In any case, thanks for contributing!

Your changes look fine. I applied them to my local tree and everything
still works as expected, so feel free to squash those into the commit.

Thanks,
Thierry

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

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

[Junio C Hamano]

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

> Your changes look fine. I applied them to my local tree and everything
> still works as expected, so feel free to squash those into the commit.

Thanks, will do.