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

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

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.