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

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

On 08/29/2012 08:06 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 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.
> 
> Ouch.  I did not know have 600+ tests that cares about CEILING.

No, I'm referring to the whole test suite, which is approximately 600
files :-)  Because the patch changes TEST_DIRECTORY, it affects the
environment of all tests that use that variable (one of which being via
GIT_CEILING_DIRECTORIES).

But really I shouldn't have made it sound like this patch is so
terrible, because it is just a logical continuation of the approach begun by

    1bd9c648 t/test-lib.sh: resolve symlinks in working directory, for
pathname comparisons (2008-05-31)

It is in fact the whole approach that I object to.

>> 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.
> 
> What exactly is broken in CEILING?

CEILING is used in setup_git_directory_gently_1() when trying to find
the GIT_DIR appropriate for the current directory.  The entries in
CEILING are compared textually to getcwd(), looking for the entry that
is the longest proper prefix of PWD.  This is then used to limit a loop
that is vaguely

    while (!is_git_directory())
            chdir("..");

The problem, as I understand it, is that when the tests are run with
root set to a path that includes a symlink, then test and
TRASH_DIRECTORY are set to the textual value of "$root/trash
directory.tXXXX", but then a little bit later test-lib.sh chdirs into
the trash directory using "cd -P $test" (thereby resolving symlinks).
So, taking my concrete example "--root=/dev/shm" where /dev/shm is a
symlink to /run/shm, we have

    test="/dev/shm/trash directory.tXXXX"
    TRASH_DIRECTORY="$test"
    ...
    cd -P "$test"

which results in PWD being "/run/shm/trash directory.tXXXX".

Then (for example) in test t4035, we have stuff like

    GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
    export GIT_CEILING_DIRECTORIES &&
    cd test-outside/non/git &&
    git diff

The problem is that because PWD and TRASH_DIRECTORY *look* different,
the code in setup_git_directory_gently_1() doesn't realize that the
value of GIT_CEILING_DIRECTORIES is a parent of PWD, so it doesn't abort
the search for GIT_DIR there, and this causes the test to fail.

The underlying problem is that setup_git_directory_gently_1() isn't
smart enough to realize that the directory in GIT_CEILING_DIRECTORIES is
in fact a parent of PWD even though it textually doesn't look like one.

The patch being discussed sets TEST_DIRECTORY *after* $test has been
canonicalized (through the use of "cd -P $test"), so that TEST_DIRECTORY
and later GIT_CEILING_DIRECTORIES start with "/run/shm/" instead of
"/dev/shm/".  This change makes setup_git_directory_gently_1()'s naive
textual prefix comparison succeed.

The same problem would occur in the real world whenever the user sets
GIT_CEILING_DIRECTORIES to a value that is *in fact* a parent of PWD but
due to symbolic links *textually* does not appear to be a parent.

So the first decision is whether this should be documented as a known
limitation of GIT_CEILING_DIRECTORIES, or whether it is a bug.  My
opinion is that it is a bug.

> I somehow thought that Jiang's patch was to make sure any variables
> that contain pathnames (and make sure future paths we might grab out
> of $(pwd)) are realpath without symlinks early in the test set-up,
> and with that arrangement, no symlink gotcha should come into
> picture, with or without CEILING.

Yes, this is correct.  But what you call a "gotcha" is actually a
real-world possibility.  Git might *really* be run in a PWD that
contains symlinks, or GIT_CEILING_DIRECTORIES might contain entries that
are symlinks.  So by resolving symlinks in PWD before running tests, we
are preventing the tests from ever encountering this situation, and
therefore failing to test whether git works correctly when PWD includes
a symlink.

> Perhaps the setting of the CEILING has not been correctly converted
> with the patch?

That's not the problem.

> Or is there something fundamentally broken, even if we do not have
> any symlinks involved, with CEILING check?

I believe that:

1. The handling of GIT_CEILING_DIRECTORIES in
setup_git_directory_gently_1() (i.e., in git itself, not how it is used
in the test suite) is correct if no symlinks are involved, but breaks in
the face of symlinks.

2. The proposed patch is a mistake because it masks the brokenness of
setup_git_directory_gently_1().

3. The old commit 1bd9c648 is an even bigger mistake because it might
mask other breakages throughout git in dealing with working directories
that contain symlinks.

Michael

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