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

Johannes Sixt [off-list ref] writes:

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.

This also is not appropriate, but it is not as bad as the one in
t2003 I earlier commented on.

This test wants an untracked symbolic link in the working tree as
path1 and wants to see "ls-files -o" report it as "other".  On a
filesystem that lack symbolic link, we obviously cannot have one,
so as a substitute we just create another regular file to make the
expected output and comparison simpler. 

This one is correct; the test wants to make sure that path1 would be
checked out as a non-directory, killing any files in path1/ that is
a directory on the filesystem.

And exclusion of path1 here is a logical consequence of the previous
change, so it is good, too.

 test_expect_success 'git ls-files -k to show killed files.' '
 	date >path2 &&
-	if test_have_prereq SYMLINKS
-	then
-		ln -s frotz path3 &&
-		ln -s nitfol path5
-	else
-		date >path3 &&
-		date >path5
-	fi &&
+	test_ln_s frotz path3 &&
+	test_ln_s nitfol path5 &&

This falls into the same category as the one in t3000 above.  The
only thing that matters in this test is path3 and path5 are non
directories so that the former is killed when path3/file3 needs to
be checked out, while path5 will not appear in "ls-files -k" output.
Ideally we would want to test regular files and symlinks as two
different kinds of "non directory" filesystem entities, but on
platforms that lack symbolic links we cannot do so, hence we punt
and create a regular file.

As the point of test is to compare $tree that has symlink with
another symlink that is identical but newly created one, I think
this _does_ want the filesystem entity to be a symbolic link, but
the index has frotz as a symlink and the mode propagates to what we
read from the filesystem on !has_symlinks systems, so this
conversion may be a correct one, though it is a bit tricky.

Likewise.

Am 04.06.2013 23:55, schrieb Junio C Hamano:

Johannes Sixt [off-list ref] writes:

quoted

diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh
index 88be904..563ac7f 100755
--- a/t/t3000-ls-files-others.sh
+++ b/t/t3000-ls-files-others.sh

@@ -19,12 +19,7 @@ filesystem.
 
 test_expect_success 'setup ' '
 	date >path0 &&
-	if test_have_prereq SYMLINKS
-	then
-		ln -s xyzzy path1
-	else
-		date >path1
-	fi &&
+	test_ln_s xyzzy path1 &&
 	mkdir path2 path3 path4 &&
 	date >path2/file2 &&
 	date >path2-junk &&

This also is not appropriate, but it is not as bad as the one in
t2003 I earlier commented on.

This test wants an untracked symbolic link in the working tree as
path1 and wants to see "ls-files -o" report it as "other".  On a
filesystem that lack symbolic link, we obviously cannot have one,
so as a substitute we just create another regular file to make the
expected output and comparison simpler. 

Exactly. This is just a convenience. The issue is not introduced by the
conversion, but dates back 4 years when I added the SYMLINKS check. We
now only use a less random string on !SYMLINKS filesystems.

quoted

 test_expect_success 'git ls-files -k to show killed files.' '
 	date >path2 &&
-	if test_have_prereq SYMLINKS
-	then
-		ln -s frotz path3 &&
-		ln -s nitfol path5
-	else
-		date >path3 &&
-		date >path5
-	fi &&
+	test_ln_s frotz path3 &&
+	test_ln_s nitfol path5 &&

This falls into the same category as the one in t3000 above.  The
only thing that matters in this test is path3 and path5 are non
directories so that the former is killed when path3/file3 needs to
be checked out, while path5 will not appear in "ls-files -k" output.
Ideally we would want to test regular files and symlinks as two
different kinds of "non directory" filesystem entities, but on
platforms that lack symbolic links we cannot do so, hence we punt
and create a regular file.

Indeed. Same answer.

quoted

diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh

quoted

-test_expect_success SYMLINKS 'diff identical, but newly created symlink and file' '
+test_expect_success 'diff identical, but newly created symlink and file' '
 	>expected &&
 	rm -f frotz nitfol &&
 	echo xyzzy >nitfol &&
 	test-chmtime +10 nitfol &&
-	ln -s xyzzy frotz &&
+	test_ln_s xyzzy frotz &&
 	git diff-index -M -p $tree >current &&
 	compare_diff_patch expected current &&

As the point of test is to compare $tree that has symlink with
another symlink that is identical but newly created one, I think
this _does_ want the filesystem entity to be a symbolic link, but
the index has frotz as a symlink and the mode propagates to what we
read from the filesystem on !has_symlinks systems, so this
conversion may be a correct one, though it is a bit tricky.

Yes, this test depends on the mode propagation. I'll add a comment along
these lines and keep the change in this patch with a title marked
"trivial cases".

quoted

@@ -100,7 +103,7 @@ test_expect_success SYMLINKS 'diff different symlink and file' '
 	+yxyyz
 	EOF
 	rm -f frotz &&
-	ln -s yxyyz frotz &&
+	test_ln_s yxyyz frotz &&
 	echo yxyyz >nitfol &&
 	git diff-index -M -p $tree >current &&
 	compare_diff_patch expected current

Likewise.

-- Hannes