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

[Junio C Hamano <gitster@pobox.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.

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

> +'
> +
>  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 &&
>  	(