{"thread":{"id":"31082","subject":"[PATCH] test: some testcases failed if cwd is on a symlink","startedAt":"2012-07-24T08:00:01Z","lastAt":"2012-09-29T05:46:30Z","messageCount":38,"participants":["Jiang Xin","Stefano Lattarini","Pete Wyckoff","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"195611","messageId":"001c60538df409d94618e80cc3faaae586ccf053.1343116581.git.worldhello.net@gmail.com","threadId":"31082","inReplyTo":null,"subject":"[PATCH] test: some testcases failed if cwd is on a symlink","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2012-07-24T08:00:01Z","receivedAt":"2012-07-24T08:00:01Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"Run command 'git rev-parse --git-dir' under subdir will return realpath\nof '.git' directory. Some test scripts compare this realpath against\n\"$TRASH_DIRECTORY\", they are not equal if current working directory is\non a symlink.\n\nIn this fix, get realpath of \"$TRASH_DIRECTORY\", store it in\n\"$TRASH_REALPATH\" variable, and use it when necessary.\n\nSigned-off-by: Jiang Xin \n---\n t/t4035-diff-quiet.sh | 8 +++++---\n t/t9903-bash-prompt.sh | 13 +++++++------\n 2 个文件被修改，插入 12 行(+)，删除 9 行(-)\n\ndiff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh\nindex 23141..5855 100755\n--- a/t/t4035-diff-quiet.sh\n+++ b/t/t4035-diff-quiet.sh\n@@ -4,6 +4,8 @@ test_description='Return value of diffs'\n \n . ./test-lib.sh\n \n+TRASH_REALPATH=\"$(cd \"$TRASH_DIRECTORY\"; pwd -P)\"\n+\n test_expect_success 'setup' '\n \techo 1 >a &&\n \tgit add . &&\n@@ -102,7 +104,7 @@ test_expect_success 'git diff, one file outside repo' '\n \n test_expect_success 'git diff, both files outside repo' '\n \t(\n-\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_REALPATH/test-outside\" &&\n \t\texport GIT_CEILING_DIRECTORIES &&\n \t\tcd test-outside/non/git &&\n \t\ttest_expect_code 0 git diff --quiet a matching-file &&\n@@ -120,7 +122,7 @@ test_expect_success 'git diff --ignore-space-at-eol, one file outside repo' '\n \n test_expect_success 'git diff --ignore-space-at-eol, both files outside repo' '\n \t(\n-\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_REALPATH/test-outside\" &&\n \t\texport GIT_CEILING_DIRECTORIES &&\n \t\tcd test-outside/non/git &&\n \t\ttest_expect_code 0 git diff --quiet --ignore-space-at-eol a trailing-space &&\n@@ -139,7 +141,7 @@ test_expect_success 'git diff --ignore-all-space, one file outside repo' '\n \n test_expect_success 'git diff --ignore-all-space, both files outside repo' '\n \t(\n-\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_REALPATH/test-outside\" &&\n \t\texport GIT_CEILING_DIRECTORIES &&\n \t\tcd test-outside/non/git &&\n \t\ttest_expect_code 0 git diff --quiet --ignore-all-space a trailing-space &&\ndiff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\nindex f17c1f..275bc 100755\n--- a/t/t9903-bash-prompt.sh\n+++ b/t/t9903-bash-prompt.sh\n@@ -10,6 +10,7 @@ test_description='test git-specific bash prompt functions'\n . \"$GIT_BUILD_DIR/contrib/completion/git-prompt.sh\"\n \n actual=\"$TRASH_DIRECTORY/actual\"\n+TRASH_REALPATH=\"$(cd \"$TRASH_DIRECTORY\"; pwd -P)\"\n \n test_expect_success 'setup for prompt tests' '\n \tmkdir -p subdir/subsubdir &&\n@@ -59,7 +60,7 @@ test_expect_success 'gitdir - .git directory in cwd' '\n '\n \n test_expect_success 'gitdir - .git directory in parent' '\n-\techo \"$TRASH_DIRECTORY/.git\" > expected &&\n+\techo \"$TRASH_REALPATH/.git\" > expected &&\n \t(\n \t\tcd subdir/subsubdir &&\n \t\t__gitdir > \"$actual\"\n@@ -77,7 +78,7 @@ test_expect_success 'gitdir - cwd is a .git directory' '\n '\n \n test_expect_success 'gitdir - parent is a .git directory' '\n-\techo \"$TRASH_DIRECTORY/.git\" > expected &&\n+\techo \"$TRASH_REALPATH/.git\" > expected &&\n \t(\n \t\tcd .git/refs/heads &&\n \t\t__gitdir > \"$actual\"\n@@ -115,7 +116,7 @@ test_expect_success 'gitdir - non-existing $GIT_DIR' '\n '\n \n test_expect_success 'gitdir - gitfile in cwd' '\n-\techo \"$TRASH_DIRECTORY/otherrepo/.git\" > expected &&\n+\techo \"$TRASH_REALPATH/otherrepo/.git\" > expected &&\n \techo \"gitdir: $TRASH_DIRECTORY/otherrepo/.git\" > subdir/.git &&\n \ttest_when_finished \"rm -f subdir/.git\" &&\n \t(\n@@ -126,7 +127,7 @@ test_expect_success 'gitdir - gitfile in cwd' '\n '\n \n test_expect_success 'gitdir - gitfile in parent' '\n-\techo \"$TRASH_DIRECTORY/otherrepo/.git\" > expected &&\n+\techo \"$TRASH_REALPATH/otherrepo/.git\" > expected &&\n \techo \"gitdir: $TRASH_DIRECTORY/otherrepo/.git\" > subdir/.git &&\n \ttest_when_finished \"rm -f subdir/.git\" &&\n \t(\n@@ -137,7 +138,7 @@ test_expect_success 'gitdir - gitfile in parent' '\n '\n \n test_expect_success SYMLINKS 'gitdir - resulting path avoids symlinks' '\n-\techo \"$TRASH_DIRECTORY/otherrepo/.git\" > expected &&\n+\techo \"$TRASH_REALPATH/otherrepo/.git\" > expected &&\n \tmkdir otherrepo/dir &&\n \ttest_when_finished \"rm -rf otherrepo/dir\" &&\n \tln -s otherrepo/dir link &&\n@@ -152,7 +153,7 @@ test_expect_success SYMLINKS 'gitdir - resulting path avoids symlinks' '\n test_expect_success 'gitdir - not a git repository' '\n \t(\n \t\tcd subdir/subsubdir &&\n-\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY\" &&\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_REALPATH\" &&\n \t\texport GIT_CEILING_DIRECTORIES &&\n \t\ttest_must_fail __gitdir\n \t)\n-- \n1.7.12.rc0.16.gf4916ac\n"},{"id":"195612","messageId":"500E5BD8.5050404@gmail.com","threadId":"31082","inReplyTo":"001c60538df409d94618e80cc3faaae586ccf053.1343116581.git.worldhello.net@gmail.com","subject":"Re: [PATCH] test: some testcases failed if cwd is on a symlink","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2012-07-24T08:24:56Z","receivedAt":"2012-07-24T08:24:56Z","isPatch":true,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"Some grammatical nits about the commit message. I hope this doesn't\ncome across as too picky/annoying ... And you might want to wait for\na native to confirm whether these nits are actually all warranted.\n\nOn 07/24/2012 10:00 AM, Jiang Xin wrote:\n> Run\n>\ns/Run/Running/\n\n> command 'git rev-parse --git-dir' under subdir\n>\ns/under subdir/under a subdir/. Or even better IMHO,\ns/under subdir/in a subdir/\n\n> will return realpath\n>\ns/realpath/the realpath/\n\n> of '.git' directory.\n>\ns/of/of the/\n\n> Some test scripts compare this realpath against\n> \"$TRASH_DIRECTORY\", they are not equal\n>\ns/they are not/but they are not/\n\n> if current working directory is on a symlink.\n>\ns/current/the current/\n\n> In this fix, get realpath\n>\ns/realpath/the realpath/\n\n> of \"$TRASH_DIRECTORY\", store it in\n> \"$TRASH_REALPATH\" variable, and use it when necessary.\n> \n> Signed-off-by: Jiang Xin \n> ---\n> t/t4035-diff-quiet.sh | 8 +++++---\n> t/t9903-bash-prompt.sh | 13 +++++++------\n> 2 个文件被修改，插入 12 行(+)，删除 9 行(-)\n> \n> diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh\n> index 23141..5855 100755\n> --- a/t/t4035-diff-quiet.sh\n> +++ b/t/t4035-diff-quiet.sh\n> @@ -4,6 +4,8 @@ test_description='Return value of diffs'\n> \n> . ./test-lib.sh\n> \n> +TRASH_REALPATH=\"$(cd \"$TRASH_DIRECTORY\"; pwd -P)\"\n> +\nBTW, the outer quotes are not needed; this is enough:\n\n TRASH_REALPATH=$(cd \"$TRASH_DIRECTORY\"; pwd -P)\n\nRegards,\n Stefano\n"},{"id":"195617","messageId":"20120724105910.GA5478@padd.com","threadId":"31082","inReplyTo":"001c60538df409d94618e80cc3faaae586ccf053.1343116581.git.worldhello.net@gmail.com","subject":"Re: [PATCH] test: some testcases failed if cwd is on a symlink","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-07-24T10:59:10Z","receivedAt":"2012-07-24T10:59:10Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"worldhello.net@gmail.com wrote on Tue, 24 Jul 2012 16:00 +0800:\n> Run command 'git rev-parse --git-dir' under subdir will return realpath\n> of '.git' directory. Some test scripts compare this realpath against\n> \"$TRASH_DIRECTORY\", they are not equal if current working directory is\n> on a symlink.\n> \n> In this fix, get realpath of \"$TRASH_DIRECTORY\", store it in\n> \"$TRASH_REALPATH\" variable, and use it when necessary.\n\nWe have the same problem with p4. I fixed it in t98* in\n23bd0c9 (git p4 test: use real_path to resolve p4 client\nsymlinks, 2012-06-27), but maybe an exported\nTRASH_DIRECTORY_REAL_PATH might be generally useful.\n\n> +TRASH_REALPATH=\"$(cd \"$TRASH_DIRECTORY\"; pwd -P)\"\n\nThere's a portable test helper that does this already:\n\n path=$(test-path-utils real_path \"$path\")\n\n\t\t-- Pete\n"},{"id":"195633","messageId":"7vboj5gqqo.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"001c60538df409d94618e80cc3faaae586ccf053.1343116581.git.worldhello.net@gmail.com","subject":"Re: [PATCH] test: some testcases failed if cwd is on a symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-24T14:47:59Z","receivedAt":"2012-07-24T14:47:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> Run command 'git rev-parse --git-dir' under subdir will return realpath\n> of '.git' directory. Some test scripts compare this realpath against\n> \"$TRASH_DIRECTORY\", they are not equal if current working directory is\n> on a symlink.\n>\n> In this fix, get realpath of \"$TRASH_DIRECTORY\", store it in\n> \"$TRASH_REALPATH\" variable, and use it when necessary.\n\nI wonder if running test in a real directory (in other words, \"fix\"\nyour cwd) may be a simpler, more robust and generally a better\nsolution, e.g. something silly like...\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex acda33d..7f6fb0a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -15,6 +15,8 @@\n # You should have received a copy of the GNU General Public License\n # along with this program. If not, see http://www.gnu.org/licenses/ .\n \n+cd \"$(pwd -P)\"\n+\n # if --tee was passed, write the output not only to the terminal, but\n # additionally to the file test-results/$BASENAME.out, too.\n case \"$GIT_TEST_TEE_STARTED, $* \" in\n"},{"id":"195667","messageId":"CANYiYbG6uTOUmPr7YyHc7VftDwSVOj0_gWtH_AKDiyxRMCahQg@mail.gmail.com","threadId":"31082","inReplyTo":"7vboj5gqqo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] test: some testcases failed if cwd is on a symlink","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2012-07-24T22:06:04Z","receivedAt":"2012-07-24T22:06:04Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"2012/7/24 Junio C Hamano :\n> I wonder if running test in a real directory (in other words, \"fix\"\n> your cwd) may be a simpler, more robust and generally a better\n> solution, e.g. something silly like...\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index acda33d..7f6fb0a 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -15,6 +15,8 @@\n> # You should have received a copy of the GNU General Public License\n> # along with this program. If not, see http://www.gnu.org/licenses/ .\n>\n> +cd \"$(pwd -P)\"\n> +\n\nYes, it's much simpler.\n\n-- \nJiang Xin\n"},{"id":"197240","messageId":"loom.20120818T162226-852@post.gmane.org","threadId":"31082","inReplyTo":"7vboj5gqqo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] test: some testcases failed if cwd is on a symlink","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-18T14:41:11Z","receivedAt":"2012-08-18T14:41:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Junio C Hamano pobox.com> writes:\n\n> \n> Jiang Xin gmail.com> writes:\n> \n> > Run command 'git rev-parse --git-dir' under subdir will return realpath\n> > of '.git' directory. Some test scripts compare this realpath against\n> > \"$TRASH_DIRECTORY\", they are not equal if current working directory is\n> > on a symlink.\n> >\n> > In this fix, get realpath of \"$TRASH_DIRECTORY\", store it in\n> > \"$TRASH_REALPATH\" variable, and use it when necessary.\n> \n> I wonder if running test in a real directory (in other words, \"fix\"\n> your cwd) may be a simpler, more robust and generally a better\n> solution, e.g. something silly like...\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index acda33d..7f6fb0a 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -15,6 +15,8 @@\n> # You should have received a copy of the GNU General Public License\n> # along with this program. If not, see http://www.gnu.org/licenses/ .\n> \n> +cd \"$(pwd -P)\"\n> +\n> # if --tee was passed, write the output not only to the terminal, but\n> # additionally to the file test-results/$BASENAME.out, too.\n> case \"$GIT_TEST_TEE_STARTED, $* \" in\n\nWhat is the status of this bug? Today I wasted a bunch of time trying to track \ndown a build breakage that was ultimately caused by this problem. I was running \nthe test suite on master with \"--root=/dev/shm\" (my usual setting), but this \ncaused tests t4035 and t9903 to fail as described upthread. (It turns out that \non my system, /dev/shm is a symlink to /run/shm.)\n\nFor me, the failure is fixed by Jiang Xin's patch, but it is not fixed by \nJunio's. In the case of t4035 in failing test \"git diff --ignore-all-space, \nboth files outside repo\", right before \"git diff\" is called,\n\nPWD=/run/shm/trash directory.t4035-diff-quiet/test-outside/non/git\nGIT_CEILING_DIRECTORIES=/dev/shm/trash directory.t4035-diff-quiet/test-outside\n\nI can work around the problem by using \"--root=/run/shm\". But it would be good \nto get this problem fixed one way or the other to spare other people the same \npain.\n\nMichael\n"},{"id":"197261","messageId":"7vfw7krl8r.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"loom.20120818T162226-852@post.gmane.org","subject":"Re: [PATCH] test: some testcases failed if cwd is on a symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-18T20:36:04Z","receivedAt":"2012-08-18T20:36:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I can work around the problem by using \"--root=/run/shm\". \n\nI do not necessarily think it is a work around.\n\nA low-impact approach may be to update the part that parses --root\noption to do\n\n\troot=$(...)\n root=$( cd \"$root\" && /bin/pwd )\n\nor something.\n"},{"id":"197285","messageId":"5030F0BF.2090500@alum.mit.edu","threadId":"31082","inReplyTo":"7vfw7krl8r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] test: some testcases failed if cwd is on a symlink","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-19T13:57:19Z","receivedAt":"2012-08-19T13:57:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/18/2012 10:36 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> I can work around the problem by using \"--root=/run/shm\". \n> \n> I do not necessarily think it is a work around.\n\nhttp://en.wiktionary.org/wiki/workaround:\n2. (computing) A procedure or a temporary fix that bypasses a problem\n and allows the user to continue working until a better solution\n can be provided; a kluge.\n\nFor me that is exactly what it was.\n\n> A low-impact approach may be to update the part that parses --root\n> option to do\n> \n> \troot=$(...)\n> root=$( cd \"$root\" && /bin/pwd )\n> \n> or something.\n\nI just verified that the combination of your two suggestions (i.e., the\npatch below) fixes the problem for me.\n\nNevertheless, I'm not sure that this is the best solution. The test\nfailures that occur without this change suggest to me that\nGIT_CEILING_DIRECTORIES is implemented in a fragile way.\n\nMichael\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex bb4f886..c7f320f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -15,6 +15,8 @@\n # You should have received a copy of the GNU General Public License\n # along with this program. If not, see http://www.gnu.org/licenses/ .\n\n+cd \"$(pwd -P)\"\n+\n # if --tee was passed, write the output not only to the terminal, but\n # additionally to the file test-results/$BASENAME.out, too.\n case \"$GIT_TEST_TEE_STARTED, $* \" in\n@@ -166,6 +168,7 @@ do\n shift ;; # was handled already\n --root=*)\n root=$(expr \"z$1\" : 'z[^=]*=\\(.*\\)')\n+ root=$(cd \"$root\" && /bin/pwd)\n shift ;;\n *)\n echo \"error: unknown test option '$1'\" >&2; exit 1 ;;\n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"197288","messageId":"7vharyq1c6.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"5030F0BF.2090500@alum.mit.edu","subject":"Re: [PATCH] test: some testcases failed if cwd is on a symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-19T16:43:37Z","receivedAt":"2012-08-19T16:43:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I just verified that the combination of your two suggestions (i.e., the\n> patch below) fixes the problem for me.\n\nGood to know.\n\nThe only remaining two worries from me are if everybody has working\npwd at that early point in the script (I think MINGW replaces pwd\nwith its own), and if the latter one should really be \"/bin/pwd\"\neverywhere. Saying \"Give the true path to --root when you run it\"\ncan sidestep the latter issue ;-)\n\n> Nevertheless, I'm not sure that this is the best solution. The test\n> failures that occur without this change suggest to me that\n> GIT_CEILING_DIRECTORIES is implemented in a fragile way.\n\nHrmph. How would you improve it? chdir() around twice and compare?\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index bb4f886..c7f320f 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -15,6 +15,8 @@\n> # You should have received a copy of the GNU General Public License\n> # along with this program. If not, see http://www.gnu.org/licenses/ .\n>\n> +cd \"$(pwd -P)\"\n> +\n> # if --tee was passed, write the output not only to the terminal, but\n> # additionally to the file test-results/$BASENAME.out, too.\n> case \"$GIT_TEST_TEE_STARTED, $* \" in\n> @@ -166,6 +168,7 @@ do\n> shift ;; # was handled already\n> --root=*)\n> root=$(expr \"z$1\" : 'z[^=]*=\\(.*\\)')\n> + root=$(cd \"$root\" && /bin/pwd)\n> shift ;;\n> *)\n> echo \"error: unknown test option '$1'\" >&2; exit 1 ;;\n"},{"id":"197503","messageId":"503323C5.9050500@alum.mit.edu","threadId":"31082","inReplyTo":"7vharyq1c6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] test: some testcases failed if cwd is on a symlink","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-21T05:59:33Z","receivedAt":"2012-08-21T05:59:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/19/2012 06:43 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> I just verified that the combination of your two suggestions (i.e., the\n>> patch below) fixes the problem for me.\n> \n> Good to know.\n> \n> The only remaining two worries from me are if everybody has working\n> pwd at that early point in the script (I think MINGW replaces pwd\n> with its own), and if the latter one should really be \"/bin/pwd\"\n> everywhere. Saying \"Give the true path to --root when you run it\"\n> can sidestep the latter issue ;-)\n> \n>> Nevertheless, I'm not sure that this is the best solution. The test\n>> failures that occur without this change suggest to me that\n>> GIT_CEILING_DIRECTORIES is implemented in a fragile way.\n> \n> Hrmph. How would you improve it? chdir() around twice and compare?\n\nI believe that the old-school method is to stat(2) the two directories\nand check whether their device IDs and inode numbers are identical. But\nI don't know whether that is portable to other allegedly\nPOSIX-compatible OSs, or even works with all modern filesystems (I think\nthere was just a thread about a FUSE filesystem that sometimes changes\ninode numbers).\n\nAnother alternative is to write a function that knows how to convert an\narbitrary path into an absolute path, including converting relative\npaths to absolute, resolving symlinks, eliminating redundant \"/\" and\n\".\", resolving \"..\", and perhaps canonicalizing \"/\" vs. \"\\\" and\nwho-knows-what-else on Windows. It takes a bit of care to implement\nthis correctly, but it might be a useful function to have around.\nPython's library function os.path.realpath() is an example [1].\n\nEither approach would avoid chdir()ing around even temporarily, which\nwould anyway be bad form in git proper (as opposed to the test suite).\nAnd it would avoid the need to chdir() permanently in the test suite,\nwhich can have the effect of making directory names appear in unfamiliar\nforms.\n\nI'm afraid I don't have time to work on this now; I'm still trying to\nfinish the next iteration of the post-receive-email hook script replacement.\n\nMichael\n\n[1] (Python) source code here:\n\n http://hg.python.org/cpython/file/20985f52b65e/Lib/posixfile.py\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"197880","messageId":"f58965733e604a9fe6ed72384d0307062403b478.1346043214.git.worldhello.net@gmail.com","threadId":"31082","inReplyTo":"5030F0BF.2090500@alum.mit.edu","subject":"[PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2012-08-27T05:13:36Z","receivedAt":"2012-08-27T05:13:36Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"Some testcases will fail if current work directory is on a symlink.\n\n symlink$ sh ./t4035-diff-quiet.sh\n $ sh ./t4035-diff-quiet.sh --root=/symlink\n $ TEST_OUTPUT_DIRECTORY=/symlink sh ./t4035-diff-quiet.sh\n\nThis is because the realpath of \".git\" directory will be returned when\nrunning the command 'git rev-parse --git-dir' in a subdir of the work\ntree, and the realpath may not equal to \"$TRASH_DIRECTORY\".\n\nIn this fix, \"$TRASH_DIRECTORY\" is determined right after the realpath\nof CWD is resolved.\n\nSigned-off-by: Jiang Xin \nReported-by: Michael Haggerty \nSigned-off-by: Jiang Xin \n---\n t/test-lib.sh | 9 +++++----\n 1 个文件被修改，插入 5 行(+)，删除 4 行(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 78c42..9a59ca8 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -531,17 +531,17 @@ fi\n test=\"trash directory.$(basename \"$0\" .sh)\"\n test -n \"$root\" && test=\"$root/$test\"\n case \"$test\" in\n-/*) TRASH_DIRECTORY=\"$test\" ;;\n- *) TRASH_DIRECTORY=\"$TEST_OUTPUT_DIRECTORY/$test\" ;;\n+/*) ;;\n+ *) test=\"$TEST_OUTPUT_DIRECTORY/$test\" ;;\n esac\n-test ! -z \"$debug\" || remove_trash=$TRASH_DIRECTORY\n+test ! -z \"$debug\" || remove_trash=$test\n rm -fr \"$test\" || {\n \tGIT_EXIT_OK=t\n \techo >&5 \"FATAL: Cannot prepare test area\"\n \texit 1\n }\n \n-HOME=\"$TRASH_DIRECTORY\"\n+HOME=\"$test\"\n export HOME\n \n if test -z \"$TEST_NO_CREATE_REPO\"; then\n@@ -552,6 +552,7 @@ fi\n # Use -P to resolve symlinks in our working directory so that the cwd\n # in subprocesses like git equals our $PWD (for pathname comparisons).\n cd -P \"$test\" || exit 1\n+TRASH_DIRECTORY=\"$(pwd)\"\n \n this_test=${0##*/}\n this_test=${this_test%%-*}\n-- \n1.7.12.92.gaa91cb5\n"},{"id":"197913","messageId":"7vk3wktiph.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"f58965733e604a9fe6ed72384d0307062403b478.1346043214.git.worldhello.net@gmail.com","subject":"Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-27T16:15:06Z","receivedAt":"2012-08-27T16:15:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> Some testcases will fail if current work directory is on a symlink.\n>\n> symlink$ sh ./t4035-diff-quiet.sh\n> $ sh ./t4035-diff-quiet.sh --root=/symlink\n> $ TEST_OUTPUT_DIRECTORY=/symlink sh ./t4035-diff-quiet.sh\n>\n> This is because the realpath of \".git\" directory will be returned when\n> running the command 'git rev-parse --git-dir' in a subdir of the work\n> tree, and the realpath may not equal to \"$TRASH_DIRECTORY\".\n>\n> In this fix, \"$TRASH_DIRECTORY\" is determined right after the realpath\n> of CWD is resolved.\n>\n> Signed-off-by: Jiang Xin \n> Reported-by: Michael Haggerty \n> Signed-off-by: Jiang Xin \n> ---\n\nI think this is in line with what was discussed in the other thread\nMichael brought this up. Thanks for following it through.\n\nMichael, this looks good to me; anything I missed?\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 78c42..9a59ca8 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -531,17 +531,17 @@ fi\n> test=\"trash directory.$(basename \"$0\" .sh)\"\n> test -n \"$root\" && test=\"$root/$test\"\n> case \"$test\" in\n> -/*) TRASH_DIRECTORY=\"$test\" ;;\n> - *) TRASH_DIRECTORY=\"$TEST_OUTPUT_DIRECTORY/$test\" ;;\n> +/*) ;;\n> + *) test=\"$TEST_OUTPUT_DIRECTORY/$test\" ;;\n> esac\n> -test ! -z \"$debug\" || remove_trash=$TRASH_DIRECTORY\n> +test ! -z \"$debug\" || remove_trash=$test\n> rm -fr \"$test\" || {\n> \tGIT_EXIT_OK=t\n> \techo >&5 \"FATAL: Cannot prepare test area\"\n> \texit 1\n> }\n> \n> -HOME=\"$TRASH_DIRECTORY\"\n> +HOME=\"$test\"\n> export HOME\n> \n> if test -z \"$TEST_NO_CREATE_REPO\"; then\n> @@ -552,6 +552,7 @@ fi\n> # Use -P to resolve symlinks in our working directory so that the cwd\n> # in subprocesses like git equals our $PWD (for pathname comparisons).\n> cd -P \"$test\" || exit 1\n> +TRASH_DIRECTORY=\"$(pwd)\"\n> \n> this_test=${0##*/}\n> this_test=${this_test%%-*}\n"},{"id":"198022","messageId":"503D973D.8040402@alum.mit.edu","threadId":"31082","inReplyTo":"7vk3wktiph.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-29T04:14:53Z","receivedAt":"2012-08-29T04:14:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/27/2012 06:15 PM, Junio C Hamano wrote:\n> Jiang Xin writes:\n> \n>> Some testcases will fail if current work directory is on a symlink.\n>>\n>> symlink$ sh ./t4035-diff-quiet.sh\n>> $ sh ./t4035-diff-quiet.sh --root=/symlink\n>> $ TEST_OUTPUT_DIRECTORY=/symlink sh ./t4035-diff-quiet.sh\n>>\n>> This is because the realpath of \".git\" directory will be returned when\n>> running the command 'git rev-parse --git-dir' in a subdir of the work\n>> tree, and the realpath may not equal to \"$TRASH_DIRECTORY\".\n>>\n>> In this fix, \"$TRASH_DIRECTORY\" is determined right after the realpath\n>> of CWD is resolved.\n>>\n>> Signed-off-by: Jiang Xin \n>> Reported-by: Michael Haggerty \n>> Signed-off-by: Jiang Xin \n>> ---\n> \n> I think this is in line with what was discussed in the other thread\n> Michael brought this up. Thanks for following it through.\n> \n> Michael, this looks good to me; anything I missed?\n\nI can confirm that this patch eliminates the test failures that I was\nseeing in t4035 and t9903.\n\nBut it also changes almost 600 *other* tests from \"succeed even in the\npresence of symlinks\" to \"never tested in the presence of symlinks\", and\nI think that is surrendering more ground than necessary. I would rather\nsee one of the following approaches:\n\n*If* the official policy is that GIT_CEILING_DIRECTORIES is not reliable\nin the presence of symlinks, then (a) that limitation should be\nmentioned in the documentation; (b) the affected tests should either be\nskipped in the case of symlinked directories or they (alone!) should\ntake measures to work around the problem.\n\n*If* the official policy is that GIT_CEILING_DIRECTORIES should work in\nthe presence of symlinks, then (a) we should add a failing test case\nthat specifically documents this bug; (b) other tests that fail as a\nside effect of this bug even though they are trying to test something\nelse should either be skipped in the case of symlinked directories or\nthey (alone!) should take measures to work around the problem; (c) the\nbug should be fixed as soon as possible.\n\nIn fact, we could even go further:\n\n* The \"cd -P\" in test-lib.sh could be changed to \"cd -L\", to avoid\nmasking other problems related to symlinks. If I make that change, I\nget test failures in 10 files when using --root=/symlinkeddir, and not\nall of them are obviously related to GIT_CEILING_DIRECTORIES. Some of\nthese might simply be sloppiness in how the test scripts are written,\nbut others might be bugs in git proper.\n\n* We could *intentionally* access the trash directories via a symlink on\nsystems that support symlinks (much like the trash directory names\nintentionally include a space) to get *systematic* test coverage of\nsymlink issues, rather than occasional testing and mysterious failures\nwhen somebody happens to have a symlink in his build path or root.\n\n(But it is still the case that I don't have time to work on this.)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198025","messageId":"7vmx1exmde.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"503D973D.8040402@alum.mit.edu","subject":"Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-29T06:06:53Z","receivedAt":"2012-08-29T06:06:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> But it also changes almost 600 *other* tests from \"succeed even in the\n> presence of symlinks\" to \"never tested in the presence of symlinks\", and\n> I think that is surrendering more ground than necessary.\n\nOuch. I did not know have 600+ tests that cares about CEILING.\n\n> I would rather\n> see one of the following approaches:\n>\n> *If* the official policy is that GIT_CEILING_DIRECTORIES is not reliable\n> in the presence of symlinks, then (a) that limitation should be\n> mentioned in the documentation; (b) the affected tests should either be\n> skipped in the case of symlinked directories or they (alone!) should\n> take measures to work around the problem.\n\nWhat exactly is broken in CEILING?\n\nI somehow thought that Jiang's patch was to make sure any variables\nthat contain pathnames (and make sure future paths we might grab out\nof $(pwd)) are realpath without symlinks early in the test set-up,\nand with that arrangement, no symlink gotcha should come into\npicture, with or without CEILING.\n\nPerhaps the setting of the CEILING has not been correctly converted\nwith the patch?\n\nOr is there something fundamentally broken, even if we do not have\nany symlinks involved, with CEILING check?\n\nPuzzled..\n"},{"id":"198034","messageId":"503DCFAE.6060307@alum.mit.edu","threadId":"31082","inReplyTo":"7vmx1exmde.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-29T08:15:42Z","receivedAt":"2012-08-29T08:15:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/29/2012 08:06 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> But it also changes almost 600 *other* tests from \"succeed even in the\n>> presence of symlinks\" to \"never tested in the presence of symlinks\", and\n>> I think that is surrendering more ground than necessary.\n> \n> Ouch. I did not know have 600+ tests that cares about CEILING.\n\nNo, I'm referring to the whole test suite, which is approximately 600\nfiles :-) Because the patch changes TEST_DIRECTORY, it affects the\nenvironment of all tests that use that variable (one of which being via\nGIT_CEILING_DIRECTORIES).\n\nBut really I shouldn't have made it sound like this patch is so\nterrible, because it is just a logical continuation of the approach begun by\n\n 1bd9c648 t/test-lib.sh: resolve symlinks in working directory, for\npathname comparisons (2008-05-31)\n\nIt is in fact the whole approach that I object to.\n\n>> I would rather\n>> see one of the following approaches:\n>>\n>> *If* the official policy is that GIT_CEILING_DIRECTORIES is not reliable\n>> in the presence of symlinks, then (a) that limitation should be\n>> mentioned in the documentation; (b) the affected tests should either be\n>> skipped in the case of symlinked directories or they (alone!) should\n>> take measures to work around the problem.\n> \n> What exactly is broken in CEILING?\n\nCEILING is used in setup_git_directory_gently_1() when trying to find\nthe GIT_DIR appropriate for the current directory. The entries in\nCEILING are compared textually to getcwd(), looking for the entry that\nis the longest proper prefix of PWD. This is then used to limit a loop\nthat is vaguely\n\n while (!is_git_directory())\n chdir(\"..\");\n\nThe problem, as I understand it, is that when the tests are run with\nroot set to a path that includes a symlink, then test and\nTRASH_DIRECTORY are set to the textual value of \"$root/trash\ndirectory.tXXXX\", but then a little bit later test-lib.sh chdirs into\nthe trash directory using \"cd -P $test\" (thereby resolving symlinks).\nSo, taking my concrete example \"--root=/dev/shm\" where /dev/shm is a\nsymlink to /run/shm, we have\n\n test=\"/dev/shm/trash directory.tXXXX\"\n TRASH_DIRECTORY=\"$test\"\n ...\n cd -P \"$test\"\n\nwhich results in PWD being \"/run/shm/trash directory.tXXXX\".\n\nThen (for example) in test t4035, we have stuff like\n\n GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n export GIT_CEILING_DIRECTORIES &&\n cd test-outside/non/git &&\n git diff\n\nThe problem is that because PWD and TRASH_DIRECTORY *look* different,\nthe code in setup_git_directory_gently_1() doesn't realize that the\nvalue of GIT_CEILING_DIRECTORIES is a parent of PWD, so it doesn't abort\nthe search for GIT_DIR there, and this causes the test to fail.\n\nThe underlying problem is that setup_git_directory_gently_1() isn't\nsmart enough to realize that the directory in GIT_CEILING_DIRECTORIES is\nin fact a parent of PWD even though it textually doesn't look like one.\n\nThe patch being discussed sets TEST_DIRECTORY *after* $test has been\ncanonicalized (through the use of \"cd -P $test\"), so that TEST_DIRECTORY\nand later GIT_CEILING_DIRECTORIES start with \"/run/shm/\" instead of\n\"/dev/shm/\". This change makes setup_git_directory_gently_1()'s naive\ntextual prefix comparison succeed.\n\nThe same problem would occur in the real world whenever the user sets\nGIT_CEILING_DIRECTORIES to a value that is *in fact* a parent of PWD but\ndue to symbolic links *textually* does not appear to be a parent.\n\nSo the first decision is whether this should be documented as a known\nlimitation of GIT_CEILING_DIRECTORIES, or whether it is a bug. My\nopinion is that it is a bug.\n\n> I somehow thought that Jiang's patch was to make sure any variables\n> that contain pathnames (and make sure future paths we might grab out\n> of $(pwd)) are realpath without symlinks early in the test set-up,\n> and with that arrangement, no symlink gotcha should come into\n> picture, with or without CEILING.\n\nYes, this is correct. But what you call a \"gotcha\" is actually a\nreal-world possibility. Git might *really* be run in a PWD that\ncontains symlinks, or GIT_CEILING_DIRECTORIES might contain entries that\nare symlinks. So by resolving symlinks in PWD before running tests, we\nare preventing the tests from ever encountering this situation, and\ntherefore failing to test whether git works correctly when PWD includes\na symlink.\n\n> Perhaps the setting of the CEILING has not been correctly converted\n> with the patch?\n\nThat's not the problem.\n\n> Or is there something fundamentally broken, even if we do not have\n> any symlinks involved, with CEILING check?\n\nI believe that:\n\n1. The handling of GIT_CEILING_DIRECTORIES in\nsetup_git_directory_gently_1() (i.e., in git itself, not how it is used\nin the test suite) is correct if no symlinks are involved, but breaks in\nthe face of symlinks.\n\n2. The proposed patch is a mistake because it masks the brokenness of\nsetup_git_directory_gently_1().\n\n3. The old commit 1bd9c648 is an even bigger mistake because it might\nmask other breakages throughout git in dealing with working directories\nthat contain symlinks.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198046","messageId":"7vy5kxwtdo.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"503DCFAE.6060307@alum.mit.edu","subject":"Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-29T16:33:07Z","receivedAt":"2012-08-29T16:33:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 08/29/2012 08:06 AM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>\n> It is in fact the whole approach that I object to.\n> ...\n>> What exactly is broken in CEILING?\n>\n> CEILING is used in setup_git_directory_gently_1() when trying to find\n> the GIT_DIR appropriate for the current directory. The entries in\n> CEILING are compared textually to getcwd(), looking for the entry that\n> is the longest proper prefix of PWD. This is then used to limit a loop\n> that is vaguely\n>\n> while (!is_git_directory())\n> chdir(\"..\");\n>\n> The problem, as I understand it, is that when the tests are run with\n> root set to a path that includes a symlink, then test and\n> TRASH_DIRECTORY are set to the textual value of \"$root/trash\n> directory.tXXXX\", but then a little bit later test-lib.sh chdirs into\n> the trash directory using \"cd -P $test\" (thereby resolving symlinks).\n> So, taking my concrete example \"--root=/dev/shm\" where /dev/shm is a\n> symlink to /run/shm, we have\n>\n> test=\"/dev/shm/trash directory.tXXXX\"\n> TRASH_DIRECTORY=\"$test\"\n> ...\n> cd -P \"$test\"\n>\n> which results in PWD being \"/run/shm/trash directory.tXXXX\".\n\nThe components of CEILING should be adjusted to strip the symlink so\nthat it begins with \"/run/shm/\" before it is used; otherwise it\nwould not work. As the current code does not do that at runtime (I\nam describing the situation, not justifying), it won't match if\nCEILING is built out of $TRASH_DIRECTORY. In the above, setting of\n$test and $TRASH is wrong; it does not match the realpath.\n\nSo \"I somehow thought that Jiang's patch was to make sure any\nvariables that contain pathnames (and make sure future paths we\nmight grab out of $(pwd)) are realpath without symlinks early in the\ntest set-up,\" in my previous message was not correct. The patch is\nnot doing that, and that is what breaks your setup.\n\nMy preference is to set things up in such a way that most of the\ntests do not have to worry about these symlink aliases. I know you\nsaid you do not like the \"whole approach\", but I'd like to see our\ntests run in a stable and reproducible testing environment.\n\nWe should have specific tests (on symlink enabled platforms) that\ncreates a directory and an alias to it via a symlink, goes into it\nand checks the CEILING (and whatever else) behaviour. We know that\nthe current code does not account for the alias introduced by\nsymlinks, and setup_git_directory_gently() may want to be patched to\nfix _that_ specific test.\n"},{"id":"198084","messageId":"503EEDF3.2080202@alum.mit.edu","threadId":"31082","inReplyTo":"7vy5kxwtdo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-30T04:37:07Z","receivedAt":"2012-08-30T04:37:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/29/2012 06:33 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> On 08/29/2012 08:06 AM, Junio C Hamano wrote:\n>>> Michael Haggerty writes:\n>>\n>> It is in fact the whole approach that I object to.\n>> ...\n>>> What exactly is broken in CEILING?\n>>\n>> CEILING is used in setup_git_directory_gently_1() when trying to find\n>> the GIT_DIR appropriate for the current directory. The entries in\n>> CEILING are compared textually to getcwd(), looking for the entry that\n>> is the longest proper prefix of PWD. This is then used to limit a loop\n>> that is vaguely\n>>\n>> while (!is_git_directory())\n>> chdir(\"..\");\n>>\n>> The problem, as I understand it, is that when the tests are run with\n>> root set to a path that includes a symlink, then test and\n>> TRASH_DIRECTORY are set to the textual value of \"$root/trash\n>> directory.tXXXX\", but then a little bit later test-lib.sh chdirs into\n>> the trash directory using \"cd -P $test\" (thereby resolving symlinks).\n>> So, taking my concrete example \"--root=/dev/shm\" where /dev/shm is a\n>> symlink to /run/shm, we have\n>>\n>> test=\"/dev/shm/trash directory.tXXXX\"\n>> TRASH_DIRECTORY=\"$test\"\n>> ...\n>> cd -P \"$test\"\n>>\n>> which results in PWD being \"/run/shm/trash directory.tXXXX\".\n> \n> The components of CEILING should be adjusted to strip the symlink so\n> that it begins with \"/run/shm/\" before it is used; otherwise it\n> would not work. As the current code does not do that at runtime (I\n> am describing the situation, not justifying), it won't match if\n> CEILING is built out of $TRASH_DIRECTORY. In the above, setting of\n> $test and $TRASH is wrong; it does not match the realpath.\n> \n> So \"I somehow thought that Jiang's patch was to make sure any\n> variables that contain pathnames (and make sure future paths we\n> might grab out of $(pwd)) are realpath without symlinks early in the\n> test set-up,\" in my previous message was not correct. The patch is\n> not doing that, and that is what breaks your setup.\n\nI've confused things by not being clear what I was describing. The\ndescription that you quoted above was the state *before* Jiang's patch.\n Jiang's patch makes sure that $TRASH_DIRECTORY is a realpath. The\nworking directory was already a realpath before his patch (due to commit\n1bd9c648). There are some other variables that are not necessarily\nrealpaths, even after Jiang's patch; for example (via a casual look at\ntest-lib.sh): $remove_trash, $HOME, $test, $TEST_DIRECTORY,\n$test_results_dir, $GIT_BUILD_DIR. I haven't checked whether these\nvariables are used in ways that could cause other problems.\n\nWhen Jiang's patch is applied the test suite runs to completion without\nany failures on my system even when I pass --root=/dev/shm (a symlink).\n\n> My preference is to set things up in such a way that most of the\n> tests do not have to worry about these symlink aliases. I know you\n> said you do not like the \"whole approach\", but I'd like to see our\n> tests run in a stable and reproducible testing environment.\n> \n> We should have specific tests (on symlink enabled platforms) that\n> creates a directory and an alias to it via a symlink, goes into it\n> and checks the CEILING (and whatever else) behaviour. We know that\n> the current code does not account for the alias introduced by\n> symlinks, and setup_git_directory_gently() may want to be patched to\n> fix _that_ specific test.\n\nYes, stable and reproducible is good. And explicit tests for\nsymlink-sensitive code would be good. But how do we find the code that\nneeds explicit testing for symlink tolerance?\n\nTherefore (in addition to specific tests) I think it would be good to\nhave an easy way to run the *whole* test suite in a symlink environment.\n For example, we could add a test suite option that *explicitly* makes\nall tests run in a directory containing symlinks. Or we could even do\nthat *all* of the time on systems that support symlinks [1]--that would\nbe a stable and reproducible testing environment that *does* detect many\nsymlink problems rather than hiding them. (It seems unlikely the the\nuse of symlinks would *hide* other bugs.) Something along the lines of\n\n mkdir -p \"$test.dir\"\n ln -s \"$test.dir\" \"$test\"\n\nBy the way, is the use of realpath(3) permissible in git code?\nGIT_CEILING_DIRECTORIES handling could be fixed relatively easily by\nusing this function to canonicalize pathnames before comparison.\n\nMichael\n\n[1] This would be analogous to the inclusion of a space in \"trash\ndirectory.*\", which I presume was done to detect space-handling problems\nquickly.\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198085","messageId":"7vzk5dt0g5.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"503EEDF3.2080202@alum.mit.edu","subject":"Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-30T05:26:18Z","receivedAt":"2012-08-30T05:26:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> By the way, is the use of realpath(3) permissible in git code?\n> GIT_CEILING_DIRECTORIES handling could be fixed relatively easily by\n> using this function to canonicalize pathnames before comparison.\n\nAs long as we can add a compat/realpath.c (perhaps lift one from\nglibc before they went GPLv3) for platforms that matter, I do not\nsee it as a huge problem. How close is abspath.c::real_path() to\nwhat you need?\n\n> [1] This would be analogous to the inclusion of a space in \"trash\n> directory.*\", which I presume was done to detect space-handling problems\n> quickly.\n\nYeah, understood where you are coming from, and I think I can agree\nwith where you are trying to go.\n"},{"id":"198141","messageId":"50406C82.2070005@alum.mit.edu","threadId":"31082","inReplyTo":"7vzk5dt0g5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] test: set the realpath of CWD as TRASH_DIRECTORY","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-08-31T07:49:22Z","receivedAt":"2012-08-31T07:49:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/30/2012 07:26 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> By the way, is the use of realpath(3) permissible in git code?\n>> GIT_CEILING_DIRECTORIES handling could be fixed relatively easily by\n>> using this function to canonicalize pathnames before comparison.\n> \n> As long as we can add a compat/realpath.c (perhaps lift one from\n> glibc before they went GPLv3) for platforms that matter, I do not\n> see it as a huge problem. How close is abspath.c::real_path() to\n> what you need?\n\nCool, I didn't know about that function. It's approximately what is\nneeded, except that it dies if fed an invalid path (unacceptable when\nprocessing GIT_CEILING_DIRECTORIES) and it's buggy (try \"test-path-utils\nreal_path ''\" or \"test-path-utils real_path '/foo'\"). However, I'm\nworking on fixing it and splitting off a variant that returns NULL on\nerrors.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"199963","messageId":"1348688090-13648-1-git-send-email-mhagger@alum.mit.edu","threadId":"31082","inReplyTo":"50406C82.2070005@alum.mit.edu","subject":"[PATCH 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-26T19:34:42Z","receivedAt":"2012-09-26T19:34:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This series fixes longest_ancestor_length() so that it works even if\nprefix_list contains entries that involve symlinks. The basic goal of\nthe series is to call real_path() on each of the entries so that a\ntextual comparison of the potential prefix to the front of path\ncorrectly decides whether the path is located inside of the entry.\nBut along the way some other things had to be changed:\n\n* real_path() die()s if the path passed to it is invalid, whereas it\n is allowed for GIT_CEILING_DIRECTORIES to contain invalid paths. So\n create a new function real_path_if_valid() that returns NULL for\n invalid paths.\n\n* Changing longest_ancestor_length() to call real_path_if_valid()\n would make the former very difficult to test (because the tests\n would depend on the contents of the whole filesystem). Therefore,\n rewrite longest_ancestor_length() in terms of functions\n string_list_split(), string_list_longest_prefix(), and\n real_path_if_valid() which are tested individually.\n\nThe net results of these changes are that:\n\n1. t1504 used to have to canonicalize TRASH_DIRECTORY to make itself\n work even if the --root directory contains symlinks. This\n canonicalization is no longer necessary (and has been removed).\n\n2. t4035, which used to fail if the --root directory contained\n symlinks, now works correctly in that situation.\n\nAfter this change, all tests pass if the --root directory does *not*\ncontain symlinks, but t9903 still fails if the --root directory\ncontains symlinks. I haven't analyzed the cause of t9903's failure,\nbut it does not appear to be related to the GIT_CEILING_DIRECTORIES\nfeature.\n\nOn the mailing list I suggested *purposely* inserting symlinks into\nthe \"trash directory.*\" paths to test symlink handling more\nsystematically. This patch series does *NOT* make that change.\n\nMichael Haggerty (8):\n Introduce new static function real_path_internal()\n Introduce new function real_path_if_valid()\n longest_ancestor_length(): use string_list_split()\n longest_ancestor_length(): explicitly filter list before loop\n longest_ancestor_length(): always add a slash to the end of prefixes\n longest_ancestor_length(): use string_list_longest_prefix()\n longest_ancestor_length(): resolve symlinks before comparing paths\n t1504: stop resolving symlinks in GIT_CEILING_DIRECTORIES\n\n abspath.c | 98 ++++++++++++++++++++++++++++++++++++++-----------\n cache.h | 1 +\n path.c | 54 ++++++++++++++++-----------\n t/t0060-path-utils.sh | 64 --------------------------------\n t/t1504-ceiling-dirs.sh | 67 +++++++++++++++++----------------\n 5 files changed, 144 insertions(+), 140 deletions(-)\n\n-- \n1.7.11.3\n"},{"id":"199964","messageId":"1348688090-13648-2-git-send-email-mhagger@alum.mit.edu","threadId":"31082","inReplyTo":"1348688090-13648-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/8] Introduce new static function real_path_internal()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-26T19:34:43Z","receivedAt":"2012-09-26T19:34:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It accepts a new parameter, die_on_error. If die_on_error is false,\nit simply cleans up after itself and returns NULL rather than dying.\n\nSigned-off-by: Michael Haggerty \n---\n abspath.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++---------------\n 1 file changed, 72 insertions(+), 21 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 05f2d79..a7ab8e9 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -15,15 +15,26 @@ int is_directory(const char *path)\n #define MAXDEPTH 5\n \n /*\n- * Use this to get the real path, i.e. resolve links. If you want an\n- * absolute path but don't mind links, use absolute_path.\n+ * Return the real path (i.e., absolute path, with symlinks resolved\n+ * and extra slashes removed) equivalent to the specified path. (If\n+ * you want an absolute path but don't mind links, use\n+ * absolute_path().) The return value is a pointer to a static\n+ * buffer.\n+ *\n+ * The input and all intermediate paths must be shorter than MAX_PATH.\n+ * The directory part of path (i.e., everything up to the last\n+ * dir_sep) must denote a valid, existing directory, but the last\n+ * component need not exist. If die_on_error is set, then die with an\n+ * informative error message if there is a problem. Otherwise, return\n+ * NULL on errors (without generating any output).\n *\n * If path is our buffer, then return path, as it's already what the\n * user wants.\n */\n-const char *real_path(const char *path)\n+static const char *real_path_internal(const char *path, int die_on_error)\n {\n \tstatic char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];\n+\tchar *retval = NULL;\n \tchar cwd[1024] = \"\";\n \tint buf_index = 1;\n \n@@ -35,11 +46,19 @@ const char *real_path(const char *path)\n \tif (path == buf || path == next_buf)\n \t\treturn path;\n \n-\tif (!*path)\n-\t\tdie(\"The empty string is not a valid path\");\n+\tif (!*path) {\n+\t\tif (die_on_error)\n+\t\t\tdie(\"The empty string is not a valid path\");\n+\t\telse\n+\t\t\tgoto error_out;\n+\t}\n \n-\tif (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)\n-\t\tdie (\"Too long path: %.*s\", 60, path);\n+\tif (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) {\n+\t\tif (die_on_error)\n+\t\t\tdie(\"Too long path: %.*s\", 60, path);\n+\t\telse\n+\t\t\tgoto error_out;\n+\t}\n \n \twhile (depth--) {\n \t\tif (!is_directory(buf)) {\n@@ -54,20 +73,36 @@ const char *real_path(const char *path)\n \t\t}\n \n \t\tif (*buf) {\n-\t\t\tif (!*cwd && !getcwd(cwd, sizeof(cwd)))\n-\t\t\t\tdie_errno (\"Could not get current working directory\");\n+\t\t\tif (!*cwd && !getcwd(cwd, sizeof(cwd))) {\n+\t\t\t\tif (die_on_error)\n+\t\t\t\t\tdie_errno(\"Could not get current working directory\");\n+\t\t\t\telse\n+\t\t\t\t\tgoto error_out;\n+\t\t\t}\n \n-\t\t\tif (chdir(buf))\n-\t\t\t\tdie_errno (\"Could not switch to '%s'\", buf);\n+\t\t\tif (chdir(buf)) {\n+\t\t\t\tif (die_on_error)\n+\t\t\t\t\tdie_errno(\"Could not switch to '%s'\", buf);\n+\t\t\t\telse\n+\t\t\t\t\tgoto error_out;\n+\t\t\t}\n+\t\t}\n+\t\tif (!getcwd(buf, PATH_MAX)) {\n+\t\t\tif (die_on_error)\n+\t\t\t\tdie_errno(\"Could not get current working directory\");\n+\t\t\telse\n+\t\t\t\tgoto error_out;\n \t\t}\n-\t\tif (!getcwd(buf, PATH_MAX))\n-\t\t\tdie_errno (\"Could not get current working directory\");\n \n \t\tif (last_elem) {\n \t\t\tsize_t len = strlen(buf);\n-\t\t\tif (len + strlen(last_elem) + 2 > PATH_MAX)\n-\t\t\t\tdie (\"Too long path name: '%s/%s'\",\n-\t\t\t\t\t\tbuf, last_elem);\n+\t\t\tif (len + strlen(last_elem) + 2 > PATH_MAX) {\n+\t\t\t\tif (die_on_error)\n+\t\t\t\t\tdie(\"Too long path name: '%s/%s'\",\n+\t\t\t\t\t buf, last_elem);\n+\t\t\t\telse\n+\t\t\t\t\tgoto error_out;\n+\t\t\t}\n \t\t\tif (len && !is_dir_sep(buf[len-1]))\n \t\t\t\tbuf[len++] = '/';\n \t\t\tstrcpy(buf + len, last_elem);\n@@ -77,10 +112,18 @@ const char *real_path(const char *path)\n \n \t\tif (!lstat(buf, &st) && S_ISLNK(st.st_mode)) {\n \t\t\tssize_t len = readlink(buf, next_buf, PATH_MAX);\n-\t\t\tif (len < 0)\n-\t\t\t\tdie_errno (\"Invalid symlink '%s'\", buf);\n-\t\t\tif (PATH_MAX <= len)\n-\t\t\t\tdie(\"symbolic link too long: %s\", buf);\n+\t\t\tif (len < 0) {\n+\t\t\t\tif (die_on_error)\n+\t\t\t\t\tdie_errno(\"Invalid symlink '%s'\", buf);\n+\t\t\t\telse\n+\t\t\t\t\tgoto error_out;\n+\t\t\t}\n+\t\t\tif (PATH_MAX <= len) {\n+\t\t\t\tif (die_on_error)\n+\t\t\t\t\tdie(\"symbolic link too long: %s\", buf);\n+\t\t\t\telse\n+\t\t\t\t\tgoto error_out;\n+\t\t\t}\n \t\t\tnext_buf[len] = '\\0';\n \t\t\tbuf = next_buf;\n \t\t\tbuf_index = 1 - buf_index;\n@@ -89,10 +132,18 @@ const char *real_path(const char *path)\n \t\t\tbreak;\n \t}\n \n+\tretval = buf;\n+error_out:\n+\tfree(last_elem);\n \tif (*cwd && chdir(cwd))\n \t\tdie_errno (\"Could not change back to '%s'\", cwd);\n \n-\treturn buf;\n+\treturn retval;\n+}\n+\n+const char *real_path(const char *path)\n+{\n+\treturn real_path_internal(path, 1);\n }\n \n static const char *get_pwd_cwd(void)\n-- \n1.7.11.3\n"},{"id":"199965","messageId":"1348688090-13648-3-git-send-email-mhagger@alum.mit.edu","threadId":"31082","inReplyTo":"1348688090-13648-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/8] Introduce new function real_path_if_valid()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-26T19:34:44Z","receivedAt":"2012-09-26T19:34:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The function is like real_path(), except that it returns NULL on error\ninstead of dying.\n\nSigned-off-by: Michael Haggerty \n---\n abspath.c | 5 +++++\n cache.h | 1 +\n 2 files changed, 6 insertions(+)\n\ndiff --git a/abspath.c b/abspath.c\nindex a7ab8e9..5748b91 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -146,6 +146,11 @@ const char *real_path(const char *path)\n \treturn real_path_internal(path, 1);\n }\n \n+const char *real_path_if_valid(const char *path)\n+{\n+\treturn real_path_internal(path, 0);\n+}\n+\n static const char *get_pwd_cwd(void)\n {\n \tstatic char cwd[PATH_MAX + 1];\ndiff --git a/cache.h b/cache.h\nindex a58df84..b0d75bc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -714,6 +714,7 @@ static inline int is_absolute_path(const char *path)\n }\n int is_directory(const char *);\n const char *real_path(const char *path);\n+const char *real_path_if_valid(const char *path);\n const char *absolute_path(const char *path);\n const char *relative_path(const char *abs, const char *base);\n int normalize_path_copy(char *dst, const char *src);\n-- \n1.7.11.3\n"},{"id":"199967","messageId":"1348688090-13648-4-git-send-email-mhagger@alum.mit.edu","threadId":"31082","inReplyTo":"1348688090-13648-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/8] longest_ancestor_length(): use string_list_split()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-26T19:34:45Z","receivedAt":"2012-09-26T19:34:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n path.c | 17 +++++++++++------\n 1 file changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex cbbdf7d..969bc17 100644\n--- a/path.c\n+++ b/path.c\n@@ -12,6 +12,7 @@\n */\n #include \"cache.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n \n static char bad_path[] = \"/bad-path/\";\n \n@@ -582,19 +583,22 @@ int normalize_path_copy(char *dst, const char *src)\n */\n int longest_ancestor_length(const char *path, const char *prefix_list)\n {\n+\tstruct string_list prefixes = STRING_LIST_INIT_DUP;\n \tchar buf[PATH_MAX+1];\n-\tconst char *ceil, *colon;\n-\tint len, max_len = -1;\n+\tint i, max_len = -1;\n \n \tif (prefix_list == NULL || !strcmp(path, \"/\"))\n \t\treturn -1;\n \n-\tfor (colon = ceil = prefix_list; *colon; ceil = colon+1) {\n-\t\tfor (colon = ceil; *colon && *colon != PATH_SEP; colon++);\n-\t\tlen = colon - ceil;\n+\tstring_list_split(&prefixes, prefix_list, PATH_SEP, -1);\n+\n+\tfor (i = 0; i < prefixes.nr; i++) {\n+\t\tconst char *ceil = prefixes.items[i].string;\n+\t\tint len = strlen(ceil);\n+\n \t\tif (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))\n \t\t\tcontinue;\n-\t\tstrlcpy(buf, ceil, len+1);\n+\t\tmemcpy(buf, ceil, len+1);\n \t\tif (normalize_path_copy(buf, buf) < 0)\n \t\t\tcontinue;\n \t\tlen = strlen(buf);\n@@ -608,6 +612,7 @@ int longest_ancestor_length(const char *path, const char *prefix_list)\n \t\t}\n \t}\n \n+\tstring_list_clear(&prefixes, 0);\n \treturn max_len;\n }\n \n-- \n1.7.11.3\n"},{"id":"199966","messageId":"1348688090-13648-5-git-send-email-mhagger@alum.mit.edu","threadId":"31082","inReplyTo":"1348688090-13648-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/8] longest_ancestor_length(): explicitly filter list before loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-26T19:34:46Z","receivedAt":"2012-09-26T19:34:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Separate the step of filtering and normalizing elements of the\nprefixes list from the iteration that looks for the longest prefix.\nThis will help keep the function testable after we not only normalize\nthe paths, but also convert them into real paths.\n\nSigned-off-by: Michael Haggerty \n---\n path.c | 32 +++++++++++++++++++++-----------\n 1 file changed, 21 insertions(+), 11 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 969bc17..6da7029 100644\n--- a/path.c\n+++ b/path.c\n@@ -568,6 +568,25 @@ int normalize_path_copy(char *dst, const char *src)\n \treturn 0;\n }\n \n+static int normalize_path_callback(struct string_list_item *item, void *cb_data)\n+{\n+\tchar buf[PATH_MAX+1];\n+\tconst char *ceil = item->string;\n+\tint len = strlen(ceil);\n+\n+\tif (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))\n+\t\treturn 0;\n+\tmemcpy(buf, ceil, len+1);\n+\tif (normalize_path_copy(buf, buf) < 0)\n+\t\treturn 0;\n+\tlen = strlen(buf);\n+\tif (len > 0 && buf[len-1] == '/')\n+\t\tbuf[--len] = '\\0';\n+\tfree(item->string);\n+\titem->string = xstrdup(buf);\n+\treturn 1;\n+}\n+\n /*\n * path = Canonical absolute path\n * prefix_list = Colon-separated list of absolute paths\n@@ -584,28 +603,19 @@ int normalize_path_copy(char *dst, const char *src)\n int longest_ancestor_length(const char *path, const char *prefix_list)\n {\n \tstruct string_list prefixes = STRING_LIST_INIT_DUP;\n-\tchar buf[PATH_MAX+1];\n \tint i, max_len = -1;\n \n \tif (prefix_list == NULL || !strcmp(path, \"/\"))\n \t\treturn -1;\n \n \tstring_list_split(&prefixes, prefix_list, PATH_SEP, -1);\n+\tfilter_string_list(&prefixes, 0, normalize_path_callback, NULL);\n \n \tfor (i = 0; i < prefixes.nr; i++) {\n \t\tconst char *ceil = prefixes.items[i].string;\n \t\tint len = strlen(ceil);\n \n-\t\tif (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))\n-\t\t\tcontinue;\n-\t\tmemcpy(buf, ceil, len+1);\n-\t\tif (normalize_path_copy(buf, buf) < 0)\n-\t\t\tcontinue;\n-\t\tlen = strlen(buf);\n-\t\tif (len > 0 && buf[len-1] == '/')\n-\t\t\tbuf[--len] = '\\0';\n-\n-\t\tif (!strncmp(path, buf, len) &&\n+\t\tif (!strncmp(path, ceil, len) &&\n \t\t path[len] == '/' &&\n \t\t len > max_len) {\n \t\t\tmax_len = len;\n-- \n1.7.11.3\n"},{"id":"199968","messageId":"1348688090-13648-6-git-send-email-mhagger@alum.mit.edu","threadId":"31082","inReplyTo":"1348688090-13648-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 5/8] longest_ancestor_length(): always add a slash to the end of prefixes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-26T19:34:47Z","receivedAt":"2012-09-26T19:34:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Previously we stripped off any slashes that were present. Instead,\nadd a slash if it is missing. This removes the need for an extra\ncheck that path has a slash following the prefix and makes the\nhandling of the root directory more natural, making the way clear to\nuse string_list_longest_prefix().\n\nSigned-off-by: Michael Haggerty \n---\n path.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 6da7029..90ef53d 100644\n--- a/path.c\n+++ b/path.c\n@@ -570,7 +570,7 @@ int normalize_path_copy(char *dst, const char *src)\n \n static int normalize_path_callback(struct string_list_item *item, void *cb_data)\n {\n-\tchar buf[PATH_MAX+1];\n+\tchar buf[PATH_MAX+2];\n \tconst char *ceil = item->string;\n \tint len = strlen(ceil);\n \n@@ -580,8 +580,10 @@ static int normalize_path_callback(struct string_list_item *item, void *cb_data)\n \tif (normalize_path_copy(buf, buf) < 0)\n \t\treturn 0;\n \tlen = strlen(buf);\n-\tif (len > 0 && buf[len-1] == '/')\n-\t\tbuf[--len] = '\\0';\n+\tif (len == 0 || buf[len-1] != '/') {\n+\t\tbuf[len++] = '/';\n+\t\tbuf[len++] = '\\0';\n+\t}\n \tfree(item->string);\n \titem->string = xstrdup(buf);\n \treturn 1;\n@@ -616,9 +618,8 @@ int longest_ancestor_length(const char *path, const char *prefix_list)\n \t\tint len = strlen(ceil);\n \n \t\tif (!strncmp(path, ceil, len) &&\n-\t\t path[len] == '/' &&\n-\t\t len > max_len) {\n-\t\t\tmax_len = len;\n+\t\t len - 1 > max_len) {\n+\t\t\tmax_len = len - 1;\n \t\t}\n \t}\n \n-- \n1.7.11.3\n"},{"id":"199970","messageId":"1348688090-13648-7-git-send-email-mhagger@alum.mit.edu","threadId":"31082","inReplyTo":"1348688090-13648-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 6/8] longest_ancestor_length(): use string_list_longest_prefix()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-26T19:34:48Z","receivedAt":"2012-09-26T19:34:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Use string_list_longest_prefix() in the implementation of\nlongest_ancestor_length(), instead of an equivalent loop.\n\nSigned-off-by: Michael Haggerty \n---\n path.c | 15 ++++-----------\n 1 file changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 90ef53d..5cace83 100644\n--- a/path.c\n+++ b/path.c\n@@ -605,23 +605,16 @@ static int normalize_path_callback(struct string_list_item *item, void *cb_data)\n int longest_ancestor_length(const char *path, const char *prefix_list)\n {\n \tstruct string_list prefixes = STRING_LIST_INIT_DUP;\n-\tint i, max_len = -1;\n+\tint max_len;\n+\tconst char *longest_prefix;\n \n \tif (prefix_list == NULL || !strcmp(path, \"/\"))\n \t\treturn -1;\n \n \tstring_list_split(&prefixes, prefix_list, PATH_SEP, -1);\n \tfilter_string_list(&prefixes, 0, normalize_path_callback, NULL);\n-\n-\tfor (i = 0; i < prefixes.nr; i++) {\n-\t\tconst char *ceil = prefixes.items[i].string;\n-\t\tint len = strlen(ceil);\n-\n-\t\tif (!strncmp(path, ceil, len) &&\n-\t\t len - 1 > max_len) {\n-\t\t\tmax_len = len - 1;\n-\t\t}\n-\t}\n+\tlongest_prefix = string_list_longest_prefix(&prefixes, path);\n+\tmax_len = longest_prefix ? strlen(longest_prefix) - 1 : -1;\n \n \tstring_list_clear(&prefixes, 0);\n \treturn max_len;\n-- \n1.7.11.3\n"},{"id":"199969","messageId":"1348688090-13648-8-git-send-email-mhagger@alum.mit.edu","threadId":"31082","inReplyTo":"1348688090-13648-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 7/8] longest_ancestor_length(): resolve symlinks before comparing paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-26T19:34:49Z","receivedAt":"2012-09-26T19:34:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"longest_ancestor_length() relies on a textual comparison of directory\nparts to find the part of path that overlaps with one of the paths in\nprefix_list. But this doesn't work if any of the prefixes involves a\nsymbolic link, because the directories will look different even though\nthey might logically refer to the same directory. So canonicalize the\npaths listed in prefix_list using real_path_if_valid() before trying\nto find matches.\n\npath is already in canonical form, so doesn't need to be canonicalized\nagain.\n\nThis fixes some problems with using GIT_CEILING_DIRECTORIES that\ncontains paths involving symlinks, including t4035 if run with --root\nset to a path involving symlinks.\n\nRemove a number of tests of longest_ancestor_length(). It is awkward\nto test longest_ancestor_length() now, because its new path\nnormalization behavior depends on the contents of the whole\nfilesystem. But we can live without the tests, because\nlongest_ancestor_length() is now built of reusable components that are\nthemselves tested separately: string_list_split(),\nstring_list_longest_prefix(), and real_path_if_valid().\n\nSigned-off-by: Michael Haggerty \n---\n path.c | 17 ++++++++------\n t/t0060-path-utils.sh | 64 ---------------------------------------------------\n 2 files changed, 10 insertions(+), 71 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 5cace83..981bb06 100644\n--- a/path.c\n+++ b/path.c\n@@ -570,22 +570,25 @@ int normalize_path_copy(char *dst, const char *src)\n \n static int normalize_path_callback(struct string_list_item *item, void *cb_data)\n {\n-\tchar buf[PATH_MAX+2];\n+\tchar *buf;\n \tconst char *ceil = item->string;\n-\tint len = strlen(ceil);\n+\tconst char *realpath;\n+\tint len;\n \n-\tif (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))\n+\tif (!*ceil || !is_absolute_path(ceil))\n \t\treturn 0;\n-\tmemcpy(buf, ceil, len+1);\n-\tif (normalize_path_copy(buf, buf) < 0)\n+\trealpath = real_path_if_valid(ceil);\n+\tif (!realpath)\n \t\treturn 0;\n-\tlen = strlen(buf);\n+\tlen = strlen(realpath);\n+\tbuf = xmalloc(len + 2); /* Leave space for possible trailing slash */\n+\tstrcpy(buf, realpath);\n \tif (len == 0 || buf[len-1] != '/') {\n \t\tbuf[len++] = '/';\n \t\tbuf[len++] = '\\0';\n \t}\n \tfree(item->string);\n-\titem->string = xstrdup(buf);\n+\titem->string = buf;\n \treturn 1;\n }\n \ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 4ef2345..c97bbf2 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -12,28 +12,6 @@ norm_path() {\n \t\"test \\\"\\$(test-path-utils normalize_path_copy '$1')\\\" = '$2'\"\n }\n \n-# On Windows, we are using MSYS's bash, which mangles the paths.\n-# Absolute paths are anchored at the MSYS installation directory,\n-# which means that the path / accounts for this many characters:\n-rootoff=$(test-path-utils normalize_path_copy / | wc -c)\n-# Account for the trailing LF:\n-if test $rootoff = 2; then\n-\trootoff=\t# we are on Unix\n-else\n-\trootoff=$(($rootoff-1))\n-fi\n-\n-ancestor() {\n-\t# We do some math with the expected ancestor length.\n-\texpected=$3\n-\tif test -n \"$rootoff\" && test \"x$expected\" != x-1; then\n-\t\texpected=$(($expected+$rootoff))\n-\tfi\n-\ttest_expect_success \"longest ancestor: $1 $2 => $expected\" \\\n-\t\"actual=\\$(test-path-utils longest_ancestor_length '$1' '$2') &&\n-\t test \\\"\\$actual\\\" = '$expected'\"\n-}\n-\n # Absolute path tests must be skipped on Windows because due to path mangling\n # the test program never sees a POSIX-style absolute path\n case $(uname -s) in\n@@ -93,48 +71,6 @@ norm_path /d1/s1//../s2/../../d2 /d2 POSIX\n norm_path /d1/.../d2 /d1/.../d2 POSIX\n norm_path /d1/..././../d2 /d1/d2 POSIX\n \n-ancestor / \"\" -1\n-ancestor / / -1\n-ancestor /foo \"\" -1\n-ancestor /foo : -1\n-ancestor /foo ::. -1\n-ancestor /foo ::..:: -1\n-ancestor /foo / 0\n-ancestor /foo /fo -1\n-ancestor /foo /foo -1\n-ancestor /foo /foo/ -1\n-ancestor /foo /bar -1\n-ancestor /foo /bar/ -1\n-ancestor /foo /foo/bar -1\n-ancestor /foo /foo:/bar/ -1\n-ancestor /foo /foo/:/bar/ -1\n-ancestor /foo /foo::/bar/ -1\n-ancestor /foo /:/foo:/bar/ 0\n-ancestor /foo /foo:/:/bar/ 0\n-ancestor /foo /:/bar/:/foo 0\n-ancestor /foo/bar \"\" -1\n-ancestor /foo/bar / 0\n-ancestor /foo/bar /fo -1\n-ancestor /foo/bar foo -1\n-ancestor /foo/bar /foo 4\n-ancestor /foo/bar /foo/ 4\n-ancestor /foo/bar /foo/ba -1\n-ancestor /foo/bar /:/fo 0\n-ancestor /foo/bar /foo:/foo/ba 4\n-ancestor /foo/bar /bar -1\n-ancestor /foo/bar /bar/ -1\n-ancestor /foo/bar /fo: -1\n-ancestor /foo/bar :/fo -1\n-ancestor /foo/bar /foo:/bar/ 4\n-ancestor /foo/bar /:/foo:/bar/ 4\n-ancestor /foo/bar /foo:/:/bar/ 4\n-ancestor /foo/bar /:/bar/:/fo 0\n-ancestor /foo/bar /:/bar/ 0\n-ancestor /foo/bar .:/foo/. 4\n-ancestor /foo/bar .:/foo/.:.: 4\n-ancestor /foo/bar /foo/./:.:/bar 4\n-ancestor /foo/bar .:/bar -1\n-\n test_expect_success 'strip_path_suffix' '\n \ttest c:/msysgit = $(test-path-utils strip_path_suffix \\\n \t\tc:/msysgit/libexec//git-core libexec/git-core)\n-- \n1.7.11.3\n"},{"id":"199971","messageId":"1348688090-13648-9-git-send-email-mhagger@alum.mit.edu","threadId":"31082","inReplyTo":"1348688090-13648-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 8/8] t1504: stop resolving symlinks in GIT_CEILING_DIRECTORIES","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-26T19:34:50Z","receivedAt":"2012-09-26T19:34:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This test used to explicitly resolve symlinks in the paths derived\nfrom TRASH_DIRECTORY that were written to GIT_CEILING_DIRECTORIES,\nbecause the code handling GIT_CEILING_DIRECTORIES was confused by\nsymlinks. This is no longer necessary.\n\nSigned-off-by: Michael Haggerty \n---\n t/t1504-ceiling-dirs.sh | 67 ++++++++++++++++++++++++-------------------------\n 1 file changed, 33 insertions(+), 34 deletions(-)\n\ndiff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh\nindex cce87a5..a4a5202 100755\n--- a/t/t1504-ceiling-dirs.sh\n+++ b/t/t1504-ceiling-dirs.sh\n@@ -14,8 +14,7 @@ test_fail() {\n \t'\n }\n \n-TRASH_ROOT=\"$PWD\"\n-ROOT_PARENT=$(dirname \"$TRASH_ROOT\")\n+ROOT_PARENT=$(dirname \"$TRASH_DIRECTORY\")\n \n \n unset GIT_CEILING_DIRECTORIES\n@@ -32,16 +31,16 @@ test_prefix ceil_at_parent \"\"\n GIT_CEILING_DIRECTORIES=\"$ROOT_PARENT/\"\n test_prefix ceil_at_parent_slash \"\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY\"\n test_prefix ceil_at_trash \"\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/\"\n test_prefix ceil_at_trash_slash \"\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sub\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sub\"\n test_prefix ceil_at_sub \"\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sub/\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sub/\"\n test_prefix ceil_at_sub_slash \"\"\n \n \n@@ -56,55 +55,55 @@ export GIT_CEILING_DIRECTORIES\n GIT_CEILING_DIRECTORIES=\"\"\n test_prefix subdir_ceil_empty \"sub/dir/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY\"\n test_fail subdir_ceil_at_trash\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/\"\n test_fail subdir_ceil_at_trash_slash\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sub\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sub\"\n test_fail subdir_ceil_at_sub\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sub/\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sub/\"\n test_fail subdir_ceil_at_sub_slash\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sub/dir\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sub/dir\"\n test_prefix subdir_ceil_at_subdir \"sub/dir/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sub/dir/\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sub/dir/\"\n test_prefix subdir_ceil_at_subdir_slash \"sub/dir/\"\n \n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/su\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/su\"\n test_prefix subdir_ceil_at_su \"sub/dir/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/su/\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/su/\"\n test_prefix subdir_ceil_at_su_slash \"sub/dir/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sub/di\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sub/di\"\n test_prefix subdir_ceil_at_sub_di \"sub/dir/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sub/di\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sub/di\"\n test_prefix subdir_ceil_at_sub_di_slash \"sub/dir/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/subdi\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/subdi\"\n test_prefix subdir_ceil_at_subdi \"sub/dir/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/subdi\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/subdi\"\n test_prefix subdir_ceil_at_subdi_slash \"sub/dir/\"\n \n \n-GIT_CEILING_DIRECTORIES=\"/foo:$TRASH_ROOT/sub\"\n+GIT_CEILING_DIRECTORIES=\"/foo:$TRASH_DIRECTORY/sub\"\n test_fail second_of_two\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sub:/bar\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sub:/bar\"\n test_fail first_of_two\n \n-GIT_CEILING_DIRECTORIES=\"/foo:$TRASH_ROOT/sub:/bar\"\n+GIT_CEILING_DIRECTORIES=\"/foo:$TRASH_DIRECTORY/sub:/bar\"\n test_fail second_of_three\n \n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sub\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sub\"\n GIT_DIR=../../.git\n export GIT_DIR\n test_prefix git_dir_specified \"\"\n@@ -123,41 +122,41 @@ export GIT_CEILING_DIRECTORIES\n GIT_CEILING_DIRECTORIES=\"\"\n test_prefix sd_ceil_empty \"s/d/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY\"\n test_fail sd_ceil_at_trash\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/\"\n test_fail sd_ceil_at_trash_slash\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/s\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/s\"\n test_fail sd_ceil_at_s\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/s/\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/s/\"\n test_fail sd_ceil_at_s_slash\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/s/d\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/s/d\"\n test_prefix sd_ceil_at_sd \"s/d/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/s/d/\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/s/d/\"\n test_prefix sd_ceil_at_sd_slash \"s/d/\"\n \n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/su\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/su\"\n test_prefix sd_ceil_at_su \"s/d/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/su/\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/su/\"\n test_prefix sd_ceil_at_su_slash \"s/d/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/s/di\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/s/di\"\n test_prefix sd_ceil_at_s_di \"s/d/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/s/di\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/s/di\"\n test_prefix sd_ceil_at_s_di_slash \"s/d/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sdi\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sdi\"\n test_prefix sd_ceil_at_sdi \"s/d/\"\n \n-GIT_CEILING_DIRECTORIES=\"$TRASH_ROOT/sdi\"\n+GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/sdi\"\n test_prefix sd_ceil_at_sdi_slash \"s/d/\"\n \n \n-- \n1.7.11.3\n"},{"id":"200041","messageId":"7vy5jvp813.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"1348688090-13648-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-27T19:42:16Z","receivedAt":"2012-09-27T19:42:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This series fixes longest_ancestor_length() so that it works even if\n> prefix_list contains entries that involve symlinks. The basic goal of\n> the series is to call real_path() on each of the entries so that a\n> textual comparison of the potential prefix to the front of path\n> correctly decides whether the path is located inside of the entry.\n> But along the way some other things had to be changed:\n>\n> * real_path() die()s if the path passed to it is invalid, whereas it\n> is allowed for GIT_CEILING_DIRECTORIES to contain invalid paths. So\n> create a new function real_path_if_valid() that returns NULL for\n> invalid paths.\n>\n> * Changing longest_ancestor_length() to call real_path_if_valid()\n> would make the former very difficult to test (because the tests\n> would depend on the contents of the whole filesystem). Therefore,\n> rewrite longest_ancestor_length() in terms of functions\n> string_list_split(), string_list_longest_prefix(), and\n> real_path_if_valid() which are tested individually.\n>\n> The net results of these changes are that:\n>\n> 1. t1504 used to have to canonicalize TRASH_DIRECTORY to make itself\n> work even if the --root directory contains symlinks. This\n> canonicalization is no longer necessary (and has been removed).\n>\n> 2. t4035, which used to fail if the --root directory contained\n> symlinks, now works correctly in that situation.\n>\n> After this change, all tests pass if the --root directory does *not*\n> contain symlinks, but t9903 still fails if the --root directory\n> contains symlinks. I haven't analyzed the cause of t9903's failure,\n> but it does not appear to be related to the GIT_CEILING_DIRECTORIES\n> feature.\n\nI haven't read the actual patches yet, but the all of the above\nsounds sensible.\n\n> On the mailing list I suggested *purposely* inserting symlinks into\n> the \"trash directory.*\" paths to test symlink handling more\n> systematically. This patch series does *NOT* make that change.\n\nAnd that may be a sensible follow-up step once the dust settles.\n\nThanks.\n"},{"id":"200049","messageId":"7vk3vfp36g.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"1348688090-13648-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/8] Introduce new static function real_path_internal()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-27T21:27:03Z","receivedAt":"2012-09-27T21:27:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> @@ -54,20 +73,36 @@ const char *real_path(const char *path)\n> \t\t}\n> \n> \t\tif (*buf) {\n> -\t\t\tif (!*cwd && !getcwd(cwd, sizeof(cwd)))\n> -\t\t\t\tdie_errno (\"Could not get current working directory\");\n> +\t\t\tif (!*cwd && !getcwd(cwd, sizeof(cwd))) {\n> +\t\t\t\tif (die_on_error)\n> +\t\t\t\t\tdie_errno(\"Could not get current working directory\");\n> +\t\t\t\telse\n> +\t\t\t\t\tgoto error_out;\n> +\t\t\t}\n> \n> -\t\t\tif (chdir(buf))\n> -\t\t\t\tdie_errno (\"Could not switch to '%s'\", buf);\n> +\t\t\tif (chdir(buf)) {\n> +\t\t\t\tif (die_on_error)\n> +\t\t\t\t\tdie_errno(\"Could not switch to '%s'\", buf);\n> +\t\t\t\telse\n> +\t\t\t\t\tgoto error_out;\n> +\t\t\t}\n> +\t\t}\n\nThe patch makes sense, but while you are touching this code, I have\nto wonder if there is an easy way to tell, before entering the loop,\nif we have to chdir() around in the loop. That would allow us to\nhoist the getcwd() that is done only so that we can come back to\nwhere we started outside the loop, making it clear why the call is\nthere, instead of cryptic \"if (!*cwd &&\" to ensure we do getcwd()\nonce and before doing any chdir().\n"},{"id":"200055","messageId":"7vr4pnnkux.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"1348688090-13648-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 3/8] longest_ancestor_length(): use string_list_split()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-27T22:48:06Z","receivedAt":"2012-09-27T22:48:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> -\tfor (colon = ceil = prefix_list; *colon; ceil = colon+1) {\n> -\t\tfor (colon = ceil; *colon && *colon != PATH_SEP; colon++);\n> -\t\tlen = colon - ceil;\n> +\tstring_list_split(&prefixes, prefix_list, PATH_SEP, -1);\n> +\n> +\tfor (i = 0; i < prefixes.nr; i++) {\n> +\t\tconst char *ceil = prefixes.items[i].string;\n> +\t\tint len = strlen(ceil);\n> +\n\nMuch nicer than the yucky original ;-)\n\n> \t\tif (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))\n> \t\t\tcontinue;\n> -\t\tstrlcpy(buf, ceil, len+1);\n> +\t\tmemcpy(buf, ceil, len+1);\n> \t\tif (normalize_path_copy(buf, buf) < 0)\n> \t\t\tcontinue;\n\nWhy do you need this memcpy in the first place? Isn't ceil already\na NUL terminated string unlike the original code that points into a\npart of the prefix_list string? IOW, why not\n\n\tnormalize_path_copy(buf, ceil);\n\nor something?\n\nCan normalize_path_copy() overflow buf[PATH_MAX+1] here (before or\nafter this patch)?\n"},{"id":"200056","messageId":"7vmx0bnktn.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"1348688090-13648-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 4/8] longest_ancestor_length(): explicitly filter list before loop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-27T22:48:52Z","receivedAt":"2012-09-27T22:48:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Makes sense.\n"},{"id":"200057","messageId":"7vipaznkom.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"1348688090-13648-8-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 7/8] longest_ancestor_length(): resolve symlinks before comparing paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-27T22:51:53Z","receivedAt":"2012-09-27T22:51:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> longest_ancestor_length() relies on a textual comparison of directory\n> parts to find the part of path that overlaps with one of the paths in\n> prefix_list. But this doesn't work if any of the prefixes involves a\n> symbolic link, because the directories will look different even though\n> they might logically refer to the same directory. So canonicalize the\n> paths listed in prefix_list using real_path_if_valid() before trying\n> to find matches.\n>\n> path is already in canonical form, so doesn't need to be canonicalized\n> again.\n>\n> This fixes some problems with using GIT_CEILING_DIRECTORIES that\n> contains paths involving symlinks, including t4035 if run with --root\n> set to a path involving symlinks.\n>\n> Remove a number of tests of longest_ancestor_length(). It is awkward\n> to test longest_ancestor_length() now, because its new path\n> normalization behavior depends on the contents of the whole\n> filesystem. But we can live without the tests, because\n> longest_ancestor_length() is now built of reusable components that are\n> themselves tested separately: string_list_split(),\n> string_list_longest_prefix(), and real_path_if_valid().\n\nErrr, components may be correct but the way to combine and construct\ncould go faulty, so...\n\n> Signed-off-by: Michael Haggerty \n> ---\n> path.c | 17 ++++++++------\n> t/t0060-path-utils.sh | 64 ---------------------------------------------------\n> 2 files changed, 10 insertions(+), 71 deletions(-)\n>\n> diff --git a/path.c b/path.c\n> index 5cace83..981bb06 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -570,22 +570,25 @@ int normalize_path_copy(char *dst, const char *src)\n> \n> static int normalize_path_callback(struct string_list_item *item, void *cb_data)\n> {\n> -\tchar buf[PATH_MAX+2];\n> +\tchar *buf;\n> \tconst char *ceil = item->string;\n> -\tint len = strlen(ceil);\n> +\tconst char *realpath;\n> +\tint len;\n> \n> -\tif (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))\n> +\tif (!*ceil || !is_absolute_path(ceil))\n> \t\treturn 0;\n> -\tmemcpy(buf, ceil, len+1);\n> -\tif (normalize_path_copy(buf, buf) < 0)\n> +\trealpath = real_path_if_valid(ceil);\n> +\tif (!realpath)\n> \t\treturn 0;\n> -\tlen = strlen(buf);\n> +\tlen = strlen(realpath);\n> +\tbuf = xmalloc(len + 2); /* Leave space for possible trailing slash */\n> +\tstrcpy(buf, realpath);\n> \tif (len == 0 || buf[len-1] != '/') {\n> \t\tbuf[len++] = '/';\n> \t\tbuf[len++] = '\\0';\n> \t}\n\nNice.\n"},{"id":"200149","messageId":"50667F7E.7040903@alum.mit.edu","threadId":"31082","inReplyTo":"7vk3vfp36g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/8] Introduce new static function real_path_internal()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T04:56:30Z","receivedAt":"2012-09-29T04:56:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/27/2012 11:27 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> @@ -54,20 +73,36 @@ const char *real_path(const char *path)\n>> \t\t}\n>> \n>> \t\tif (*buf) {\n>> -\t\t\tif (!*cwd && !getcwd(cwd, sizeof(cwd)))\n>> -\t\t\t\tdie_errno (\"Could not get current working directory\");\n>> +\t\t\tif (!*cwd && !getcwd(cwd, sizeof(cwd))) {\n>> +\t\t\t\tif (die_on_error)\n>> +\t\t\t\t\tdie_errno(\"Could not get current working directory\");\n>> +\t\t\t\telse\n>> +\t\t\t\t\tgoto error_out;\n>> +\t\t\t}\n>> \n>> -\t\t\tif (chdir(buf))\n>> -\t\t\t\tdie_errno (\"Could not switch to '%s'\", buf);\n>> +\t\t\tif (chdir(buf)) {\n>> +\t\t\t\tif (die_on_error)\n>> +\t\t\t\t\tdie_errno(\"Could not switch to '%s'\", buf);\n>> +\t\t\t\telse\n>> +\t\t\t\t\tgoto error_out;\n>> +\t\t\t}\n>> +\t\t}\n> \n> The patch makes sense, but while you are touching this code, I have\n> to wonder if there is an easy way to tell, before entering the loop,\n> if we have to chdir() around in the loop. That would allow us to\n> hoist the getcwd() that is done only so that we can come back to\n> where we started outside the loop, making it clear why the call is\n> there, instead of cryptic \"if (!*cwd &&\" to ensure we do getcwd()\n> once and before doing any chdir().\n\nI don't see an easy way to predict, before entering the loop, whether\nchdir() will be needed. For example, compare\n\n touch filename\n ln -s filename foo\n ./test-path-utils real_path foo\n\nwith\n\n touch filename\n ln -s $(pwd)/filename foo\n ./test-path-utils real_path foo\n\nIn the first case no chdir() is needed, whereas in the second case a\nchdir() is needed but only on the second loop iteration.\n\nAll I can offer you is a palliative like the one below.\n\nMichael\n\ndiff --git a/abspath.c b/abspath.c\nindex 5748b91..40cdc46 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -35,7 +35,14 @@ static const char *real_path_internal(const char\n*path, int die_on_error)\n {\n static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf =\nbufs[1];\n char *retval = NULL;\n+\n+ /*\n+ * If we have to temporarily chdir(), store the original CWD\n+ * here so that we can chdir() back to it at the end of the\n+ * function:\n+ */\n char cwd[1024] = \"\";\n+\n int buf_index = 1;\n\n int depth = MAXDEPTH;\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"200151","messageId":"5066863E.1030005@alum.mit.edu","threadId":"31082","inReplyTo":"7vr4pnnkux.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/8] longest_ancestor_length(): use string_list_split()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T05:25:18Z","receivedAt":"2012-09-29T05:25:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/28/2012 12:48 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> -\tfor (colon = ceil = prefix_list; *colon; ceil = colon+1) {\n>> -\t\tfor (colon = ceil; *colon && *colon != PATH_SEP; colon++);\n>> -\t\tlen = colon - ceil;\n>> +\tstring_list_split(&prefixes, prefix_list, PATH_SEP, -1);\n>> +\n>> +\tfor (i = 0; i < prefixes.nr; i++) {\n>> +\t\tconst char *ceil = prefixes.items[i].string;\n>> +\t\tint len = strlen(ceil);\n>> +\n> \n> Much nicer than the yucky original ;-)\n\nIf your winky-smiley implies irony, then I would like to object. Even\nthough the original is not difficult to understand, it is more difficult\nto review than the new version because one has to think about off-by-one\nerrors etc. The new version has a bit more boilerplate, but a quick\nread suffices both to understand it and to see that it is correct.\nThough of course I admit that the improvement is small.\n\nBut the main point of this change is to move towards using more testable\nparts, so it is a step forward regardless of whether it is more readable.\n\n>> \t\tif (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))\n>> \t\t\tcontinue;\n>> -\t\tstrlcpy(buf, ceil, len+1);\n>> +\t\tmemcpy(buf, ceil, len+1);\n>> \t\tif (normalize_path_copy(buf, buf) < 0)\n>> \t\t\tcontinue;\n> \n> Why do you need this memcpy in the first place? Isn't ceil already\n> a NUL terminated string unlike the original code that points into a\n> part of the prefix_list string? IOW, why not\n> \n> \tnormalize_path_copy(buf, ceil);\n> \n> or something?\n\nGood point. I will fix this in v2.\n\n> Can normalize_path_copy() overflow buf[PATH_MAX+1] here (before or\n> after this patch)?\n\nnormalize_path_copy() can only shrink paths, not grow them. So the\nlength check on ceil guarantees that the result of normalize_path_copy()\nwill fit in buf.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"200154","messageId":"7vy5jtidyd.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"50667F7E.7040903@alum.mit.edu","subject":"Re: [PATCH 1/8] Introduce new static function real_path_internal()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-29T05:40:42Z","receivedAt":"2012-09-29T05:40:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>> The patch makes sense, but while you are touching this code, I have\n>> to wonder if there is an easy way to tell, before entering the loop,\n>> if we have to chdir() around in the loop. That would allow us to\n>> hoist the getcwd() that is done only so that we can come back to\n>> where we started outside the loop, making it clear why the call is\n>> there, instead of cryptic \"if (!*cwd &&\" to ensure we do getcwd()\n>> once and before doing any chdir().\n>\n> I don't see an easy way to predict, before entering the loop, whether\n> chdir() will be needed. For example, compare\n>\n> touch filename\n> ln -s filename foo\n> ./test-path-utils real_path foo\n>\n> with\n>\n> touch filename\n> ln -s $(pwd)/filename foo\n> ./test-path-utils real_path foo\n>\n> In the first case no chdir() is needed, whereas in the second case a\n> chdir() is needed but only on the second loop iteration.\n\nThanks for an example, and it is perfectly OK if we really have to\nhave that \"only fires once and only if needed\" logic in the loop.\n\n> All I can offer you is a palliative like the one below.\n\nI think that is an improvement; or we could rename it not cwd[] but\nsome other name that includes the word \"original\" in it.\n\nThanks.\n\n> Michael\n>\n> diff --git a/abspath.c b/abspath.c\n> index 5748b91..40cdc46 100644\n> --- a/abspath.c\n> +++ b/abspath.c\n> @@ -35,7 +35,14 @@ static const char *real_path_internal(const char\n> *path, int die_on_error)\n> {\n> static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf =\n> bufs[1];\n> char *retval = NULL;\n> +\n> + /*\n> + * If we have to temporarily chdir(), store the original CWD\n> + * here so that we can chdir() back to it at the end of the\n> + * function:\n> + */\n> char cwd[1024] = \"\";\n> +\n> int buf_index = 1;\n>\n> int depth = MAXDEPTH;\n"},{"id":"200155","messageId":"7vtxuhidtp.fsf@alter.siamese.dyndns.org","threadId":"31082","inReplyTo":"5066863E.1030005@alum.mit.edu","subject":"Re: [PATCH 3/8] longest_ancestor_length(): use string_list_split()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-29T05:43:30Z","receivedAt":"2012-09-29T05:43:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 09/28/2012 12:48 AM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>> \n>>> -\tfor (colon = ceil = prefix_list; *colon; ceil = colon+1) {\n>>> -\t\tfor (colon = ceil; *colon && *colon != PATH_SEP; colon++);\n>>> -\t\tlen = colon - ceil;\n>>> +\tstring_list_split(&prefixes, prefix_list, PATH_SEP, -1);\n>>> +\n>>> +\tfor (i = 0; i < prefixes.nr; i++) {\n>>> +\t\tconst char *ceil = prefixes.items[i].string;\n>>> +\t\tint len = strlen(ceil);\n>>> +\n>> \n>> Much nicer than the yucky original ;-)\n>\n> If your winky-smiley implies irony, then I would like to object.\n\nNo irony. The original was hard to read, especially with the for\nloop that does in-line strchr() on a single line. The updated one\nis much easier to read.\n\n> normalize_path_copy() can only shrink paths, not grow them. So the\n> length check on ceil guarantees that the result of normalize_path_copy()\n> will fit in buf.\n\nOK.\n\nThanks.\n"},{"id":"200156","messageId":"50668B36.8020708@alum.mit.edu","threadId":"31082","inReplyTo":"7vipaznkom.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/8] longest_ancestor_length(): resolve symlinks before comparing paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T05:46:30Z","receivedAt":"2012-09-29T05:46:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/28/2012 12:51 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> longest_ancestor_length() relies on a textual comparison of directory\n>> parts to find the part of path that overlaps with one of the paths in\n>> prefix_list. But this doesn't work if any of the prefixes involves a\n>> symbolic link, because the directories will look different even though\n>> they might logically refer to the same directory. So canonicalize the\n>> paths listed in prefix_list using real_path_if_valid() before trying\n>> to find matches.\n>>\n>> path is already in canonical form, so doesn't need to be canonicalized\n>> again.\n>>\n>> This fixes some problems with using GIT_CEILING_DIRECTORIES that\n>> contains paths involving symlinks, including t4035 if run with --root\n>> set to a path involving symlinks.\n>>\n>> Remove a number of tests of longest_ancestor_length(). It is awkward\n>> to test longest_ancestor_length() now, because its new path\n>> normalization behavior depends on the contents of the whole\n>> filesystem. But we can live without the tests, because\n>> longest_ancestor_length() is now built of reusable components that are\n>> themselves tested separately: string_list_split(),\n>> string_list_longest_prefix(), and real_path_if_valid().\n> \n> Errr, components may be correct but the way to combine and construct\n> could go faulty, so...\n\nI don't see a realistic alternative. Testing real_path() is itself is\nalready quite awkward (see t0060), so testing longest_ancestor_length()\nwould be even more so. Of course, the GIT_CEILING_DIRECTORIES tests\nindirectly test longest_ancestor_length(), though not systematically.\n\nIf you have a better suggestion, please let me know.\n\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> path.c | 17 ++++++++------\n>> t/t0060-path-utils.sh | 64 ---------------------------------------------------\n>> 2 files changed, 10 insertions(+), 71 deletions(-)\n>>\n>> diff --git a/path.c b/path.c\n>> index 5cace83..981bb06 100644\n>> --- a/path.c\n>> +++ b/path.c\n>> @@ -570,22 +570,25 @@ int normalize_path_copy(char *dst, const char *src)\n>> \n>> static int normalize_path_callback(struct string_list_item *item, void *cb_data)\n>> {\n>> -\tchar buf[PATH_MAX+2];\n>> +\tchar *buf;\n>> \tconst char *ceil = item->string;\n>> -\tint len = strlen(ceil);\n>> +\tconst char *realpath;\n>> +\tint len;\n>> \n>> -\tif (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))\n>> +\tif (!*ceil || !is_absolute_path(ceil))\n>> \t\treturn 0;\n>> -\tmemcpy(buf, ceil, len+1);\n>> -\tif (normalize_path_copy(buf, buf) < 0)\n>> +\trealpath = real_path_if_valid(ceil);\n>> +\tif (!realpath)\n>> \t\treturn 0;\n>> -\tlen = strlen(buf);\n>> +\tlen = strlen(realpath);\n>> +\tbuf = xmalloc(len + 2); /* Leave space for possible trailing slash */\n>> +\tstrcpy(buf, realpath);\n>> \tif (len == 0 || buf[len-1] != '/') {\n>> \t\tbuf[len++] = '/';\n>> \t\tbuf[len++] = '\\0';\n>> \t}\n> \n> Nice.\n\nI just noticed that the second \"len++\" in the final \"if\" is misleading.\n I will fix that in v2.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}