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

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

Michael Haggerty [off-list ref] writes:

quoted

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

quoted

Michael Haggerty [off-list ref] writes:

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

quoted

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".

The components of CEILING should be adjusted to strip the symlink so
that it begins with "/run/shm/" before it is used; otherwise it
would not work.  As the current code does not do that at runtime (I
am describing the situation, not justifying), it won't match if
CEILING is built out of $TRASH_DIRECTORY.  In the above, setting of
$test and $TRASH is wrong; it does not match the realpath.

So "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," in my previous message was not correct.  The patch is
not doing that, and that is what breaks your setup.

I've confused things by not being clear what I was describing.  The
description that you quoted above was the state *before* Jiang's patch.
 Jiang's patch makes sure that $TRASH_DIRECTORY is a realpath.  The
working directory was already a realpath before his patch (due to commit
1bd9c648).  There are some other variables that are not necessarily
realpaths, even after Jiang's patch; for example (via a casual look at
test-lib.sh): $remove_trash, $HOME, $test, $TEST_DIRECTORY,
$test_results_dir, $GIT_BUILD_DIR.  I haven't checked whether these
variables are used in ways that could cause other problems.

When Jiang's patch is applied the test suite runs to completion without
any failures on my system even when I pass --root=/dev/shm (a symlink).

My preference is to set things up in such a way that most of the
tests do not have to worry about these symlink aliases.  I know you
said you do not like the "whole approach", but I'd like to see our
tests run in a stable and reproducible testing environment.

We should have specific tests (on symlink enabled platforms) that
creates a directory and an alias to it via a symlink, goes into it
and checks the CEILING (and whatever else) behaviour.  We know that
the current code does not account for the alias introduced by
symlinks, and setup_git_directory_gently() may want to be patched to
fix _that_ specific test.

Yes, stable and reproducible is good.  And explicit tests for
symlink-sensitive code would be good.  But how do we find the code that
needs explicit testing for symlink tolerance?

Therefore (in addition to specific tests) I think it would be good to
have an easy way to run the *whole* test suite in a symlink environment.
 For example, we could add a test suite option that *explicitly* makes
all tests run in a directory containing symlinks.  Or we could even do
that *all* of the time on systems that support symlinks [1]--that would
be a stable and reproducible testing environment that *does* detect many
symlink problems rather than hiding them.  (It seems unlikely the the
use of symlinks would *hide* other bugs.)  Something along the lines of

    mkdir -p "$test.dir"
    ln -s "$test.dir" "$test"

By the way, is the use of realpath(3) permissible in git code?
GIT_CEILING_DIRECTORIES handling could be fixed relatively easily by
using this function to canonicalize pathnames before comparison.

Michael

[1] This would be analogous to the inclusion of a space in "trash
directory.*", which I presume was done to detect space-handling problems
quickly.

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