patchtest: some testcases failed if cwd is on a symlink

Run command 'git rev-parse --git-dir' under subdir will return realpath of '.git' directory. Some test scripts compare this realpath against "$TRASH_DIRECTORY", they are not equal if current working directory is on a symlink.

In this fix, get realpath of "$TRASH_DIRECTORY", store it in "$TRASH_REALPATH" variable, and use it when necessary.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 t/t4035-diff-quiet.sh  |  8 +++++---
 t/t9903-bash-prompt.sh | 13 +++++++------
 2 个文件被修改，插入 12 行(+)，删除 9 行(-)

Some grammatical nits about the commit message. I hope this doesn't come across as too picky/annoying ... And you might want to wait for a native to confirm whether these nits are actually all warranted.

On 07/24/2012 10:00 AM, Jiang Xin wrote:

s/Run/Running/

s/under subdir/under a subdir/. Or even better IMHO, s/under subdir/in a subdir/

s/realpath/the realpath/

s/of/of the/

s/they are not/but they are not/

s/current/the current/

s/realpath/the realpath/

BTW, the outer quotes are not needed; this is enough:

    TRASH_REALPATH=$(cd "$TRASH_DIRECTORY"; pwd -P)

Regards,
  Stefano

worldhello.net@gmail.com wrote on Tue, 24 Jul 2012 16:00 +0800:

We have the same problem with p4. I fixed it in t98* in 23bd0c9 (git p4 test: use real_path to resolve p4 client symlinks, 2012-06-27), but maybe an exported TRASH_DIRECTORY_REAL_PATH might be generally useful.

There's a portable test helper that does this already:

    path=$(test-path-utils real_path "$path")

		-- Pete

Jiang Xin <worldhello.net@gmail.com> writes:

I wonder if running test in a real directory (in other words, "fix" your cwd) may be a simpler, more robust and generally a better solution, e.g. something silly like...

2012/7/24 Junio C Hamano <gitster@pobox.com>:

Yes, it's much simpler.

Junio C Hamano <gitster <at> pobox.com> writes:

What is the status of this bug? Today I wasted a bunch of time trying to track down a build breakage that was ultimately caused by this problem. I was running the test suite on master with "--root=/dev/shm" (my usual setting), but this caused tests t4035 and t9903 to fail as described upthread. (It turns out that on my system, /dev/shm is a symlink to /run/shm.)

For me, the failure is fixed by Jiang Xin's patch, but it is not fixed by Junio's. In the case of t4035 in failing test "git diff --ignore-all-space, both files outside repo", right before "git diff" is called,

PWD=/run/shm/trash directory.t4035-diff-quiet/test-outside/non/git GIT_CEILING_DIRECTORIES=/dev/shm/trash directory.t4035-diff-quiet/test-outside

I can work around the problem by using "--root=/run/shm". But it would be good to get this problem fixed one way or the other to spare other people the same pain.

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

I do not necessarily think it is a work around.

A low-impact approach may be to update the part that parses --root option to do

	root=$(...)
        root=$( cd "$root" && /bin/pwd )

or something.

On 08/18/2012 10:36 PM, Junio C Hamano wrote:

http://en.wiktionary.org/wiki/workaround:
2. (computing) A procedure or a temporary fix that bypasses a problem
   and allows the user to continue working until a better solution
   can be provided; a kluge.

For me that is exactly what it was.

I just verified that the combination of your two suggestions (i.e., the patch below) fixes the problem for me.

Nevertheless, I'm not sure that this is the best solution. The test failures that occur without this change suggest to me that GIT_CEILING_DIRECTORIES is implemented in a fragile way.

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

Good to know.

The only remaining two worries from me are if everybody has working pwd at that early point in the script (I think MINGW replaces pwd with its own), and if the latter one should really be "/bin/pwd" everywhere. Saying "Give the true path to --root when you run it" can sidestep the latter issue ;-)

Hrmph.  How would you improve it?  chdir() around twice and compare?

On 08/19/2012 06:43 PM, Junio C Hamano wrote:

I believe that the old-school method is to stat(2) the two directories and check whether their device IDs and inode numbers are identical. But I don't know whether that is portable to other allegedly POSIX-compatible OSs, or even works with all modern filesystems (I think there was just a thread about a FUSE filesystem that sometimes changes inode numbers).

Another alternative is to write a function that knows how to convert an arbitrary path into an absolute path, including converting relative paths to absolute, resolving symlinks, eliminating redundant "/" and ".", resolving "..", and perhaps canonicalizing "/" vs. "\" and who-knows-what-else on Windows. It takes a bit of care to implement this correctly, but it might be a useful function to have around. Python's library function os.path.realpath() is an example [1].

