{"thread":{"id":"31673","subject":"[PATCH v2 0/9] Fix GIT_CEILING_DIRECTORIES that contain symlinks","startedAt":"2012-09-29T06:15:53Z","lastAt":"2012-10-08T18:23:44Z","messageCount":17,"participants":["Michael Haggerty","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":2,"patchTotal":9},"messages":[{"id":"200159","messageId":"1348899362-4057-1-git-send-email-mhagger@alum.mit.edu","threadId":"31673","inReplyTo":null,"subject":"[PATCH v2 0/9] Fix GIT_CEILING_DIRECTORIES that contain symlinks","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T06:15:53Z","receivedAt":"2012-09-29T06:15:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"v2 of the series, with the following changes from v1:\n\nPatch 2/9 is new.\n\nPatch 4/9: remove an unnecessary memcpy().  (This change doesn't\naffect the tip of the branch because the memcpy() was removed anyway\nin the second-to-last patch.)\n\nPatch 8/9: remove a superfluous \"len++\", and improve the excuses in\nthe commit message for removing the tests of\nlongest_ancestor_length().\n\nThanks to Junio for his helpful comments.\n\nMichael Haggerty (9):\n  Introduce new static function real_path_internal()\n  real_path_internal(): add comment explaining use of cwd\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               | 105 ++++++++++++++++++++++++++++++++++++++----------\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, 151 insertions(+), 140 deletions(-)\n\n-- \n1.7.11.3\n"},{"id":"200160","messageId":"1348899362-4057-2-git-send-email-mhagger@alum.mit.edu","threadId":"31673","inReplyTo":"1348899362-4057-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 1/9] Introduce new static function real_path_internal()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T06:15:54Z","receivedAt":"2012-09-29T06:15:54Z","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 <mhagger@alum.mit.edu>\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":"200161","messageId":"1348899362-4057-3-git-send-email-mhagger@alum.mit.edu","threadId":"31673","inReplyTo":"1348899362-4057-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 2/9] real_path_internal(): add comment explaining use of cwd","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T06:15:55Z","receivedAt":"2012-09-29T06:15:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n abspath.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/abspath.c b/abspath.c\nindex a7ab8e9..f8a526f 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -35,7 +35,14 @@ 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+\n+\t/*\n+\t * If we have to temporarily chdir(), store the original CWD\n+\t * here so that we can chdir() back to it at the end of the\n+\t * function:\n+\t */\n \tchar cwd[1024] = \"\";\n+\n \tint buf_index = 1;\n \n \tint depth = MAXDEPTH;\n-- \n1.7.11.3\n"},{"id":"200162","messageId":"1348899362-4057-4-git-send-email-mhagger@alum.mit.edu","threadId":"31673","inReplyTo":"1348899362-4057-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 3/9] Introduce new function real_path_if_valid()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T06:15:56Z","receivedAt":"2012-09-29T06:15:56Z","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 <mhagger@alum.mit.edu>\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 f8a526f..40cdc46 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -153,6 +153,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":"200163","messageId":"1348899362-4057-5-git-send-email-mhagger@alum.mit.edu","threadId":"31673","inReplyTo":"1348899362-4057-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 4/9] longest_ancestor_length(): use string_list_split()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T06:15:57Z","receivedAt":"2012-09-29T06:15:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n path.c | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex cbbdf7d..f455e8e 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,20 +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\tif (normalize_path_copy(buf, buf) < 0)\n+\t\tif (normalize_path_copy(buf, ceil) < 0)\n \t\t\tcontinue;\n \t\tlen = strlen(buf);\n \t\tif (len > 0 && buf[len-1] == '/')\n@@ -608,6 +611,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":"200164","messageId":"1348899362-4057-6-git-send-email-mhagger@alum.mit.edu","threadId":"31673","inReplyTo":"1348899362-4057-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 5/9] longest_ancestor_length(): explicitly filter list before loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T06:15:58Z","receivedAt":"2012-09-29T06:15:58Z","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 <mhagger@alum.mit.edu>\n---\n path.c | 30 ++++++++++++++++++++----------\n 1 file changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex f455e8e..b255a74 100644\n--- a/path.c\n+++ b/path.c\n@@ -568,6 +568,24 @@ 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+\tif (normalize_path_copy(buf, ceil) < 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,27 +602,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\tif (normalize_path_copy(buf, ceil) < 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":"200165","messageId":"1348899362-4057-7-git-send-email-mhagger@alum.mit.edu","threadId":"31673","inReplyTo":"1348899362-4057-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 6/9] longest_ancestor_length(): always add a slash to the end of prefixes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T06:15:59Z","receivedAt":"2012-09-29T06:15:59Z","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 <mhagger@alum.mit.edu>\n---\n path.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex b255a74..a56c64a 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@@ -579,8 +579,10 @@ static int normalize_path_callback(struct string_list_item *item, void *cb_data)\n \tif (normalize_path_copy(buf, ceil) < 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@@ -615,9 +617,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":"200169","messageId":"1348899362-4057-8-git-send-email-mhagger@alum.mit.edu","threadId":"31673","inReplyTo":"1348899362-4057-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 7/9] longest_ancestor_length(): use string_list_longest_prefix()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T06:16:00Z","receivedAt":"2012-09-29T06:16:00Z","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 <mhagger@alum.mit.edu>\n---\n path.c | 15 ++++-----------\n 1 file changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex a56c64a..b20f2fb 100644\n--- a/path.c\n+++ b/path.c\n@@ -604,23 +604,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":"200168","messageId":"1348899362-4057-9-git-send-email-mhagger@alum.mit.edu","threadId":"31673","inReplyTo":"1348899362-4057-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 8/9] longest_ancestor_length(): resolve symlinks before comparing paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T06:16:01Z","receivedAt":"2012-09-29T06:16:01Z","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.  On the other hand:\n\n* longest_ancestor_length() is now built of reusable components that\n  are themselves tested separately (string_list_split(),\n  string_list_longest_prefix(), and real_path_if_valid()), so it\n  contains less code that can go wrong.\n\n* longest_ancestor_length() gets some testing (albeit not systematic)\n  via the GIT_CEILING_DIRECTORIES tests.\n\nTherefore the work of updating these tests exceeds any expected\nbenefits.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n path.c                | 18 +++++++++------\n t/t0060-path-utils.sh | 64 ---------------------------------------------------\n 2 files changed, 11 insertions(+), 71 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex b20f2fb..40d7360 100644\n--- a/path.c\n+++ b/path.c\n@@ -570,21 +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-\tif (normalize_path_copy(buf, ceil) < 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\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":"200166","messageId":"1348899362-4057-10-git-send-email-mhagger@alum.mit.edu","threadId":"31673","inReplyTo":"1348899362-4057-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 9/9] t1504: stop resolving symlinks in GIT_CEILING_DIRECTORIES","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-29T06:16:02Z","receivedAt":"2012-09-29T06:16:02Z","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 <mhagger@alum.mit.edu>\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":"200205","messageId":"7vd314gcti.fsf@alter.siamese.dyndns.org","threadId":"31673","inReplyTo":"1348899362-4057-9-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 8/9] longest_ancestor_length(): resolve symlinks before comparing paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-30T08:00:25Z","receivedAt":"2012-09-30T08:00:25Z","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> 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\nI somehow feel that this is making the logic unnecessarily\nconvoluted by solving the problem at a wrong level.\n\nIf longest_ancestor_length() takes a single string and a list of\ncandidate string prefixes, conceptually it should be usable for any\nhierarchy-looking string that uses slashes as hierarchy separator\n(e.g. refs that may be stored in packed-refs that you cannot expect\nlstat(2) or readlink(2) to give you any sensible results).\n\nThe real problem is that the list given from the environment may\ncontain a path that violates that \"it suffices to take the longest\nstring-prefix taking slashes into account\" assumption in such a\ngeneric l-a-l implementation, no?  And this patch solves it by\nmaking l-a-l _less_ generic and forcing it to be aware of the glitch\nof its caller (you can either blame clueless user who lies when\nsetting the GIT_CEILING_DIRECTORIES by including paths contaminated\nwith symlinks, or blame the calling setup_git_directory_gently_1()\nfunction that does not resolve the symbolic links before calling\nthis function).\n\nAs l-a-l is only used by the \"stop at the ceiling\" logic, isn't it a\nfar simpler solution to keep the function work at the string level,\nand make the caller fix its input, i.e. the value taken from the\nenvironment variable, before calling it?  That is, grab the value of\nGIT_CEILING_DIRECTORIES, split it into a list at PATH_SEP (while\nrejecting any non-absolute paths), normalize the elements of that\nlist by resolving symbolic links, and then pass the cwd[] and the\nnormalized string list to l-a-l?\n\nThe resulting callsite in setup_git_directory_gently_1() would pass\ncwd[] and the ceiling_dirs (which now is a string list), all of\nwhose elements would happen to begin with \"/\" (or dos drive prefix\nfollowed by the \"root\" symbol), but l-a-l can be written in such a\nway that it does not even require that all the input has to begin at\nroot, which would later make it usable with things that are not\npaths (references, for example, all of which begin with \"refs/\" and\nnot \"/refs/\").\n\nHrm?\n"},{"id":"200239","messageId":"50692150.8080504@alum.mit.edu","threadId":"31673","inReplyTo":"7vd314gcti.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 8/9] longest_ancestor_length(): resolve symlinks before comparing paths","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-10-01T04:51:28Z","receivedAt":"2012-10-01T04:51:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/30/2012 10:00 AM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> 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> I somehow feel that this is making the logic unnecessarily\n> convoluted by solving the problem at a wrong level.\n> \n> If longest_ancestor_length() takes a single string and a list of\n> candidate string prefixes, conceptually it should be usable for any\n> hierarchy-looking string that uses slashes as hierarchy separator\n> (e.g. refs that may be stored in packed-refs that you cannot expect\n> lstat(2) or readlink(2) to give you any sensible results).\n> \n> The real problem is that the list given from the environment may\n> contain a path that violates that \"it suffices to take the longest\n> string-prefix taking slashes into account\" assumption in such a\n> generic l-a-l implementation, no?  And this patch solves it by\n> making l-a-l _less_ generic and forcing it to be aware of the glitch\n> of its caller (you can either blame clueless user who lies when\n> setting the GIT_CEILING_DIRECTORIES by including paths contaminated\n> with symlinks, or blame the calling setup_git_directory_gently_1()\n> function that does not resolve the symbolic links before calling\n> this function).\n> \n> As l-a-l is only used by the \"stop at the ceiling\" logic, isn't it a\n> far simpler solution to keep the function work at the string level,\n> and make the caller fix its input, i.e. the value taken from the\n> environment variable, before calling it?  That is, grab the value of\n> GIT_CEILING_DIRECTORIES, split it into a list at PATH_SEP (while\n> rejecting any non-absolute paths), normalize the elements of that\n> list by resolving symbolic links, and then pass the cwd[] and the\n> normalized string list to l-a-l?\n> \n> The resulting callsite in setup_git_directory_gently_1() would pass\n> cwd[] and the ceiling_dirs (which now is a string list), all of\n> whose elements would happen to begin with \"/\" (or dos drive prefix\n> followed by the \"root\" symbol), but l-a-l can be written in such a\n> way that it does not even require that all the input has to begin at\n> root, which would later make it usable with things that are not\n> paths (references, for example, all of which begin with \"refs/\" and\n> not \"/refs/\").\n\nI agree that longest_ancestor_length() is not very generic or\ninteresting anymore.  Nearly all of its \"added value\" comes from the\nnormalize_path_callback() helper function.  One possibility would be to\ninline it at the one place it is called.\n\nThe function string_list_longest_prefix() was my attempt to isolate the\nkernel of generic functionality from longest_ancestor_path().  It is\n*almost* the function that you are proposing, with the exception that it\ndoes not ensure that the prefix match ends at a \"/\" boundary.  So\nanother alternative could be to change this function to respect \"/\"\nboundaries.  (After the change, the function might not belong in the\nstring-list API anymore.)\n\nHowever, the semantics of a function that matches prefixes at \"/\"\nboundaries is not entirely obvious.  Suppose we would implement the test\nvia a function like path_prefixcmp(path, prefix).  I can think of a few\npolicy questions that would have to be answered:\n\n* Is a trailing slash on the prefix argument required, optional, or\nprohibited?  What if the prefix is \"/\" or \"//\" or \"c:/\"?\n\n* Is a trailing slash on the path argument optional/prohibited?  Are \"/\"\nor \"//\" allowed as path arguments?\n\n* Is a path its own prefix?\n\n    path_prefixcmp(\"a/b\", \"a/b\") -> true or false?\n\n(For the implementation of longest_ancestor_path(), we would prefer this\nto return \"false\".)  Or does the answer depend on whether the prefix has\na trailing slash?\n\n    path_prefixcmp(\"a/b\", \"a/b\") -> true?\n    path_prefixcmp(\"a/b\", \"a/b/\") -> false?\n\nPart of the reason that I implemented string_list_longest_prefix()\ninstead of the function that you suggest is that the behavior of the\nformer is far more obvious.\n\nI think I would advocate that the prefix has to match the front of the\npath exactly (including any trailing slashes) and either\n\n    strlen(prefix) == 0\n    or the prefix ended with a '/'\n    or the prefix and path are identical\n    or the character in path following the matching part is a '/'\n\nThis would allow the \"is path its own prefix\" policy to be decided by\nthe caller by either including or omitting a trailing slash on the\nprefix argument.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"200241","messageId":"7vipauep32.fsf@alter.siamese.dyndns.org","threadId":"31673","inReplyTo":"50692150.8080504@alum.mit.edu","subject":"Re: [PATCH v2 8/9] longest_ancestor_length(): resolve symlinks before comparing paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-01T05:30:41Z","receivedAt":"2012-10-01T05:30:41Z","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> I think I would advocate that the prefix has to match the front of\n> the path exactly (including any trailing slashes) and either\n>\n>     strlen(prefix) == 0\n>     or the prefix ended with a '/'\n>     or the prefix and path are identical\n>     or the character in path following the matching part is a '/'\n>\n> This would allow the \"is path its own prefix\" policy to be decided by\n> the caller by either including or omitting a trailing slash on the\n> prefix argument.\n\nI think that is sensible thing to do.\n\nThe primary thing I found questionable was that the function, given\n\"/net/wink/project/frotz\" to check against \"/pub:/s\" (or \"/pub/:/s/\"\nif you like), will report that \"/net/wink/project\" directory is the\nlongest ancestor, when \"/s\" is a symlink that happens to point at\n\"/net/wink/project\".  It is very counter-intuitive when you view its\ntwo input strings as strings.  By making its sole caller expand the\nsymbolic links, it would be a lot clearer what is going on to\nanybody who follow the codepath.  We have one path obtained from\ngetcwd() and a set of paths all of which are real paths without\nsymbolic aliasing, and we check if one among the latter cover\nan earlier part of the former.\n"},{"id":"200657","messageId":"506FE619.1020608@alum.mit.edu","threadId":"31673","inReplyTo":"50692150.8080504@alum.mit.edu","subject":"Proposed function path_in_directory() [was: Re: [PATCH v2 8/9] longest_ancestor_length(): resolve symlinks before comparing paths]","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-10-06T08:04:41Z","receivedAt":"2012-10-06T08:04:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/01/2012 06:51 AM, Michael Haggerty wrote:\n> I think I would advocate that the prefix has to match the front of the\n> path exactly (including any trailing slashes) and either\n> \n>     strlen(prefix) == 0\n>     or the prefix ended with a '/'\n>     or the prefix and path are identical\n>     or the character in path following the matching part is a '/'\n> \n> This would allow the \"is path its own prefix\" policy to be decided by\n> the caller by either including or omitting a trailing slash on the\n> prefix argument.\n\nThinking about this more, I don't think it will work.  As usual, the\nspecial cases around \"/\" and \"//\" make things awkward.  I think it is\nnecessary to have a separate argument to specify whether \"path is its\nown prefix\".\n\nSo I am trying to decide how a function path_in_directory() should work,\nand would like to get some feedback, especially on the following two points:\n\n1. How should \"//\" be handled?  I don't really have experience with the\npeculiar paths that start with \"//\", so I'm not sure how they should be\nhandled (or even if the handling needs to be platform-dependent).  My\nworking hypothesis is that the inputs should be normalized by the\ncaller, so if the caller wants \"//\" to be treated as equivalent to \"/\"\nthen the caller should normalize them *before* calling this function.\nConversely, if the caller passes \"//\" to the function, that implies that\n\"//\" is distinct from \"/\" and \"//\" is considered a proper subdirectory\nof \"/\".  See the cases marked with \"??????\" below.\n\n2. Does there need to be any special related to DOS paths?\n\n> /*\n>  * Return true iff path is within dir.  The comparison is textual,\n>  * meaning that path and dir should be normalized and either both be\n>  * absolute or both be relative to the same directory.  If path and\n>  * dir represent the *same* path, then return true iff allow_equal is\n>  * true.  Single trailing slashes on either path or dir are ignored,\n>  * (except for the special case \"//\"); i.e., \"a/b\" and \"a/b/\" are\n>  * treated equivalently, as are \"\" and \"/\".  Examples (* means \"don't\n>  * care\"):\n>  *\n>  * - path_in_directory(\"a/b\", \"a\", *) -> true\n>  * - path_in_directory(\"a\", \"a/b\", *) -> false\n>  * - path_in_directory(\"ab\", \"a\", *) -> false\n>  * - path_in_directory(\"a/b\", \"a/b\", 0) -> false\n>  *   (same if either argument is replaced with \"a/b/\")\n>  * - path_in_directory(\"a/b\", \"a/b\", 1) -> true\n>  *   (same if either argument is replaced with \"a/b/\")\n>  * - path_in_directory(*, \"/\", 1) -> true\n>  * - path_in_directory(\"/\", \"/\", 0) -> false\n>  * - path_in_directory(\"//\", \"/\", 0) -> true    ??????\n>  * - path_in_directory(\"//\", \"/\", 1) -> true\n>  * - path_in_directory(\"/\", \"//\", 0) -> false\n>  * - path_in_directory(\"/\", \"//\", 1) -> false   ??????\n>  * - path_in_directory(\"/a/b\", \"//\", *) -> false\n>  */\n> int path_in_directory(const char *path, const char *dir, int allow_equal);\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"200752","messageId":"7vy5jhlz70.fsf@alter.siamese.dyndns.org","threadId":"31673","inReplyTo":"506FE619.1020608@alum.mit.edu","subject":"Re: Proposed function path_in_directory()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-08T16:13:07Z","receivedAt":"2012-10-08T16:13:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> On 10/01/2012 06:51 AM, Michael Haggerty wrote:\n>> I think I would advocate that the prefix has to match the front of the\n>> path exactly (including any trailing slashes) and either\n>> \n>>     strlen(prefix) == 0\n>>     or the prefix ended with a '/'\n>>     or the prefix and path are identical\n>>     or the character in path following the matching part is a '/'\n>> \n>> This would allow the \"is path its own prefix\" policy to be decided by\n>> the caller by either including or omitting a trailing slash on the\n>> prefix argument.\n>\n> Thinking about this more, I don't think it will work.  As usual, the\n> special cases around \"/\" and \"//\" make things awkward.  I think it is\n> necessary to have a separate argument to specify whether \"path is its\n> own prefix\".\n>\n> So I am trying to decide how a function path_in_directory() should work,\n> and would like to get some feedback, especially on the following two points:\n>\n> 1. How should \"//\" be handled?  I don't really have experience with the\n> peculiar paths that start with \"//\", so I'm not sure how they should be\n> handled (or even if the handling needs to be platform-dependent).  My\n> working hypothesis is that the inputs should be normalized by the\n> caller, so if the caller wants \"//\" to be treated as equivalent to \"/\"\n> then the caller should normalize them *before* calling this function.\n> Conversely, if the caller passes \"//\" to the function, that implies that\n> \"//\" is distinct from \"/\" and \"//\" is considered a proper subdirectory\n> of \"/\".  See the cases marked with \"??????\" below.\n\nI think POSIX essentially says that anything that begins with \"//\"\nis up to the platform, but in practice, the only real-life variant\nwe see is \"//dosshare/path/from/root\".  I agree with your \"caller\nshould normalize for the semantics it wants to see\".\n\nIf our lazy programming creates paths with duplicated slashes, we\nshould be fixing such codepaths anyway, so assuming that all paths\nwe create ourselves are free of double-slashes _inside_ a path (i.e.\nwhen concatenating a subdirectory name to a directory name), the\nonly case we need to worry about is the double-slashes given by the\nuser (either from the command line, environment, or configuration).\nThe path normalizing logic removes double-slashes inside a path, and\nI think it should do so while keeping the leading slashes, so that we\ndo not lose distinction between \"//dosshare/\" and \"/dosshare/\".\n\n> 2. Does there need to be any special related to DOS paths?\n\nThe ceiling computation may need special case for dos.  What does\nthe getcwd() give us?  Do we learn only the path within the \"current\ndrive\" and need to prefix C: (or D: or X:) ourselves if we really\nwant to tell C:\\bin and D:\\bin apart?\n\nAssuming that is the case, the ceiling computation would need a\nhelper function that hides the gory details of prefixing getcwd()\nresult with drive letter or whatever needed, and another that\nnormalizes the elements of the environment variables (I presume that\nif an element in it without the drive prefix should be normalized to\nadd the current drive letter to it so that the normalized getcwd()\nresult can be compared with it).  E.g. if the ceiling list is\n\"D:/a/b;/trash/\" then getcwd() returning \"/a\" alone does not make it\noutside the ceiling due to \"D:/a/b\"---our current drive must be \"D\"\nfor that pattern to kick in.  The unqualified /trash would apply to\nany drive.\n\nDoes that make sense?\n"},{"id":"200771","messageId":"50731968.7020602@kdbg.org","threadId":"31673","inReplyTo":"7vy5jhlz70.fsf@alter.siamese.dyndns.org","subject":"Re: Proposed function path_in_directory()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-10-08T18:20:24Z","receivedAt":"2012-10-08T18:20:24Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 08.10.2012 18:13, schrieb Junio C Hamano:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>> 2. Does there need to be any special related to DOS paths?\n> \n> The ceiling computation may need special case for dos.  What does\n> the getcwd() give us?  Do we learn only the path within the \"current\n> drive\" and need to prefix C: (or D: or X:) ourselves if we really\n> want to tell C:\\bin and D:\\bin apart?\n\nWe don't have to do that. getcwd() returns the drive letter.\n\n> Assuming that is the case, the ceiling computation would need a\n> helper function that hides the gory details of prefixing getcwd()\n> result with drive letter or whatever needed, and another that\n> normalizes the elements of the environment variables (I presume that\n> if an element in it without the drive prefix should be normalized to\n> add the current drive letter to it so that the normalized getcwd()\n> result can be compared with it).  E.g. if the ceiling list is\n> \"D:/a/b;/trash/\" then getcwd() returning \"/a\" alone does not make it\n> outside the ceiling due to \"D:/a/b\"---our current drive must be \"D\"\n> for that pattern to kick in.  The unqualified /trash would apply to\n> any drive.\n\nA component in an path list like GIT_CEILING_DIRECTORIES or PATH that\ndoes not contain the drive letter is a user error. Do not cater for it.\n\nIt is more important to take into account that those components can\ncontain backslash as directory separators, while our mingw_getcwd()\nreturns forward slashes.\n\n-- Hannes\n"},{"id":"200772","messageId":"7vmwzwlt5b.fsf@alter.siamese.dyndns.org","threadId":"31673","inReplyTo":"50731968.7020602@kdbg.org","subject":"Re: Proposed function path_in_directory()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-08T18:23:44Z","receivedAt":"2012-10-08T18:23:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Am 08.10.2012 18:13, schrieb Junio C Hamano:\n>> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>>> 2. Does there need to be any special related to DOS paths?\n>> \n>> The ceiling computation may need special case for dos.  What does\n>> the getcwd() give us?  Do we learn only the path within the \"current\n>> drive\" and need to prefix C: (or D: or X:) ourselves if we really\n>> want to tell C:\\bin and D:\\bin apart?\n>\n> We don't have to do that. getcwd() returns the drive letter.\n> ...\n> A component in an path list like GIT_CEILING_DIRECTORIES or PATH that\n> does not contain the drive letter is a user error. Do not cater for it.\n\nThat makes it much simpler and saner.  Thanks.\n"}]}