On Tue, Nov 29, 2022 at 10:20:05AM +0900, Junio C Hamano wrote:
> Thierry Reding <thierry.reding@gmail.com> writes:
> 
> > +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
> > +'
> > +
> >  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
> > +'
> > +
> >  test_expect_success 'am with post-applypatch hook' '
> >  	rm -fr .git/rebase-apply &&
> >  	git reset --hard &&
> 
> These two tests will still pass if you change the implementation to
> run the hook and simply ignore its exit status, but I recall you
> making a good argument against that alternative implementation ,in
> comparison to "not running the hook at all".
> 
> I think these tests should make sure that the hooks did not even
> run.  Perhaps by creating a marker file before running "git am",
> adding a "rm" that marker file in the hook, and making sure that
> the marker file still exists after "git am" returns, or something
> like that.

All good points. I've updated both tests to check that patches have been
applied correctly and that the scripts haven't been run.

Thierry