Re: [PATCH v2 2/2] t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 12, 2015, 22:38 UTC
Message-ID: <xmqq4mrv7hgg.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1420931503-22857-3-git-send-email-rhansen@bbn.com>

Richard Hansen <rhansen@bbn.com> writes:

> Make sure hooks are executed at the top-level directory and that
> GIT_PREFIX is set (as documented).

The same comment as the one for 1/2 applies here. If we substitute 'hook' everywhere with 'post-checkout hook' in this patch, it makes perfect sense to me, but otherwise this is far from "check _hook_" in general.

Show 22 quoted lines

> Signed-off-by: Richard Hansen <rhansen@bbn.com>
> ---
>  t/t1020-subdirectory.sh | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
> index 2edb4f2..0ccbb7e 100755
> --- a/t/t1020-subdirectory.sh
> +++ b/t/t1020-subdirectory.sh
> @@ -128,6 +128,17 @@ test_expect_success !MINGW '!alias expansion' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'hook pwd' '
> +	rm -f actual &&
> +	mkdir -p .git/hooks &&
> +	write_script .git/hooks/post-checkout <<-\EOF &&
> +		pwd >actual
> +	EOF
> +	test_when_finished "rm -f .git/hooks/post-checkout actual" &&
> +	(cd dir && git checkout -- two) &&
> +	test_path_is_file actual

Cute, but it is misleading to use "pwd" there, because the contents of the file does not matter for this test, even though the test is about the current directory. It forces the reader to look for the place where you are comparing the contents of that file with expected path to the current directory, and no such code exists.

"date >actual", "echo >actual", or even just a redirection without command, i.e. ">actual", woudl have been easier to see what is going on (I would have used the last form if I were doing this patch).

Show 20 quoted lines

> +'
> +
>  test_expect_success 'GIT_PREFIX for !alias' '
>  	printf "dir/" >expect &&
>  	(
> @@ -154,6 +165,18 @@ test_expect_success 'GIT_PREFIX for built-ins' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'GIT_PREFIX for hooks' '
> +	printf "dir/" >expect &&
> +	rm -f actual &&
> +	mkdir -p .git/hooks &&
> +	write_script .git/hooks/post-checkout <<-\EOF &&
> +		printf %s "$GIT_PREFIX" >actual
> +	EOF
> +	test_when_finished "rm -f .git/hooks/post-checkout expect actual" &&
> +	(cd dir && git checkout -- two) &&
> +	test_cmp expect actual
> +'

It is not wrong per-se, but the same cute trick could have been used, i.e.

	write_script ... post-checkout <<-\EOF &&
        >"$GIT_PREFIX/actual"
        EOF
        ...
        test_path_is_file dir/actual

> +
>  test_expect_success 'no file/rev ambiguity check inside .git' '
>  	git commit -a -m 1 &&
>  	(