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

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 28, 2022, 23:07 UTC
Message-ID: <Y4U/OhHSg85UV+5L@nand.local>
In-Reply-To: <20221128174825.1510407-1-thierry.reding@gmail.com>

Hi Thierry,

This is looking pretty good. Thanks for pushing out a couple of new rounds. I took a look at the discussion and have some thoughts below, but they are minor.

This will likely not be in the upcoming v2.39.0, since Junio has already cut and pushed the first release candidate tag. But you should feel free to keep working on it in the meantime and we can revisit it post-2.39.

On Mon, Nov 28, 2022 at 06:48:25PM +0100, Thierry Reding wrote:

Show 10 quoted lines

> 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]

Spelling this as "[--no-verify]" and not "[--[no-]verify]" is right, since the option itself is called "--no-verify" (and thus has the OPT_NONEG flag bit set), and "--verify" does not exist. Good.

Show 11 quoted lines

>  	 [--[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.

Should we be more explicit here, given the discussion in the earlier rounds? IOW, instead of saying that they are "bypassed" (which might lead the reader to believe that they are run and their output/exit code ignored), should we say that they are "not run" and be clearer?

The (elided) implementation below looks good to me.

Show 16 quoted lines

> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index cdad4b688078..84e6bebeca7a 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -345,6 +345,16 @@ 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 &&
> +	exit 1
> +	EOF
> +	git am --no-verify patch1

Should we verify that the patch was applied with the expected message?

Show 17 quoted lines

> +'
> +
>  test_expect_success 'am with pre-applypatch hook' '
>  	rm -fr .git/rebase-apply &&
>  	git reset --hard &&
> @@ -374,6 +384,16 @@ 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 &&
> +	test_hook pre-applypatch <<-\EOF &&
> +	exit 1
> +	EOF
> +	git am --no-verify patch1

Ditto here.

Thanks, Taylor