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

[Johannes Sixt <j6t@kdbg.org>]

Am 10.01.2015 um 07:49 schrieb Richard Hansen:
> Make sure hooks are executed at the top-level directory and that
> GIT_PREFIX is set (as documented).
> 
> Signed-off-by: Richard Hansen <rhansen@bbn.com>
> ---
>  t/t1020-subdirectory.sh | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
> index 2edb4f2..03bb0a2 100755
> --- a/t/t1020-subdirectory.sh
> +++ b/t/t1020-subdirectory.sh
> @@ -128,6 +128,23 @@ test_expect_success !MINGW '!alias expansion' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'hook pwd' '
> +	pwd >expect &&
> +	(
> +		rm -f actual &&
> +		mkdir -p .git/hooks &&
> +		! test -e .git/hooks/post-checkout &&

What is the purpose of this test?

> +		cat <<-\EOF >.git/hooks/post-checkout &&
> +			#!/bin/sh
> +			pwd >actual
> +		EOF
> +		chmod +x .git/hooks/post-checkout &&

Use write_script() to construct a shell script.

> +		(cd dir && git checkout -- two) &&
> +		rm -f .git/hooks/post-checkout

This cleanup would be skipped if the checkout fails for some reason. Use
test_when_finished.

> +	) &&

The outer sub-shell us unnecessary, isn't it?

> +	test_cmp expect actual

If 'git checkout' runs the hook from the wrong directory, there would
not exist a file 'actual' at this point because it was rm -f'd earlier,
and the test would fail. Perhaps it would make sense to document this
failure case by inserting

	test_path_is_file actual &&

before the test_cmp?

Which makes me think: Would the test for existence of 'actual' be
sufficient? Then the test_cmp could be omitted. The advantage is that we
do not depend on how the `pwd` is formatted: With or without symbolic
links in any leading path or c:/foo vs. /c/foo on Windows. (I anticipate
that the test as written fails on Windows because 'expect' is in c:/foo
form and 'actual' is in /c/foo form.)

> +'
> +
>  test_expect_success 'GIT_PREFIX for !alias' '
>  	printf "dir/" >expect &&
>  	(
> @@ -154,6 +171,23 @@ 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 &&
> +		! test -e .git/hooks/post-checkout &&
> +		cat <<-\EOF >.git/hooks/post-checkout &&
> +			#!/bin/sh
> +			printf %s "$GIT_PREFIX" >actual
> +		EOF
> +		chmod +x .git/hooks/post-checkout &&
> +		(cd dir && git checkout -- two) &&
> +		rm -f .git/hooks/post-checkout
> +	)  &&

The comments about the sub-shell, write_script, and clean-up apply here,
too.

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

-- Hannes