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.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. > 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 throughout git in dealing with working directories that contain symlinks. 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