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

From: Johannes Sixt <j6t@kdbg.org>
Date: Jun 5, 2013, 20:23 UTC
Message-ID: <51AF9E26.4050809@kdbg.org>
In-Reply-To: <7v8v2p5y7o.fsf@alter.siamese.dyndns.org>

Am 05.06.2013 00:04, schrieb Junio C Hamano:

Show 20 quoted lines

> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> @@ -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.

Show 8 quoted lines

> 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