Either approach would avoid chdir()ing around even temporarily, which would anyway be bad form in git proper (as opposed to the test suite). And it would avoid the need to chdir() permanently in the test suite, which can have the effect of making directory names appear in unfamiliar forms.

I'm afraid I don't have time to work on this now; I'm still trying to finish the next iteration of the post-receive-email hook script replacement.

Michael

[1] (Python) source code here:

    http://hg.python.org/cpython/file/20985f52b65e/Lib/posixfile.py

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>
---
 t/test-lib.sh | 9 +++++----
 1 个文件被修改，插入 5 行(+)，删除 4 行(-)

Jiang Xin <worldhello.net@gmail.com> writes:

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?

On 08/27/2012 06:15 PM, Junio C Hamano wrote:

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> writes:

Ouch.  I did not know have 600+ tests that cares about CEILING.

What exactly is broken in CEILING?

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.

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

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

Puzzled..

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

Michael Haggerty <mhagger@alum.mit.edu> writes:

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.

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.

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

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

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> writes:

As long as we can add a compat/realpath.c (perhaps lift one from glibc before they went GPLv3) for platforms that matter, I do not see it as a huge problem. How close is abspath.c::real_path() to what you need?

Yeah, understood where you are coming from, and I think I can agree with where you are trying to go.

On 08/30/2012 07:26 AM, Junio C Hamano wrote:

Cool, I didn't know about that function. It's approximately what is needed, except that it dies if fed an invalid path (unacceptable when processing GIT_CEILING_DIRECTORIES) and it's buggy (try "test-path-utils real_path ''" or "test-path-utils real_path '/foo'"). However, I'm working on fixing it and splitting off a variant that returns NULL on errors.

Michael

This series fixes longest_ancestor_length() so that it works even if prefix_list contains entries that involve symlinks. The basic goal of the series is to call real_path() on each of the entries so that a textual comparison of the potential prefix to the front of path correctly decides whether the path is located inside of the entry. But along the way some other things had to be changed:

* real_path() die()s if the path passed to it is invalid, whereas it
  is allowed for GIT_CEILING_DIRECTORIES to contain invalid paths.  So
  create a new function real_path_if_valid() that returns NULL for
  invalid paths.

* Changing longest_ancestor_length() to call real_path_if_valid()
  would make the former very difficult to test (because the tests
  would depend on the contents of the whole filesystem).  Therefore,
  rewrite longest_ancestor_length() in terms of functions
  string_list_split(), string_list_longest_prefix(), and
  real_path_if_valid() which are tested individually.

The net results of these changes are that:

1. t1504 used to have to canonicalize TRASH_DIRECTORY to make itself
   work even if the --root directory contains symlinks.  This
   canonicalization is no longer necessary (and has been removed).

2. t4035, which used to fail if the --root directory contained
   symlinks, now works correctly in that situation.

After this change, all tests pass if the --root directory does *not* contain symlinks, but t9903 still fails if the --root directory contains symlinks. I haven't analyzed the cause of t9903's failure, but it does not appear to be related to the GIT_CEILING_DIRECTORIES feature.

On the mailing list I suggested *purposely* inserting symlinks into the "trash directory.*" paths to test symlink handling more systematically. This patch series does *NOT* make that change.

Michael Haggerty (8):
  Introduce new static function real_path_internal()
  Introduce new function real_path_if_valid()
  longest_ancestor_length(): use string_list_split()
  longest_ancestor_length(): explicitly filter list before loop
  longest_ancestor_length(): always add a slash to the end of prefixes
  longest_ancestor_length(): use string_list_longest_prefix()
  longest_ancestor_length(): resolve symlinks before comparing paths
  t1504: stop resolving symlinks in GIT_CEILING_DIRECTORIES

 abspath.c               | 98 ++++++++++++++++++++++++++++++++++++++-----------
 cache.h                 |  1 +
 path.c                  | 54 ++++++++++++++++-----------
 t/t0060-path-utils.sh   | 64 --------------------------------
 t/t1504-ceiling-dirs.sh | 67 +++++++++++++++++----------------
 5 files changed, 144 insertions(+), 140 deletions(-)

It accepts a new parameter, die_on_error. If die_on_error is false, it simply cleans up after itself and returns NULL rather than dying.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 abspath.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 72 insertions(+), 21 deletions(-)

Michael Haggerty <mhagger@alum.mit.edu> writes:

The patch makes sense, but while you are touching this code, I have to wonder if there is an easy way to tell, before entering the loop, if we have to chdir() around in the loop. That would allow us to hoist the getcwd() that is done only so that we can come back to where we started outside the loop, making it clear why the call is there, instead of cryptic "if (!*cwd &&" to ensure we do getcwd() once and before doing any chdir().

On 09/27/2012 11:27 PM, Junio C Hamano wrote:

