{"thread":{"id":"31421","subject":"[PATCH 2/7] absolute_path(): reject the empty string","startedAt":"2012-09-04T08:14:23Z","lastAt":"2012-09-09T05:27:42Z","messageCount":25,"participants":["mhagger@alum.mit.edu","Junio C Hamano","Johannes Sixt","Nguyen Thai Ngoc Duy","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"198282","messageId":"1346746470-23127-1-git-send-email-mhagger@alum.mit.edu","threadId":"31421","inReplyTo":null,"subject":"[PATCH 0/7] Fix some bugs in abspath.c","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-04T08:14:23Z","receivedAt":"2012-09-04T08:14:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nI really just wanted to tidy up filter_refs(), but I've been sucked\ninto a cascade of recursive yak shaving.  This is my first attempt to\npop the yak stack.\n\nI want to use real_path() for making the handling of\nGIT_CEILING_DIRECTORIES more robust, but I noticed that it is broken\nfor some cases that will be important in this context.  This patch\nseries adds some new tests of that function and fixes the ones that\nare broken, including a similar breakage in absolute_path().  It\napplies to the current master.\n\nPlease note that both absolute_path(\"\") and real_path(\"\") used to\nreturn the current directory followed by a slash.  I believe that this\nwas a bug, and that it is more appropriate for both functions to\nreject the empty string.  The evidence is as follows:\n\n* If this were intended behavior, presumably the return value would\n  *not* have a trailing slash.\n\n* I couldn't find any callers that appeared to depend on the old\n  behavior.\n\n* The test suite runs without errors after the change.\n\nBut I didn't do a thorough code review to ensure that no callers ever\nrely on the old behavior.  The most likely scenario would be if one of\nthese functions were used to parse something like $PATH, where the\nempty string denotes the current directory, but I didn't see any\ncallers that seemed to be doing anything like this.\n\nMichael Haggerty (7):\n  t0000: verify that absolute_path() fails if passed the empty string\n  absolute_path(): reject the empty string\n  t0000: verify that real_path() fails if passed the empty string\n  real_path(): reject the empty string\n  t0000: verify that real_path() works correctly with absolute paths\n  real_path(): properly handle nonexistent top-level paths\n  t0000: verify that real_path() removes extra slashes\n\n abspath.c        | 12 ++++++++++--\n t/t0000-basic.sh | 31 ++++++++++++++++++++++++++++++-\n 2 files changed, 40 insertions(+), 3 deletions(-)\n\n-- \n1.7.11.3\n"},{"id":"198281","messageId":"1346746470-23127-2-git-send-email-mhagger@alum.mit.edu","threadId":"31421","inReplyTo":"1346746470-23127-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/7] t0000: verify that absolute_path() fails if passed the empty string","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-04T08:14:24Z","receivedAt":"2012-09-04T08:14:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nIt doesn't, so mark the test as failing.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t0000-basic.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex ccb5435..7347fb8 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -450,6 +450,10 @@ test_expect_success 'update-index D/F conflict' '\n \ttest $numpath0 = 1\n '\n \n+test_expect_failure 'absolute path rejects the empty string' '\n+\ttest_must_fail test-path-utils absolute_path \"\"\n+'\n+\n test_expect_success SYMLINKS 'real path works as expected' '\n \tmkdir first &&\n \tln -s ../.git first/.git &&\n-- \n1.7.11.3\n"},{"id":"198279","messageId":"1346746470-23127-3-git-send-email-mhagger@alum.mit.edu","threadId":"31421","inReplyTo":"1346746470-23127-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/7] absolute_path(): reject the empty string","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-04T08:14:25Z","receivedAt":"2012-09-04T08:14:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n abspath.c        | 4 +++-\n t/t0000-basic.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex f04ac18..5d62430 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -123,7 +123,9 @@ const char *absolute_path(const char *path)\n {\n \tstatic char buf[PATH_MAX + 1];\n \n-\tif (is_absolute_path(path)) {\n+\tif (!*path) {\n+\t\tdie(\"The empty string is not a valid path\");\n+\t} else if (is_absolute_path(path)) {\n \t\tif (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)\n \t\t\tdie(\"Too long path: %.*s\", 60, path);\n \t} else {\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 7347fb8..5963a6c 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -450,7 +450,7 @@ test_expect_success 'update-index D/F conflict' '\n \ttest $numpath0 = 1\n '\n \n-test_expect_failure 'absolute path rejects the empty string' '\n+test_expect_success 'absolute path rejects the empty string' '\n \ttest_must_fail test-path-utils absolute_path \"\"\n '\n \n-- \n1.7.11.3\n"},{"id":"198280","messageId":"1346746470-23127-4-git-send-email-mhagger@alum.mit.edu","threadId":"31421","inReplyTo":"1346746470-23127-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/7] t0000: verify that real_path() fails if passed the empty string","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-04T08:14:26Z","receivedAt":"2012-09-04T08:14:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nIt doesn't, so mark the test as failing.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t0000-basic.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 5963a6c..363e190 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -454,6 +454,10 @@ test_expect_success 'absolute path rejects the empty string' '\n \ttest_must_fail test-path-utils absolute_path \"\"\n '\n \n+test_expect_failure 'real path rejects the empty string' '\n+\ttest_must_fail test-path-utils real_path \"\"\n+'\n+\n test_expect_success SYMLINKS 'real path works as expected' '\n \tmkdir first &&\n \tln -s ../.git first/.git &&\n-- \n1.7.11.3\n"},{"id":"198283","messageId":"1346746470-23127-5-git-send-email-mhagger@alum.mit.edu","threadId":"31421","inReplyTo":"1346746470-23127-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/7] real_path(): reject the empty string","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-04T08:14:27Z","receivedAt":"2012-09-04T08:14:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n abspath.c        | 3 +++\n t/t0000-basic.sh | 2 +-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 5d62430..3e8325c 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -35,6 +35,9 @@ 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+\n \tif (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)\n \t\tdie (\"Too long path: %.*s\", 60, path);\n \ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 363e190..1a51634 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -454,7 +454,7 @@ test_expect_success 'absolute path rejects the empty string' '\n \ttest_must_fail test-path-utils absolute_path \"\"\n '\n \n-test_expect_failure 'real path rejects the empty string' '\n+test_expect_success 'real path rejects the empty string' '\n \ttest_must_fail test-path-utils real_path \"\"\n '\n \n-- \n1.7.11.3\n"},{"id":"198286","messageId":"1346746470-23127-6-git-send-email-mhagger@alum.mit.edu","threadId":"31421","inReplyTo":"1346746470-23127-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-04T08:14:28Z","receivedAt":"2012-09-04T08:14:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nThere is currently a bug: if passed an absolute top-level path that\ndoesn't exist (e.g., \"/foo\") it incorrectly interprets the path as a\nrelative path (e.g., returns \"$(pwd)/foo\").  So mark the test as\nfailing.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t0000-basic.sh | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 1a51634..ad002ee 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -458,7 +458,17 @@ test_expect_success 'real path rejects the empty string' '\n \ttest_must_fail test-path-utils real_path \"\"\n '\n \n-test_expect_success SYMLINKS 'real path works as expected' '\n+test_expect_failure 'real path works on absolute paths' '\n+\tnopath=\"hopefully-absent-path\" &&\n+\ttest \"/\" = \"$(test-path-utils real_path \"/\")\" &&\n+\ttest \"/$nopath\" = \"$(test-path-utils real_path \"/$nopath\")\" &&\n+\t# Find an existing top-level directory for the remaining tests:\n+\td=$(pwd -P | sed -e \"s|^\\(/[^/]*\\)/.*|\\1|\") &&\n+\ttest \"$d\" = \"$(test-path-utils real_path \"$d\")\" &&\n+\ttest \"$d/$nopath\" = \"$(test-path-utils real_path \"$d/$nopath\")\"\n+'\n+\n+test_expect_success SYMLINKS 'real path works on symlinks' '\n \tmkdir first &&\n \tln -s ../.git first/.git &&\n \tmkdir second &&\n-- \n1.7.11.3\n"},{"id":"198285","messageId":"1346746470-23127-7-git-send-email-mhagger@alum.mit.edu","threadId":"31421","inReplyTo":"1346746470-23127-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 6/7] real_path(): properly handle nonexistent top-level paths","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-04T08:14:29Z","receivedAt":"2012-09-04T08:14:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n abspath.c        | 5 ++++-\n t/t0000-basic.sh | 2 +-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 3e8325c..0e1cd7f 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -45,8 +45,11 @@ const char *real_path(const char *path)\n \t\tif (!is_directory(buf)) {\n \t\t\tchar *last_slash = find_last_dir_sep(buf);\n \t\t\tif (last_slash) {\n-\t\t\t\t*last_slash = '\\0';\n \t\t\t\tlast_elem = xstrdup(last_slash + 1);\n+\t\t\t\tif (last_slash == buf)\n+\t\t\t\t\tlast_slash[1] = '\\0';\n+\t\t\t\telse\n+\t\t\t\t\tlast_slash[0] = '\\0';\n \t\t\t} else {\n \t\t\t\tlast_elem = xstrdup(buf);\n \t\t\t\t*buf = '\\0';\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex ad002ee..d929578 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -458,7 +458,7 @@ test_expect_success 'real path rejects the empty string' '\n \ttest_must_fail test-path-utils real_path \"\"\n '\n \n-test_expect_failure 'real path works on absolute paths' '\n+test_expect_success 'real path works on absolute paths' '\n \tnopath=\"hopefully-absent-path\" &&\n \ttest \"/\" = \"$(test-path-utils real_path \"/\")\" &&\n \ttest \"/$nopath\" = \"$(test-path-utils real_path \"/$nopath\")\" &&\n-- \n1.7.11.3\n"},{"id":"198284","messageId":"1346746470-23127-8-git-send-email-mhagger@alum.mit.edu","threadId":"31421","inReplyTo":"1346746470-23127-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 7/7] t0000: verify that real_path() removes extra slashes","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-04T08:14:30Z","receivedAt":"2012-09-04T08:14:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nThese tests already pass, but make sure they don't break in the\nfuture.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nIt would be great if somebody would check whether these tests pass on\nWindows, and if not, give me a tip about how to fix them.\n\n t/t0000-basic.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex d929578..3c75e97 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -468,6 +468,17 @@ test_expect_success 'real path works on absolute paths' '\n \ttest \"$d/$nopath\" = \"$(test-path-utils real_path \"$d/$nopath\")\"\n '\n \n+test_expect_success 'real path removes extra slashes' '\n+\tnopath=\"hopefully-absent-path\" &&\n+\ttest \"/\" = \"$(test-path-utils real_path \"///\")\" &&\n+\ttest \"/$nopath\" = \"$(test-path-utils real_path \"///$nopath\")\" &&\n+\t# We need an existing top-level directory to use in the\n+\t# remaining tests.  Use the top-level ancestor of $(pwd):\n+\td=$(pwd -P | sed -e \"s|^\\(/[^/]*\\)/.*|\\1|\") &&\n+\ttest \"$d\" = \"$(test-path-utils real_path \"//$d///\")\" &&\n+\ttest \"$d/$nopath\" = \"$(test-path-utils real_path \"//$d///$nopath\")\"\n+'\n+\n test_expect_success SYMLINKS 'real path works on symlinks' '\n \tmkdir first &&\n \tln -s ../.git first/.git &&\n-- \n1.7.11.3\n"},{"id":"198317","messageId":"7va9x5bqz4.fsf@alter.siamese.dyndns.org","threadId":"31421","inReplyTo":"1346746470-23127-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/7] Fix some bugs in abspath.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-04T18:08:47Z","receivedAt":"2012-09-04T18:08:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mhagger@alum.mit.edu writes:\n\n> From: Michael Haggerty <mhagger@alum.mit.edu>\n>\n> I really just wanted to tidy up filter_refs(), but I've been sucked\n> into a cascade of recursive yak shaving.  This is my first attempt to\n> pop the yak stack.\n\nThanks.\n\n> Please note that both absolute_path(\"\") and real_path(\"\") used to\n> return the current directory followed by a slash.  I believe that this\n> was a bug, and that it is more appropriate for both functions to\n> reject the empty string.  The evidence is as follows:\n>\n> * If this were intended behavior, presumably the return value would\n>   *not* have a trailing slash.\n\nThat is weak.  The only thing you can infer from that observation is\nthat the presense or absense of trailing '/' would not make any\ndifference to the caller who wanted a path to the cwd (and is more\nconvenient if the call is made so that a path relative to the cwd is\ntucked after it).\n\n> * I couldn't find any callers that appeared to depend on the old\n>   behavior.\n\nThat is a very good argument (especially if the audit were\nthorough).\n\nI would be tempted to say that we should die() on \"\" for now, cook\nthe result outside \"master\" for a few weeks while auditing the\ncallchains, and see if any of them complains.\n"},{"id":"198318","messageId":"7v1uihbqhf.fsf@alter.siamese.dyndns.org","threadId":"31421","inReplyTo":"1346746470-23127-8-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-04T18:19:24Z","receivedAt":"2012-09-04T18:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mhagger@alum.mit.edu writes:\n\n> From: Michael Haggerty <mhagger@alum.mit.edu>\n>\n> These tests already pass, but make sure they don't break in the\n> future.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>\n> It would be great if somebody would check whether these tests pass on\n> Windows, and if not, give me a tip about how to fix them.\n\nI think this (perhaps unwarranted) removal of the double leading\nslashes is the root cause of\n\n    http://thread.gmane.org/gmane.comp.version-control.git/204469\n\n(people involved in that thread Cc'ed).\n\n>  t/t0000-basic.sh | 11 +++++++++++\n>  1 file changed, 11 insertions(+)\n>\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index d929578..3c75e97 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -468,6 +468,17 @@ test_expect_success 'real path works on absolute paths' '\n>  \ttest \"$d/$nopath\" = \"$(test-path-utils real_path \"$d/$nopath\")\"\n>  '\n>  \n> +test_expect_success 'real path removes extra slashes' '\n> +\tnopath=\"hopefully-absent-path\" &&\n> +\ttest \"/\" = \"$(test-path-utils real_path \"///\")\" &&\n> +\ttest \"/$nopath\" = \"$(test-path-utils real_path \"///$nopath\")\" &&\n> +\t# We need an existing top-level directory to use in the\n> +\t# remaining tests.  Use the top-level ancestor of $(pwd):\n> +\td=$(pwd -P | sed -e \"s|^\\(/[^/]*\\)/.*|\\1|\") &&\n> +\ttest \"$d\" = \"$(test-path-utils real_path \"//$d///\")\" &&\n> +\ttest \"$d/$nopath\" = \"$(test-path-utils real_path \"//$d///$nopath\")\"\n> +'\n> +\n>  test_expect_success SYMLINKS 'real path works on symlinks' '\n>  \tmkdir first &&\n>  \tln -s ../.git first/.git &&\n"},{"id":"198323","messageId":"7vehmha9uw.fsf@alter.siamese.dyndns.org","threadId":"31421","inReplyTo":"7va9x5bqz4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/7] Fix some bugs in abspath.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-04T19:03:51Z","receivedAt":"2012-09-04T19:03:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> mhagger@alum.mit.edu writes:\n>\n>> Please note that both absolute_path(\"\") and real_path(\"\") used to\n>> return the current directory followed by a slash.  I believe that this\n>> was a bug, and that it is more appropriate for both functions to\n>> reject the empty string.  The evidence is as follows:\n>>\n>> * If this were intended behavior, presumably the return value would\n>>   *not* have a trailing slash.\n>\n> That is weak.  The only thing you can infer from that observation is\n> that the presense or absense of trailing '/' would not make any\n> difference to the caller who wanted a path to the cwd (and is more\n> convenient if the call is made so that a path relative to the cwd is\n> tucked after it).\n\nI still wonder why you want to reject \"\" as an input, as it could be\nargued that it is a valid way to say the current directory.\n\nWhat does realpath(3) do?\n\n    ... goes and looks ...\n\nThe realpath(3) wants an existing path, and \"\" naturally gives NOENT,\nso we cannot really draw parallel from it.\n\nOk, let's do this again.\n\n    $ ./test-path-utils absolute_path \"\"\n    /srv/git/git.git/\n    $ ./test-path-utils absolute_path \"foo\"\n    /srv/git/git.git/foo\n\nso a caller has to be prepared to see the returned path sometimes\nterminated with slash and sometimes without, to form an absolute\npath to the file \"bar\" in the directory it gives to these functions\n(i.e. \"/srv/git/git.git/bar\" vs \"/srv/git/git.git/foo/bar\").\n\nThat is a reasonably good sign that either nobody calls these\nfunctions with \"\" or existing callers are buggy and would not handle\nthat case well and need to be fixed anyway.\n\nSo I am fine with die() in your patch.\n\nThanks.\n\n>> * I couldn't find any callers that appeared to depend on the old\n>>   behavior.\n>\n> That is a very good argument (especially if the audit were\n> thorough).\n>\n> I would be tempted to say that we should die() on \"\" for now, cook\n> the result outside \"master\" for a few weeks while auditing the\n> callchains, and see if any of them complains.\n"},{"id":"198361","messageId":"50470FFB.2070204@viscovery.net","threadId":"31421","inReplyTo":"1346746470-23127-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-09-05T08:40:27Z","receivedAt":"2012-09-05T08:40:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/4/2012 10:14, schrieb mhagger@alum.mit.edu:\n> From: Michael Haggerty <mhagger@alum.mit.edu>\n> \n> There is currently a bug: if passed an absolute top-level path that\n> doesn't exist (e.g., \"/foo\") it incorrectly interprets the path as a\n> relative path (e.g., returns \"$(pwd)/foo\").  So mark the test as\n> failing.\n> \n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  t/t0000-basic.sh | 12 +++++++++++-\n>  1 file changed, 11 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index 1a51634..ad002ee 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -458,7 +458,17 @@ test_expect_success 'real path rejects the empty string' '\n>  \ttest_must_fail test-path-utils real_path \"\"\n>  '\n>  \n\nThese tests should really be in t0060-path-utils.sh.\n\n> -test_expect_success SYMLINKS 'real path works as expected' '\n> +test_expect_failure 'real path works on absolute paths' '\n> +\tnopath=\"hopefully-absent-path\" &&\n> +\ttest \"/\" = \"$(test-path-utils real_path \"/\")\" &&\n> +\ttest \"/$nopath\" = \"$(test-path-utils real_path \"/$nopath\")\" &&\n\nThese tests fail on Windows because test-path-utils operates on a\nDOS-style absolute path even if a POSIX style absolute path is passed as\nargument. The best thing would be to move this to a separate test that is\nprotected by a POSIX prerequisite (which is set in t0060).\n\n> +\t# Find an existing top-level directory for the remaining tests:\n> +\td=$(pwd -P | sed -e \"s|^\\(/[^/]*\\)/.*|\\1|\") &&\n\npwd -P actually expands to pwd -W on Windows, and produces a DOS-style\npath. I suggest to insert [^/]* to skip drive letter-colon like so:\n\n\td=$(pwd -P | sed -e \"s|^\\([^/]*/[^/]*\\)/.*|\\1|\") &&\n\n> +\ttest \"$d\" = \"$(test-path-utils real_path \"$d\")\" &&\n> +\ttest \"$d/$nopath\" = \"$(test-path-utils real_path \"$d/$nopath\")\"\n\nThen these tests pass.\n\n-- Hannes\n"},{"id":"198360","messageId":"50471003.9010207@viscovery.net","threadId":"31421","inReplyTo":"1346746470-23127-8-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-09-05T08:40:35Z","receivedAt":"2012-09-05T08:40:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/4/2012 10:14, schrieb mhagger@alum.mit.edu:\n> From: Michael Haggerty <mhagger@alum.mit.edu>\n> \n> These tests already pass, but make sure they don't break in the\n> future.\n> \n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n> \n> It would be great if somebody would check whether these tests pass on\n> Windows, and if not, give me a tip about how to fix them.\n> \n>  t/t0000-basic.sh | 11 +++++++++++\n>  1 file changed, 11 insertions(+)\n> \n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index d929578..3c75e97 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -468,6 +468,17 @@ test_expect_success 'real path works on absolute paths' '\n>  \ttest \"$d/$nopath\" = \"$(test-path-utils real_path \"$d/$nopath\")\"\n>  '\n>  \n> +test_expect_success 'real path removes extra slashes' '\n> +\tnopath=\"hopefully-absent-path\" &&\n> +\ttest \"/\" = \"$(test-path-utils real_path \"///\")\" &&\n> +\ttest \"/$nopath\" = \"$(test-path-utils real_path \"///$nopath\")\" &&\n\nSame here: test-path-utils operates on a DOS-style absolute path.\nFurthermore, as Junio pointed out elsewhere, it is desirable that slashes\n(dir-separators) at the beginning are not collapsed if there are exactly\ntwo of them. Three or more can be collapsed to a single slash.\n\n> +\t# We need an existing top-level directory to use in the\n> +\t# remaining tests.  Use the top-level ancestor of $(pwd):\n> +\td=$(pwd -P | sed -e \"s|^\\(/[^/]*\\)/.*|\\1|\") &&\n\nSame here: Account for the drive letter-colon.\n\n> +\ttest \"$d\" = \"$(test-path-utils real_path \"//$d///\")\" &&\n> +\ttest \"$d/$nopath\" = \"$(test-path-utils real_path \"//$d///$nopath\")\"\n\nSince $d is a DOS-style path on Windows, //$d means something entirely\ndifferent than $d. You should split the test in two: One test checks that\nslashes at the end or before $nopath are collapsed (this must work on\nWindows as well), and another test protected by POSIX prerequisite to\ncheck that slashes at the beginning are collapsed.\n\n-- Hannes\n"},{"id":"198368","messageId":"CACsJy8DAbp1uDsNFFk1g9tuEV1qMnM2DZtxOwp5H9_VE7VwO1g@mail.gmail.com","threadId":"31421","inReplyTo":"7v1uihbqhf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-09-05T10:52:10Z","receivedAt":"2012-09-05T10:52:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 5, 2012 at 1:19 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> mhagger@alum.mit.edu writes:\n>\n>> From: Michael Haggerty <mhagger@alum.mit.edu>\n>>\n>> These tests already pass, but make sure they don't break in the\n>> future.\n>>\n>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n>> ---\n>>\n>> It would be great if somebody would check whether these tests pass on\n>> Windows, and if not, give me a tip about how to fix them.\n>\n> I think this (perhaps unwarranted) removal of the double leading\n> slashes is the root cause of\n>\n>     http://thread.gmane.org/gmane.comp.version-control.git/204469\n>\n> (people involved in that thread Cc'ed).\n\nI gave up on that thread because I did not have a proper environment\nto further troubleshoot. Thanks Mike for giving me the clue about\nreal_path(). I traced link_alt_odb_entry() and it does this:\n\n\tif (!is_absolute_path(entry) && relative_base) {\n\t\tstrbuf_addstr(&pathbuf, real_path(relative_base));\n\t\tstrbuf_addch(&pathbuf, '/');\n\t}\n\tstrbuf_add(&pathbuf, entry, len);\n\tnormalize_path_copy(pathbuf.buf, pathbuf.buf);\n\nThe culprit might be normalize_path_copy (I don't have Windows to test\nwhether real_path() strips the leading slash in //server/somewhere). I\ntested normalize_path_copy() separately and it does strip leading\nslashes, so it needs to be fixed. Something like maybe:\n\ndiff --git a/path.c b/path.c\nindex 66acd24..ad2881c 100644\n--- a/path.c\n+++ b/path.c\n@@ -503,6 +503,10 @@ int normalize_path_copy(char *dst, const char *src)\n                *dst++ = *src++;\n                *dst++ = *src++;\n        }\n+#ifdef WIN32\n+       else if (src[0] == '/' && src[1] == '/')\n+               *dst++ = *src++;\n+#endif\n        dst0 = dst;\n\n        if (is_dir_sep(*src)) {\n\nI'm not suitable for following this up as I don't have environment to\ntest it. Maybe some msysgit guy want to step in?\n-- \nDuy\n"},{"id":"198371","messageId":"CACsJy8BmK64FB61LOH6gn-sW1dnq9CV0SoLXAUdcBCY=5z5aGQ@mail.gmail.com","threadId":"31421","inReplyTo":"CAGHpTB+LipLt3CgVt5O0s9xj5HHf9Y5z9QWEy+FKYH8s4O7Q2w@mail.gmail.com","subject":"Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-09-05T11:13:56Z","receivedAt":"2012-09-05T11:13:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 5, 2012 at 6:05 PM, Orgad and Raizel Shaneh\n<orgads@gmail.com> wrote:\n> This seems to fix it. Just change src[x] == '/' to is_dir_sep(src[x]).\n\nSo you are able to test the changes. Would you mind submitting a patch\nso other users benefit it as well?\n-- \nDuy\n\n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n"},{"id":"198412","messageId":"7v1uif7s1d.fsf@alter.siamese.dyndns.org","threadId":"31421","inReplyTo":"CACsJy8DAbp1uDsNFFk1g9tuEV1qMnM2DZtxOwp5H9_VE7VwO1g@mail.gmail.com","subject":"Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-06T03:23:58Z","receivedAt":"2012-09-06T03:23:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> diff --git a/path.c b/path.c\n> index 66acd24..ad2881c 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -503,6 +503,10 @@ int normalize_path_copy(char *dst, const char *src)\n>                 *dst++ = *src++;\n>                 *dst++ = *src++;\n>         }\n> +#ifdef WIN32\n> +       else if (src[0] == '/' && src[1] == '/')\n> +               *dst++ = *src++;\n> +#endif\n\nThe two-byte copy we see above the context is conditional on a nice\nabstraction \"has_dos_drive_prefix()\" so that we do not have to\nsuffer from these ugly ifdefs.  Could we do something similar?\n\n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n"},{"id":"198423","messageId":"20120906054407.GA25981@duynguyen-vnpc.dek-tpc.internal","threadId":"31421","inReplyTo":"7v1uif7s1d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-09-06T05:44:07Z","receivedAt":"2012-09-06T05:44:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 05, 2012 at 08:23:58PM -0700, Junio C Hamano wrote:\n> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n> \n> > diff --git a/path.c b/path.c\n> > index 66acd24..ad2881c 100644\n> > --- a/path.c\n> > +++ b/path.c\n> > @@ -503,6 +503,10 @@ int normalize_path_copy(char *dst, const char *src)\n> >                 *dst++ = *src++;\n> >                 *dst++ = *src++;\n> >         }\n> > +#ifdef WIN32\n> > +       else if (src[0] == '/' && src[1] == '/')\n> > +               *dst++ = *src++;\n> > +#endif\n> \n> The two-byte copy we see above the context is conditional on a nice\n> abstraction \"has_dos_drive_prefix()\" so that we do not have to\n> suffer from these ugly ifdefs.  Could we do something similar?\n\nJust an idea. We could unify \"[a-z]:\" and \"//host\" into \"dos root\"\nconcept. That would teach other code paths about UNC paths too.\n\nReplace has_dos_drive_prefix() with has_dos_root_prefix(). Because the\nroot component's length is not fixed, offset_1st_component() should be\nused instead of hard coding \"2\".\n\nSomething like this. Totally untested.\n\n-- 8< --\ndiff --git a/cache.h b/cache.h\nindex 67f28b4..7946489 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -711,7 +711,7 @@ extern char *expand_user_path(const char *path);\n const char *enter_repo(const char *path, int strict);\n static inline int is_absolute_path(const char *path)\n {\n-\treturn is_dir_sep(path[0]) || has_dos_drive_prefix(path);\n+\treturn is_dir_sep(path[0]) || has_dos_root_prefix(path);\n }\n int is_directory(const char *);\n const char *real_path(const char *path);\n@@ -721,7 +721,7 @@ int normalize_path_copy(char *dst, const char *src);\n int longest_ancestor_length(const char *path, const char *prefix_list);\n char *strip_path_suffix(const char *path, const char *suffix);\n int daemon_avoid_alias(const char *path);\n-int offset_1st_component(const char *path);\n+int offset_1st_component(const char *path, int keep_root);\n \n /* object replacement */\n #define READ_SHA1_FILE_REPLACE 1\ndiff --git a/compat/basename.c b/compat/basename.c\nindex d8f8a3c..178b60d 100644\n--- a/compat/basename.c\n+++ b/compat/basename.c\n@@ -5,8 +5,7 @@ char *gitbasename (char *path)\n {\n \tconst char *base;\n \t/* Skip over the disk name in MSDOS pathnames. */\n-\tif (has_dos_drive_prefix(path))\n-\t\tpath += 2;\n+\tpath += offset_1st_component(path, 0);\n \tfor (base = path; *path; path++) {\n \t\tif (is_dir_sep(*path))\n \t\t\tbase = path + 1;\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 61a6521..1ca3e19 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -302,7 +302,9 @@ int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format\n  * git specific compatibility\n  */\n \n-#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')\n+#define has_dos_root_prefix(path) \\\n+\t((is_dir_sep[(path)[0]] && is_dir_sep[(path)[1]]) \\\n+\t || (isalpha(*(path)) && (path)[1] == ':'))\n #define is_dir_sep(c) ((c) == '/' || (c) == '\\\\')\n static inline char *mingw_find_last_dir_sep(const char *path)\n {\ndiff --git a/connect.c b/connect.c\nindex 55a85ad..3d9f7fe 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -521,7 +521,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\tend = host;\n \n \tpath = strchr(end, c);\n-\tif (path && !has_dos_drive_prefix(end)) {\n+\tif (path && (!isalpha(*end) || end[1] != ':')) {\n \t\tif (c == ':') {\n \t\t\tprotocol = PROTO_SSH;\n \t\t\t*path++ = '\\0';\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 35b095e..c6656e2 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -216,8 +216,8 @@ extern char *gitbasename(char *);\n #define STRIP_EXTENSION \"\"\n #endif\n \n-#ifndef has_dos_drive_prefix\n-#define has_dos_drive_prefix(path) 0\n+#ifndef has_dos_root_prefix\n+#define has_dos_root_prefix(path) 0\n #endif\n \n #ifndef is_dir_sep\ndiff --git a/path.c b/path.c\nindex 66acd24..0e4e2d7 100644\n--- a/path.c\n+++ b/path.c\n@@ -498,11 +498,12 @@ const char *relative_path(const char *abs, const char *base)\n int normalize_path_copy(char *dst, const char *src)\n {\n \tchar *dst0;\n+\tint i, len;\n \n-\tif (has_dos_drive_prefix(src)) {\n+\tlen = offset_1st_component(src, 1);\n+\tfor (i = 0; i < len; i++)\n \t\t*dst++ = *src++;\n-\t\t*dst++ = *src++;\n-\t}\n+\n \tdst0 = dst;\n \n \tif (is_dir_sep(*src)) {\n@@ -702,9 +703,18 @@ int daemon_avoid_alias(const char *p)\n \t}\n }\n \n-int offset_1st_component(const char *path)\n+int offset_1st_component(const char *path, int keep_root)\n {\n-\tif (has_dos_drive_prefix(path))\n-\t\treturn 2 + is_dir_sep(path[2]);\n-\treturn is_dir_sep(path[0]);\n+\tif (has_dos_root_prefix(path)) {\n+\t\tif (path[1] == ':')\n+\t\t\treturn 2 + (keep_root ? 0 : is_dir_sep(path[2]));\n+\t\telse {\n+\t\t\tconst char *s = strchr(path + 2, '/');\n+\t\t\t/* //host is considered a \"drive\" */\n+\t\t\tif (s)\n+\t\t\t\treturn s - path + (keep_root ? 0 : 1);\n+\t\t}\n+\t}\n+\n+\treturn keep_root ? 0 : is_dir_sep(path[0]);\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex 2f8159f..d4d339a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -755,7 +755,7 @@ int verify_path(const char *path)\n {\n \tchar c;\n \n-\tif (has_dos_drive_prefix(path))\n+\tif (has_dos_root_prefix(path))\n \t\treturn 0;\n \n \tgoto inside;\ndiff --git a/setup.c b/setup.c\nindex 9139bee..e010cf8 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -26,7 +26,7 @@ static char *prefix_path_gently(const char *prefix, int len, const char *path)\n \t\tif (!work_tree)\n \t\t\tgoto error_out;\n \t\tlen = strlen(work_tree);\n-\t\troot_len = offset_1st_component(work_tree);\n+\t\troot_len = offset_1st_component(work_tree, 0);\n \t\ttotal = strlen(sanitized) + 1;\n \t\tif (strncmp(sanitized, work_tree, len) ||\n \t\t    (len > root_len && sanitized[len] != '\\0' && sanitized[len] != '/')) {\n@@ -587,7 +587,7 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi\n \tif (offset != len) {\n \t\tif (chdir(cwd))\n \t\t\tdie_errno(\"Cannot come back to cwd\");\n-\t\troot_len = offset_1st_component(cwd);\n+\t\troot_len = offset_1st_component(cwd, 0);\n \t\tcwd[offset > root_len ? offset : root_len] = '\\0';\n \t\tset_git_dir(cwd);\n \t}\n@@ -654,7 +654,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n \t\treturn setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);\n \n \tceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);\n-\tif (ceil_offset < 0 && has_dos_drive_prefix(cwd))\n+\tif (ceil_offset < 0 && has_dos_root_prefix(cwd))\n \t\tceil_offset = 1;\n \n \t/*\ndiff --git a/sha1_file.c b/sha1_file.c\nindex af5cfbd..21ce020 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -102,7 +102,7 @@ int mkdir_in_gitdir(const char *path)\n \n int safe_create_leading_directories(char *path)\n {\n-\tchar *pos = path + offset_1st_component(path);\n+\tchar *pos = path + offset_1st_component(path, 0);\n \tstruct stat st;\n \n \twhile (pos) {\ndiff --git a/transport.c b/transport.c\nindex 1811b50..5141e8f 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -869,7 +869,7 @@ static int is_local(const char *url)\n \tconst char *colon = strchr(url, ':');\n \tconst char *slash = strchr(url, '/');\n \treturn !colon || (slash && slash < colon) ||\n-\t\thas_dos_drive_prefix(url);\n+\t\thas_dos_root_prefix(url);\n }\n \n static int is_file(const char *url)\n-- 8< --\n\n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n"},{"id":"198463","messageId":"7vharb3vib.fsf@alter.siamese.dyndns.org","threadId":"31421","inReplyTo":"20120906054407.GA25981@duynguyen-vnpc.dek-tpc.internal","subject":"Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-06T17:34:52Z","receivedAt":"2012-09-06T17:34:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> Just an idea. We could unify \"[a-z]:\" and \"//host\" into \"dos root\"\n> concept. That would teach other code paths about UNC paths too.\n> ...\n> diff --git a/path.c b/path.c\n> index 66acd24..0e4e2d7 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -498,11 +498,12 @@ const char *relative_path(const char *abs, const char *base)\n>  int normalize_path_copy(char *dst, const char *src)\n>  {\n>  \tchar *dst0;\n> +\tint i, len;\n>  \n> -\tif (has_dos_drive_prefix(src)) {\n> +\tlen = offset_1st_component(src, 1);\n> +\tfor (i = 0; i < len; i++)\n>  \t\t*dst++ = *src++;\n> -\t\t*dst++ = *src++;\n> -\t}\n> +\n>  \tdst0 = dst;\n\nModulo that I suspect you could get rid of offset_1st_component()\naltogether and has_dos_drive_prefix() return the length of the \"d:\"\nor \"//d\" part (which needs to be copied literally regardless of the\n\"normalization\"), what you suggest feels like the right approach.\nWhy do you need the \"keep_root\" parameter and do things differently\ndepending on the setting by the way?  Wouldn't \"skip the root level\nwhen computing the offset of the first path component\" something the\ncaller can easily decide to do or not to do, and wouldn't it make\nthe semantics of the function cleaner and simpler by making it do\nonly one thing and one thing well?\n"},{"id":"198481","messageId":"50491199.8090306@alum.mit.edu","threadId":"31421","inReplyTo":"50470FFB.2070204@viscovery.net","subject":"Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-06T21:11:53Z","receivedAt":"2012-09-06T21:11:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/05/2012 10:40 AM, Johannes Sixt wrote:\n> Am 9/4/2012 10:14, schrieb mhagger@alum.mit.edu:\n>> From: Michael Haggerty <mhagger@alum.mit.edu>\n>>\n>> There is currently a bug: if passed an absolute top-level path that\n>> doesn't exist (e.g., \"/foo\") it incorrectly interprets the path as a\n>> relative path (e.g., returns \"$(pwd)/foo\").  So mark the test as\n>> failing.\n>>\n>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n>> ---\n>>  t/t0000-basic.sh | 12 +++++++++++-\n>>  1 file changed, 11 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n>> index 1a51634..ad002ee 100755\n>> --- a/t/t0000-basic.sh\n>> +++ b/t/t0000-basic.sh\n>> @@ -458,7 +458,17 @@ test_expect_success 'real path rejects the empty string' '\n>>  \ttest_must_fail test-path-utils real_path \"\"\n>>  '\n>>  \n> \n> These tests should really be in t0060-path-utils.sh.\n> \n>> -test_expect_success SYMLINKS 'real path works as expected' '\n>> +test_expect_failure 'real path works on absolute paths' '\n>> +\tnopath=\"hopefully-absent-path\" &&\n>> +\ttest \"/\" = \"$(test-path-utils real_path \"/\")\" &&\n>> +\ttest \"/$nopath\" = \"$(test-path-utils real_path \"/$nopath\")\" &&\n> \n> These tests fail on Windows because test-path-utils operates on a\n> DOS-style absolute path even if a POSIX style absolute path is passed as\n> argument. The best thing would be to move this to a separate test that is\n> protected by a POSIX prerequisite (which is set in t0060).\n> \n>> +\t# Find an existing top-level directory for the remaining tests:\n>> +\td=$(pwd -P | sed -e \"s|^\\(/[^/]*\\)/.*|\\1|\") &&\n> \n> pwd -P actually expands to pwd -W on Windows, and produces a DOS-style\n> path. I suggest to insert [^/]* to skip drive letter-colon like so:\n> \n> \td=$(pwd -P | sed -e \"s|^\\([^/]*/[^/]*\\)/.*|\\1|\") &&\n> \n>> +\ttest \"$d\" = \"$(test-path-utils real_path \"$d\")\" &&\n>> +\ttest \"$d/$nopath\" = \"$(test-path-utils real_path \"$d/$nopath\")\"\n> \n> Then these tests pass.\n\nThanks for the help.  They will be incorporated in v2.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198485","messageId":"5049241D.10604@alum.mit.edu","threadId":"31421","inReplyTo":"50471003.9010207@viscovery.net","subject":"Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-06T22:30:53Z","receivedAt":"2012-09-06T22:30:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/05/2012 10:40 AM, Johannes Sixt wrote:\n> Am 9/4/2012 10:14, schrieb mhagger@alum.mit.edu:\n>> From: Michael Haggerty <mhagger@alum.mit.edu>\n>>\n>> These tests already pass, but make sure they don't break in the\n>> future.\n>>\n>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n>> ---\n>>\n>> It would be great if somebody would check whether these tests pass on\n>> Windows, and if not, give me a tip about how to fix them.\n>>\n>>  t/t0000-basic.sh | 11 +++++++++++\n>>  1 file changed, 11 insertions(+)\n>>\n>> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n>> index d929578..3c75e97 100755\n>> --- a/t/t0000-basic.sh\n>> +++ b/t/t0000-basic.sh\n>> @@ -468,6 +468,17 @@ test_expect_success 'real path works on absolute paths' '\n>>  \ttest \"$d/$nopath\" = \"$(test-path-utils real_path \"$d/$nopath\")\"\n>>  '\n>>  \n>> +test_expect_success 'real path removes extra slashes' '\n>> +\tnopath=\"hopefully-absent-path\" &&\n>> +\ttest \"/\" = \"$(test-path-utils real_path \"///\")\" &&\n>> +\ttest \"/$nopath\" = \"$(test-path-utils real_path \"///$nopath\")\" &&\n> \n> Same here: test-path-utils operates on a DOS-style absolute path.\n\nACK.\n\n> Furthermore, as Junio pointed out elsewhere, it is desirable that slashes\n> (dir-separators) at the beginning are not collapsed if there are exactly\n> two of them. Three or more can be collapsed to a single slash.\n\nThe empirical behavior of real_path() on Linux is (assuming \"/tmp\"\nexists and \"/foo\" does not)\n\n                         Before my changes       After my changes\nreal_path(\"/tmp\")        /tmp                    /tmp\nreal_path(\"//tmp\")       /tmp                    /tmp\nreal_path(\"/foo\")        $(pwd)/foo              /foo\nreal_path(\"//foo\")       /foo                    /foo\n\nreal_path(\"/tmp/bar\")    /tmp/bar                /tmp/bar\nreal_path(\"//tmp/bar\")   /tmp/bar                /tmp/bar\nreal_path(\"/foo/bar\")    --dies--                --dies--\nreal_path(\"//foo/bar\")   --dies--                --dies--\n\nSo I think that my changes makes things strictly better (albeit probably\nnot perfect).\n\nreal_path() relies on chdir() / getcwd() to canonicalize the path,\nexcept for one case:\n\nIf the specified path is not itself a directory, then it strips off the\nlast component of the name, tries chdir() / getcwd() on the shortened\npath, then pastes the last component on again.  The stripping off is\ndone by find_last_dir_sep(), which literally just looks for the last '/'\n(or for Windows also '\\') in the string.  Please note that the pasting\nback is done using '/' as a separator.\n\nSo I think that the only problematic case is a path that only includes a\nsingle group of slashes, like \"//foo\" or maybe \"c:\\\\foo\", and also is\nnot is_directory() [1].  What should be the algorithm for such cases,\nand how should it depend on the platform?  (And does it really matter?\nTop-level Linux paths are usually directories.  Are Windows-style\nnetwork shares also considered directories in the sense of is_directory()?)\n\nI will make an easy improvement: not to remove *any* slashes when\nstripping the last path component from the end of the path (letting\nchdir() deal with them).  This results in the same results for Linux and\nperhaps more hope under Windows:\n\nOn Linux: \"//foo\" -> (chdir(\"//\"), \"foo\") -> (\"/\", \"foo\") -> \"/foo\"\n\nOn Windows: \"//foo\" -> (chdir(\"//\"), \"foo\") -> (?, \"foo\") -> ?\n(what is the result of chdir(\"//\") then getcwd()?)\n\nOn Windows: \"c:\\foo\" -> (chdir(\"c:\\\"), \"foo\") -> (?, \"foo\") -> ?\n(what is the result of chdir(\"c:\\\") then getcwd()?)\n\n>> +\t# We need an existing top-level directory to use in the\n>> +\t# remaining tests.  Use the top-level ancestor of $(pwd):\n>> +\td=$(pwd -P | sed -e \"s|^\\(/[^/]*\\)/.*|\\1|\") &&\n> \n> Same here: Account for the drive letter-colon.\n\nACK\n\n>> +\ttest \"$d\" = \"$(test-path-utils real_path \"//$d///\")\" &&\n>> +\ttest \"$d/$nopath\" = \"$(test-path-utils real_path \"//$d///$nopath\")\"\n> \n> Since $d is a DOS-style path on Windows, //$d means something entirely\n> different than $d. You should split the test in two: One test checks that\n> slashes at the end or before $nopath are collapsed (this must work on\n> Windows as well), and another test protected by POSIX prerequisite to\n> check that slashes at the beginning are collapsed.\n\nDone.\n\nThanks again for your help.\n\nMichael\n\n[1] If there is more than one group of slashes in the name, like\n\"//foo//bar\" or \"c:\\\\foo\\\\bar\" then:\n\n* All but the last groups of slashes are handled by\n  chdir(\"//foo/\")/getcwd() and presumably de-duplicated or not as\n  appropriate\n\n* The extras from the last group of slashes are trailing slashes in\n  the string passed to chdir() and are therefore removed.\n\nso everything should be OK.\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198495","messageId":"7v627q21hl.fsf@alter.siamese.dyndns.org","threadId":"31421","inReplyTo":"1346746470-23127-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-06T23:08:38Z","receivedAt":"2012-09-06T23:08:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mhagger@alum.mit.edu writes:\n\n> From: Michael Haggerty <mhagger@alum.mit.edu>\n>\n> There is currently a bug: if passed an absolute top-level path that\n> doesn't exist (e.g., \"/foo\") it incorrectly interprets the path as a\n> relative path (e.g., returns \"$(pwd)/foo\").  So mark the test as\n> failing.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  t/t0000-basic.sh | 12 +++++++++++-\n>  1 file changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index 1a51634..ad002ee 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -458,7 +458,17 @@ test_expect_success 'real path rejects the empty string' '\n>  \ttest_must_fail test-path-utils real_path \"\"\n>  '\n>  \n> -test_expect_success SYMLINKS 'real path works as expected' '\n> +test_expect_failure 'real path works on absolute paths' '\n> +\tnopath=\"hopefully-absent-path\" &&\n> +\ttest \"/\" = \"$(test-path-utils real_path \"/\")\" &&\n> +\ttest \"/$nopath\" = \"$(test-path-utils real_path \"/$nopath\")\" &&\n\nYou could perhaps do\n\n\tsfx=0 &&\n        while test -e \"/$nopath$sfx\"\n        do\n\t\tsfx=$(( $sfx + 1 ))\n\tdone && nopath=$nopath$sfx &&\n\ttest \"/$nopath\" = \"$(test-path-utils real_path \"/$nopath\")\" &&\n\nif you really cared.\n"},{"id":"198498","messageId":"CACsJy8AqMGo5+jPS9RHEjSwA-VrmO+WCM3Y2hPN9U8FRie4U-A@mail.gmail.com","threadId":"31421","inReplyTo":"7vharb3vib.fsf@alter.siamese.dyndns.org","subject":"Re: Re: [PATCH 7/7] t0000: verify that real_path() removes extra slashes","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-09-07T01:18:39Z","receivedAt":"2012-09-07T01:18:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Sep 7, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Modulo that I suspect you could get rid of offset_1st_component()\n> altogether and has_dos_drive_prefix() return the length of the \"d:\"\n> or \"//d\" part (which needs to be copied literally regardless of the\n> \"normalization\"), what you suggest feels like the right approach.\n> Why do you need the \"keep_root\" parameter and do things differently\n> depending on the setting by the way?\n\nThat's how offset_1st_component() originally works, root slash if\npresent is counted.\n\n> Wouldn't \"skip the root level\n> when computing the offset of the first path component\" something the\n> caller can easily decide to do or not to do, and wouldn't it make\n> the semantics of the function cleaner and simpler by making it do\n> only one thing and one thing well?\n\nYeah. I'll have a closer look later and see if we can simplify the function.\n-- \nDuy\n\n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n"},{"id":"198587","messageId":"504C1B8A.3000406@alum.mit.edu","threadId":"31421","inReplyTo":"7v627q21hl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-09T04:31:06Z","receivedAt":"2012-09-09T04:31:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/07/2012 01:08 AM, Junio C Hamano wrote:\n> mhagger@alum.mit.edu writes:\n> \n>> From: Michael Haggerty <mhagger@alum.mit.edu>\n>>\n>> There is currently a bug: if passed an absolute top-level path that\n>> doesn't exist (e.g., \"/foo\") it incorrectly interprets the path as a\n>> relative path (e.g., returns \"$(pwd)/foo\").  So mark the test as\n>> failing.\n>>\n>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n>> ---\n>>  t/t0000-basic.sh | 12 +++++++++++-\n>>  1 file changed, 11 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n>> index 1a51634..ad002ee 100755\n>> --- a/t/t0000-basic.sh\n>> +++ b/t/t0000-basic.sh\n>> @@ -458,7 +458,17 @@ test_expect_success 'real path rejects the empty string' '\n>>  \ttest_must_fail test-path-utils real_path \"\"\n>>  '\n>>  \n>> -test_expect_success SYMLINKS 'real path works as expected' '\n>> +test_expect_failure 'real path works on absolute paths' '\n>> +\tnopath=\"hopefully-absent-path\" &&\n>> +\ttest \"/\" = \"$(test-path-utils real_path \"/\")\" &&\n>> +\ttest \"/$nopath\" = \"$(test-path-utils real_path \"/$nopath\")\" &&\n> \n> You could perhaps do\n> \n> \tsfx=0 &&\n>         while test -e \"/$nopath$sfx\"\n>         do\n> \t\tsfx=$(( $sfx + 1 ))\n> \tdone && nopath=$nopath$sfx &&\n> \ttest \"/$nopath\" = \"$(test-path-utils real_path \"/$nopath\")\" &&\n> \n> if you really cared.\n\nThe possibility is obvious.  Are you advocating it?\n\nI considered that approach, but came to the opinion that it would be\noverkill that would only complicate the code for no real advantage,\ngiven that (1) I picked a name that is pretty implausible for an\nexisting file, (2) the test suite only reads the file, never writes it\n(so there is no risk that a copy from a previous run gets left behind),\n(3) it's only test suite code, and any failures would have minor\nconsequences.\n\nPlease let me know if you assess the situation differently.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"198593","messageId":"7vfw6ru7de.fsf@alter.siamese.dyndns.org","threadId":"31421","inReplyTo":"504C1B8A.3000406@alum.mit.edu","subject":"Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-09T04:50:53Z","receivedAt":"2012-09-09T04:50:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> The possibility is obvious.  Are you advocating it?\n>\n> I considered that approach, but came to the opinion that it would be\n> overkill that would only complicate the code for no real advantage,\n> given that (1) I picked a name that is pretty implausible for an\n> existing file, (2) the test suite only reads the file, never writes it\n> (so there is no risk that a copy from a previous run gets left behind),\n> (3) it's only test suite code, and any failures would have minor\n> consequences.\n\n(4) if it only runs once at the very beginning of the test and sets\na variable that is named prominently clear what it means and lives\nthroughout the test, then we do not even have to say \"hopefully\" and\nappear lazy and loose to the readers of the test who wonders what\nhappens when the path does exist; doing so will help reducing the\nnoise on the mailing list in the future.\n"},{"id":"198596","messageId":"7vvcfnsr3l.fsf@alter.siamese.dyndns.org","threadId":"31421","inReplyTo":"7vfw6ru7de.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-09T05:27:42Z","receivedAt":"2012-09-09T05:27:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> (4) if it only runs once at the very beginning of the test and sets\n> a variable that is named prominently clear what it means and lives\n> throughout the test, then we do not even have to say \"hopefully\" and\n> appear lazy and loose to the readers of the test who wonders what\n> happens when the path does exist; doing so will help reducing the\n> noise on the mailing list in the future.\n\nHaving said that, I really do not deeply care either way.  If you\nare rerollilng the series for other changes, I wouldn't shed tears\neven if I do not see any change to this part.\n"}]}