subject:"\[PATCH v2\] test\: set the realpath of CWD as TRASH

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

2012-08-31 Thread Michael Haggerty

On 08/30/2012 07:26 AM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:
 
 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.
 
 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?

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

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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-08-29 Thread Junio C Hamano

Michael Haggerty mhag...@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.

 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?

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..
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-08-29 Thread Michael Haggerty

On 08/29/2012 08:06 AM, Junio C Hamano wrote:
 Michael Haggerty mhag...@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.t, 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.t
TRASH_DIRECTORY=$test
...
cd -P $test

which results in PWD being /run/shm/trash directory.t.

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

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

2012-08-29 Thread Michael Haggerty

On 08/29/2012 06:33 PM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:
 
 On 08/29/2012 08:06 AM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:

 It is in fact the whole approach that I object to.
 ...
 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.t, 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.t
 TRASH_DIRECTORY=$test
 ...
 cd -P $test

 which results in PWD being /run/shm/trash directory.t.
 
 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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-08-29 Thread Junio C Hamano

Michael Haggerty mhag...@alum.mit.edu writes:

 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.

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?

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

Yeah, understood where you are coming from, and I think I can agree
with where you are trying to go.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-08-28 Thread Michael Haggerty

On 08/27/2012 06:15 PM, Junio C Hamano wrote:
 Jiang Xin worldhello@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@gmail.com
 Reported-by: Michael Haggerty mhag...@alum.mit.edu
 Signed-off-by: Jiang Xin worldhello@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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-08-27 Thread Junio C Hamano

Jiang Xin worldhello@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@gmail.com
 Reported-by: Michael Haggerty mhag...@alum.mit.edu
 Signed-off-by: Jiang Xin worldhello@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?

 diff --git a/t/test-lib.sh b/t/test-lib.sh
 index 78c42..9a59ca8 100644
 --- a/t/test-lib.sh
 +++ b/t/test-lib.sh
 @@ -531,17 +531,17 @@ fi
  test=trash directory.$(basename $0 .sh)
  test -n $root  test=$root/$test
  case $test in
 -/*) TRASH_DIRECTORY=$test ;;
 - *) TRASH_DIRECTORY=$TEST_OUTPUT_DIRECTORY/$test ;;
 +/*) ;;
 + *) test=$TEST_OUTPUT_DIRECTORY/$test ;;
  esac
 -test ! -z $debug || remove_trash=$TRASH_DIRECTORY
 +test ! -z $debug || remove_trash=$test
  rm -fr $test || {
   GIT_EXIT_OK=t
   echo 5 FATAL: Cannot prepare test area
   exit 1
  }
  
 -HOME=$TRASH_DIRECTORY
 +HOME=$test
  export HOME
  
  if test -z $TEST_NO_CREATE_REPO; then
 @@ -552,6 +552,7 @@ fi
  # Use -P to resolve symlinks in our working directory so that the cwd
  # in subprocesses like git equals our $PWD (for pathname comparisons).
  cd -P $test || exit 1
 +TRASH_DIRECTORY=$(pwd)
  
  this_test=${0##*/}
  this_test=${this_test%%-*}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-08-26 Thread Jiang Xin

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@gmail.com
Reported-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jiang Xin worldhello@gmail.com
---
 t/test-lib.sh | 9 +
 1 个文件被修改，插入 5 行(+)，删除 4 行(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 78c42..9a59ca8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -531,17 +531,17 @@ fi
 test=trash directory.$(basename $0 .sh)
 test -n $root  test=$root/$test
 case $test in
-/*) TRASH_DIRECTORY=$test ;;
- *) TRASH_DIRECTORY=$TEST_OUTPUT_DIRECTORY/$test ;;
+/*) ;;
+ *) test=$TEST_OUTPUT_DIRECTORY/$test ;;
 esac
-test ! -z $debug || remove_trash=$TRASH_DIRECTORY
+test ! -z $debug || remove_trash=$test
 rm -fr $test || {
GIT_EXIT_OK=t
echo 5 FATAL: Cannot prepare test area
exit 1
 }
 
-HOME=$TRASH_DIRECTORY
+HOME=$test
 export HOME
 
 if test -z $TEST_NO_CREATE_REPO; then
@@ -552,6 +552,7 @@ fi
 # Use -P to resolve symlinks in our working directory so that the cwd
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P $test || exit 1
+TRASH_DIRECTORY=$(pwd)
 
 this_test=${0##*/}
 this_test=${this_test%%-*}
-- 
1.7.12.92.gaa91cb5

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

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

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

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

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

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

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

8 matches

Site Navigation

Mail list logo

Footer information