Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY
On 08/30/2012 07:26 AM, Junio C Hamano wrote: > Michael Haggerty 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
Michael Haggerty 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
On 08/29/2012 06:33 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> On 08/29/2012 08:06 AM, Junio C Hamano wrote: >>> Michael Haggerty 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
Michael Haggerty writes: > On 08/29/2012 08:06 AM, Junio C Hamano wrote: >> Michael Haggerty 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. 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. -- 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
On 08/29/2012 08:06 AM, Junio C Hamano wrote: > Michael Haggerty 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 big
Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY
Michael Haggerty 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
On 08/27/2012 06:15 PM, Junio C Hamano wrote: > Jiang Xin 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 >> Reported-by: Michael Haggerty >> Signed-off-by: Jiang Xin >> --- > > 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
Jiang Xin 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 > Reported-by: Michael Haggerty > Signed-off-by: Jiang Xin > --- 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
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 Reported-by: Michael Haggerty Signed-off-by: Jiang Xin --- 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