Re: [PATCH 05/11] tests: use test_ln_s_add to remove SYMLINKS prerequisite (trivial cases)

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 4, 2013, 21:06 UTC
Message-ID: <7vwqq960wn.fsf@alter.siamese.dyndns.org>
In-Reply-To: <3c8aaaa4ad606d2afb7c110a28c119792caa8aba.1370076477.git.j6t@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

Show 13 quoted lines

> There are many instances where the treatment of symbolic links in the
> object model and the algorithms are tested, but where it is not
> necessary to actually have a symbolic link in the worktree. Make
> adjustments to the tests and remove the SYMLINKS prerequisite when
> appropriate in trivial cases, where "trivial" means:
>
> - merely a replacement of 'ln -s a b' to test_ln_s or of
>   'ln -s a b && git add b' to test_ln_s_add is needed;
>
> - a test for symbolic link on the file system can be split off (and
>   remains protected by SYMLINKS);
>
> - existing code is equivalent to test_ln_s[_add].

This is too big to review in one go, so I may have separate messages later on this same patch.

Show 17 quoted lines

> diff --git a/t/t2003-checkout-cache-mkdir.sh b/t/t2003-checkout-cache-mkdir.sh
> index ff163cf..bd17ba2 100755
> --- a/t/t2003-checkout-cache-mkdir.sh
> +++ b/t/t2003-checkout-cache-mkdir.sh
> @@ -19,10 +19,10 @@ test_expect_success 'setup' '
>  	git update-index --add path0 path1/file1
>  '
>  
> -test_expect_success SYMLINKS 'have symlink in place where dir is expected.' '
> +test_expect_success 'have symlink in place where dir is expected.' '
>  	rm -fr path0 path1 &&
>  	mkdir path2 &&
> -	ln -s path2 path1 &&
> +	test_ln_s path2 path1 &&
>  	git checkout-index -f -a &&
>  	test ! -h path1 && test -d path1 &&
>  	test -f path1/file1 && test ! -f path2/file1

I do not think this hunk is correct.

We have two regular files in the index: path0, path1/file1, and we add a symbolic link path1 that happens to point at directory path2/ in the working tree.

The test is about making sure that checkout-index is not confused by the symbolic link in the working tree, by attempting to checkout path1/file1.

Under the precondition checkout-index runs in this test, a casual

	echo rezrov >path1/file1

would leave path1 as a symlink without turning it into a real directory, and we will end up creating path2/file1. We are making sure that checkout-index does not behave that way, and it is essential to have symlink support in the working tree for the "bug" to trigger.

On a filesystem without symbolic links, the patched test would pass just fine, but there can be no aliasing between path1 and path2 in the first place.

Show 13 quoted lines

> @@ -79,10 +79,10 @@ test_expect_success SYMLINKS 'use --prefix=tmp/orary- where tmp is a symlink' '
>  	test -h tmp
>  '
>  
> -test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' '
> +test_expect_success 'use --prefix=tmp- where tmp-path1 is a symlink' '
>  	rm -fr path0 path1 path2 tmp* &&
>  	mkdir tmp1 &&
> -	ln -s tmp1 tmp-path1 &&
> +	test_ln_s tmp1 tmp-path1 &&
>  	git checkout-index --prefix=tmp- -f -a &&
>  	test -f tmp-path0 &&
>  	test ! -h tmp-path1 &&

This change has the same issue, I think. We prepare tmp-path1 symbolic link to trap a casual "echo rezrov >tmp-path1/file1" to be redirected to tmp1/file1 while leaving tmp-path1 as a symlink, making sure we do the equivalent of "rm tmp-path1; mkdir tmp-path1" before "echo rezrov >tmp-path1/file1".