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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Richard Hansen <rhansen@xxxxxxx> 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@xxxxxxx>
> ---
>  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 &&
>  	(
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]