Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY

[Michael Haggerty <mhagger@alum.mit.edu>]

On 08/27/2012 06:15 PM, Junio C Hamano wrote:
> Jiang Xin <worldhello.net@gmail.com> writes:
> 
>> Some testcases will fail if current work directory is on a symlink.
>>
>>     symlink$ sh ./t4035-diff-quiet.sh
>>     $ sh ./t4035-diff-quiet.sh --root=/symlink
>>     $ TEST_OUTPUT_DIRECTORY=/symlink sh ./t4035-diff-quiet.sh
>>
>> This is because the realpath of ".git" directory will be returned when
>> running the command 'git rev-parse --git-dir' in a subdir of the work
>> tree, and the realpath may not equal to "$TRASH_DIRECTORY".
>>
>> In this fix, "$TRASH_DIRECTORY" is determined right after the realpath
>> of CWD is resolved.
>>
>> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
>> Reported-by: Michael Haggerty <mhagger@alum.mit.edu>
>> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
>> ---
> 
> I think this is in line with what was discussed in the other thread
> Michael brought this up.  Thanks for following it through.
> 
> Michael, this looks good to me; anything I missed?

I can confirm that this patch eliminates the test failures that I was
seeing in t4035 and t9903.

But it also changes almost 600 *other* tests from "succeed even in the
presence of symlinks" to "never tested in the presence of symlinks", and
I think that is surrendering more ground than necessary.  I would rather
see one of the following approaches:

*If* the official policy is that GIT_CEILING_DIRECTORIES is not reliable
in the presence of symlinks, then (a) that limitation should be
mentioned in the documentation; (b) the affected tests should either be
skipped in the case of symlinked directories or they (alone!) should
take measures to work around the problem.

*If* the official policy is that GIT_CEILING_DIRECTORIES should work in
the presence of symlinks, then (a) we should add a failing test case
that specifically documents this bug; (b) other tests that fail as a
side effect of this bug even though they are trying to test something
else should either be skipped in the case of symlinked directories or
they (alone!) should take measures to work around the problem; (c) the
bug should be fixed as soon as possible.

In fact, we could even go further:

* The "cd -P" in test-lib.sh could be changed to "cd -L", to avoid
masking other problems related to symlinks.  If I make that change, I
get test failures in 10 files when using --root=/symlinkeddir, and not
all of them are obviously related to GIT_CEILING_DIRECTORIES.  Some of
these might simply be sloppiness in how the test scripts are written,
but others might be bugs in git proper.

* We could *intentionally* access the trash directories via a symlink on
systems that support symlinks (much like the trash directory names
intentionally include a space) to get *systematic* test coverage of
symlink issues, rather than occasional testing and mysterious failures
when somebody happens to have a symlink in his build path or root.

(But it is still the case that I don't have time to work on this.)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/