I don't see an easy way to predict, before entering the loop, whether chdir() will be needed. For example, compare

    touch filename
    ln -s filename foo
    ./test-path-utils real_path foo

with

    touch filename
    ln -s $(pwd)/filename foo
    ./test-path-utils real_path foo

In the first case no chdir() is needed, whereas in the second case a chdir() is needed but only on the second loop iteration.

All I can offer you is a palliative like the one below.

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

Thanks for an example, and it is perfectly OK if we really have to have that "only fires once and only if needed" logic in the loop.

I think that is an improvement; or we could rename it not cwd[] but some other name that includes the word "original" in it.

Thanks.

The function is like real_path(), except that it returns NULL on error instead of dying.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 abspath.c | 5 +++++
 cache.h   | 1 +
 2 files changed, 6 insertions(+)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 path.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Michael Haggerty <mhagger@alum.mit.edu> writes:

Much nicer than the yucky original ;-)

Why do you need this memcpy in the first place? Isn't ceil already a NUL terminated string unlike the original code that points into a part of the prefix_list string? IOW, why not

	normalize_path_copy(buf, ceil);

or something?

Can normalize_path_copy() overflow buf[PATH_MAX+1] here (before or after this patch)?

On 09/28/2012 12:48 AM, Junio C Hamano wrote:

If your winky-smiley implies irony, then I would like to object. Even though the original is not difficult to understand, it is more difficult to review than the new version because one has to think about off-by-one errors etc. The new version has a bit more boilerplate, but a quick read suffices both to understand it and to see that it is correct. Though of course I admit that the improvement is small.

But the main point of this change is to move towards using more testable parts, so it is a step forward regardless of whether it is more readable.

Good point.  I will fix this in v2.

normalize_path_copy() can only shrink paths, not grow them. So the length check on ceil guarantees that the result of normalize_path_copy() will fit in buf.

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

No irony. The original was hard to read, especially with the for loop that does in-line strchr() on a single line. The updated one is much easier to read.

OK.

Thanks.

Separate the step of filtering and normalizing elements of the prefixes list from the iteration that looks for the longest prefix. This will help keep the function testable after we not only normalize the paths, but also convert them into real paths.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 path.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

Makes sense.

Previously we stripped off any slashes that were present. Instead, add a slash if it is missing. This removes the need for an extra check that path has a slash following the prefix and makes the handling of the root directory more natural, making the way clear to use string_list_longest_prefix().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 path.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Use string_list_longest_prefix() in the implementation of longest_ancestor_length(), instead of an equivalent loop.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 path.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

longest_ancestor_length() relies on a textual comparison of directory parts to find the part of path that overlaps with one of the paths in prefix_list. But this doesn't work if any of the prefixes involves a symbolic link, because the directories will look different even though they might logically refer to the same directory. So canonicalize the paths listed in prefix_list using real_path_if_valid() before trying to find matches.

path is already in canonical form, so doesn't need to be canonicalized again.

This fixes some problems with using GIT_CEILING_DIRECTORIES that contains paths involving symlinks, including t4035 if run with --root set to a path involving symlinks.

Remove a number of tests of longest_ancestor_length(). It is awkward to test longest_ancestor_length() now, because its new path normalization behavior depends on the contents of the whole filesystem. But we can live without the tests, because longest_ancestor_length() is now built of reusable components that are themselves tested separately: string_list_split(), string_list_longest_prefix(), and real_path_if_valid().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 path.c                | 17 ++++++++------
 t/t0060-path-utils.sh | 64 ---------------------------------------------------
 2 files changed, 10 insertions(+), 71 deletions(-)

Michael Haggerty <mhagger@alum.mit.edu> writes:

Errr, components may be correct but the way to combine and construct could go faulty, so...

Nice.

On 09/28/2012 12:51 AM, Junio C Hamano wrote:

I don't see a realistic alternative. Testing real_path() is itself is already quite awkward (see t0060), so testing longest_ancestor_length() would be even more so. Of course, the GIT_CEILING_DIRECTORIES tests indirectly test longest_ancestor_length(), though not systematically.

If you have a better suggestion, please let me know.

I just noticed that the second "len++" in the final "if" is misleading.
 I will fix that in v2.

Michael

This test used to explicitly resolve symlinks in the paths derived from TRASH_DIRECTORY that were written to GIT_CEILING_DIRECTORIES, because the code handling GIT_CEILING_DIRECTORIES was confused by symlinks. This is no longer necessary.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1504-ceiling-dirs.sh | 67 ++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

Michael Haggerty <mhagger@alum.mit.edu> writes:

I haven't read the actual patches yet, but the all of the above sounds sensible.

And that may be a sensible follow-up step once the dust settles.

Thanks.