Re: [PATCH 07/11] t2100: use test_ln_s_add to remove SYMLINKS prerequisite

Johannes Sixt [off-list ref] writes:

This is also borderline questionable.  With path2, we are already
testing that adding a regular file (one variant of "non directory")
at a path that the index expects to see a directory (the index has
path2/file2 in it at this point) fails, and the test about path3 is
to make sure that an attempt to add a symbolic link, the other
variant of "non directory", is rejected the same way.  So it may
make more sense to skip this test when !SYMLINKS.

If we want to really test the equivalent on a filesystem without
symbolic links, it would be a more faithful test to attempt to add
it using "--add --cacheinfo" and see it fail, i.e.

   test_must_fail_to_ln_s_add xyzzy path3

which would be a copy of test_ln_s_add but has test_must_fail before
two calls to git_update_index it makes.

I think all the test_ln_s_add conversion in the series make sense,
but many uses of test_ln_s are questionable, and I suspect it would
invite similar confusion down the road.

Am 05.06.2013 00:04, schrieb Junio C Hamano:

Johannes Sixt [off-list ref] writes:

quoted

@@ -62,12 +57,7 @@ test_expect_success 'git update-index to add conflicting file path2 should fail'
 
 test_expect_success 'git update-index to add conflicting symlink path3 should fail' '
 
-	if test_have_prereq SYMLINKS
-	then
-		ln -s xyzzy path3
-	else
-		date >path3
-	fi &&
+	test_ln_s xyzzy path3 &&
 	test_must_fail git update-index --add -- path3
 '

This is also borderline questionable.  With path2, we are already
testing that adding a regular file (one variant of "non directory")
at a path that the index expects to see a directory (the index has
path2/file2 in it at this point) fails,...

You are right, of course. But again, the issue is not new with this patch.

If we want to really test the equivalent on a filesystem without
symbolic links, it would be a more faithful test to attempt to add
it using "--add --cacheinfo" and see it fail, i.e.

   test_must_fail_to_ln_s_add xyzzy path3

which would be a copy of test_ln_s_add but has test_must_fail before
two calls to git_update_index it makes.

That's not necessary, IMO. We can just add an unconditional --cache-info
test here and add SYMLINKS to the above test.

I think all the test_ln_s_add conversion in the series make sense,
but many uses of test_ln_s are questionable, and I suspect it would
invite similar confusion down the road.

Incidentally, I've been running the test_ln_s_add conversions since,
whoa!, two years now, but added test_ln_s only when I cleaned up the
patch for submission. Perhaps it's better when I rip out test_ln_s again.

-- Hannes