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

On 08/29/2012 08:06 AM, Junio C Hamano wrote:

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.

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.

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.

That's not the problem.

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