From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sun, 05 Jul 2015 15:58:49 GMT
Subject: Re: [PATCH 09/12] t4150: am with post-applypatch hook
Message-ID: <17f64cb49c820eb41d0fb58435c91c40@www.dscho.org>
In-Reply-To: <1435861000-25278-10-git-send-email-pyokagan@gmail.com>

Hi Paul,

On 2015-07-02 20:16, Paul Tan wrote:

> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index dd6fe81..62b678c 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -275,6 +275,48 @@ test_expect_success 'am with failing pre-applypatch hook' '
>  	test_cmp_rev first HEAD
>  '
>  
> +test_expect_success 'am with post-applypatch hook' '
> +	test_when_finished "rm -f .git/hooks/post-applypatch" &&
> +	rm -fr .git/rebase-apply &&
> +	git reset --hard &&
> +	git checkout first &&
> +	mkdir -p .git/hooks &&
> +	cat >.git/hooks/post-applypatch <<-\EOF &&
> +	#!/bin/sh
> +	git rev-parse HEAD >head.actual
> +	git diff second >diff.actual
> +	exit 0
> +	EOF
> +	chmod +x .git/hooks/post-applypatch &&
> +	git am patch1 &&
> +	test_path_is_missing .git/rebase-apply &&
> +	test_cmp_rev second HEAD &&
> +	git rev-parse second >head.expected &&
> +	test_cmp head.expected head.actual &&
> +	git diff second >diff.expected &&
> +	test_cmp diff.expected diff.actual
> +'
> +
> +test_expect_success 'am with failing post-applypatch hook' '
> +	test_when_finished "rm -f .git/hooks/post-applypatch" &&
> +	rm -fr .git/rebase-apply &&
> +	git reset --hard &&
> +	git checkout first &&
> +	mkdir -p .git/hooks &&
> +	cat >.git/hooks/post-applypatch <<-\EOF &&
> +	#!/bin/sh
> +	git rev-parse HEAD >head.actual
> +	exit 1
> +	EOF
> +	chmod +x .git/hooks/post-applypatch &&
> +	git am patch1 &&
> +	test_path_is_missing .git/rebase-apply &&
> +	git diff --exit-code second &&
> +	test_cmp_rev second HEAD &&
> +	git rev-parse second >head.expected &&
> +	test_cmp head.expected head.actual
> +'

These 2 tests as well as the previous patches look to me as if they could be refactored (the paradigm is the same: add a certain hook after resetting and then apply the patch, verify that the hook ran/failed)... do you think there is a chance for that?

Just to make sure: I think the patch series looks good, and if it would be too cumbersome (or just not feasible) to simplify those test cases, I am totally fine with them as-are.

Ciao,
Dscho