[PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[Michael Haggerty]

v3 of the series, reworked WRT v2:

* Change longest_ancestor_length() to take a string_list for its
  prefixes argument (instead of a PATH_SEP-separated string).

* Move the responsibility for canonicalizing prefixes from
  longest_ancestor_length() to its caller in
  setup_git_directory_gently_1().  This keeps
  longest_ancestor_length() testable, though the test inputs now have
  to be canonicalized.

* Remove function string_list_longest_prefix(), which was mainly
  intended to be used in the implementation of
  longest_ancestor_length() but is now unneeded.

Thanks to Junio for his comments.

I would like to explicitly point out a possible objection to this
whole enterprise.  GIT_CEILING_DIRECTORIES is used to prevent git from
mucking around in certain directories, to avoid expensive accesses
(for example of remote directories).  The original motivation for the
feature was a user whose home directory /home/$USER was automounted.
When git was invoked outside of a git tree, it would crawl up the
filesystem tree, eventually reaching /home, and then try to access the
paths

    /home
    /home/.git
    /home/.git/objects
    /home/objects

The last three accesses would be very expensive because the system
would attempt to automount the entries listed.

This patch series has the side effect that all of the directories
listed in GIT_CEILING_DIRECTORIES are accessed *unconditionally* to
resolve any symlinks that are present in their paths.  It is
admittedly odd that a feature intended to avoid accessing expensive
directories would now *intentionally* access directories near the
expensive ones.  In the above scenario this shouldn't be a problem,
because /home would be the directory listed in
GIT_CEILING_DIRECTORIES, and accessing /home itself shouldn't be
expensive.  But there might be other scenarios for which this patch
series causes a performance regression.

Another point: After this change, it would be trivially possible to
support non-absolute paths in GIT_CEILING_DIRECTORIES; just remove the
call to is_absolute_path() from normalize_ceiling_entry().  This might
be convenient for the test suite.

Michael Haggerty (8):
  Introduce new static function real_path_internal()
  real_path_internal(): add comment explaining use of cwd
  Introduce new function real_path_if_valid()
  longest_ancestor_length(): use string_list_split()
  longest_ancestor_length(): take a string_list argument for prefixes
  longest_ancestor_length(): require prefix list entries to be
    normalized
  normalize_ceiling_entry(): resolve symlinks
  string_list_longest_prefix(): remove function

 Documentation/technical/api-string-list.txt |   8 ---
 abspath.c                                   | 105 ++++++++++++++++++++++------
 cache.h                                     |   3 +-
 path.c                                      |  46 ++++++------
 setup.c                                     |  32 ++++++++-
 string-list.c                               |  20 ------
 string-list.h                               |   8 ---
 t/t0060-path-utils.sh                       |  41 ++++-------
 t/t0063-string-list.sh                      |  30 --------
 test-path-utils.c                           |   8 ++-
 test-string-list.c                          |  20 ------
 11 files changed, 157 insertions(+), 164 deletions(-)

-- 
1.7.11.3

[PATCH v3 1/8] Introduce new static function real_path_internal()

[Michael Haggerty]

It accepts a new parameter, die_on_error.  If die_on_error is false,
it simply cleans up after itself and returns NULL rather than dying.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 abspath.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 72 insertions(+), 21 deletions(-)

diff --git a/abspath.c b/abspath.c
index 05f2d79..a7ab8e9 100644
--- a/abspath.c
+++ b/abspath.c
@@ -15,15 +15,26 @@ int is_directory(const char *path)
 #define MAXDEPTH 5
 
 /*
- * Use this to get the real path, i.e. resolve links. If you want an
- * absolute path but don't mind links, use absolute_path.
+ * Return the real path (i.e., absolute path, with symlinks resolved
+ * and extra slashes removed) equivalent to the specified path.  (If
+ * you want an absolute path but don't mind links, use
+ * absolute_path().)  The return value is a pointer to a static
+ * buffer.
+ *
+ * The input and all intermediate paths must be shorter than MAX_PATH.
+ * The directory part of path (i.e., everything up to the last
+ * dir_sep) must denote a valid, existing directory, but the last
+ * component need not exist.  If die_on_error is set, then die with an
+ * informative error message if there is a problem.  Otherwise, return
+ * NULL on errors (without generating any output).
  *
  * If path is our buffer, then return path, as it's already what the
  * user wants.
  */
-const char *real_path(const char *path)
+static const char *real_path_internal(const char *path, int die_on_error)
 {
 	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
+	char *retval = NULL;
 	char cwd[1024] = "";
 	int buf_index = 1;
 
@@ -35,11 +46,19 @@ const char *real_path(const char *path)
 	if (path == buf || path == next_buf)
 		return path;
 
-	if (!*path)
-		die("The empty string is not a valid path");
+	if (!*path) {
+		if (die_on_error)
+			die("The empty string is not a valid path");
+		else
+			goto error_out;
+	}
 
-	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
-		die ("Too long path: %.*s", 60, path);
+	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) {
+		if (die_on_error)
+			die("Too long path: %.*s", 60, path);
+		else
+			goto error_out;
+	}
 
 	while (depth--) {
 		if (!is_directory(buf)) {
@@ -54,20 +73,36 @@ const char *real_path(const char *path)
 		}
 
 		if (*buf) {
-			if (!*cwd && !getcwd(cwd, sizeof(cwd)))
-				die_errno ("Could not get current working directory");
+			if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
+				if (die_on_error)
+					die_errno("Could not get current working directory");
+				else
+					goto error_out;
+			}
 
-			if (chdir(buf))
-				die_errno ("Could not switch to '%s'", buf);
+			if (chdir(buf)) {
+				if (die_on_error)
+					die_errno("Could not switch to '%s'", buf);
+				else
+					goto error_out;
+			}
+		}
+		if (!getcwd(buf, PATH_MAX)) {
+			if (die_on_error)
+				die_errno("Could not get current working directory");
+			else
+				goto error_out;
 		}
-		if (!getcwd(buf, PATH_MAX))
-			die_errno ("Could not get current working directory");
 
 		if (last_elem) {
 			size_t len = strlen(buf);
-			if (len + strlen(last_elem) + 2 > PATH_MAX)
-				die ("Too long path name: '%s/%s'",
-						buf, last_elem);
+			if (len + strlen(last_elem) + 2 > PATH_MAX) {
+				if (die_on_error)
+					die("Too long path name: '%s/%s'",
+					    buf, last_elem);
+				else
+					goto error_out;
+			}
 			if (len && !is_dir_sep(buf[len-1]))
 				buf[len++] = '/';
 			strcpy(buf + len, last_elem);
@@ -77,10 +112,18 @@ const char *real_path(const char *path)
 
 		if (!lstat(buf, &st) && S_ISLNK(st.st_mode)) {
 			ssize_t len = readlink(buf, next_buf, PATH_MAX);
-			if (len < 0)
-				die_errno ("Invalid symlink '%s'", buf);
-			if (PATH_MAX <= len)
-				die("symbolic link too long: %s", buf);
+			if (len < 0) {
+				if (die_on_error)
+					die_errno("Invalid symlink '%s'", buf);
+				else
+					goto error_out;
+			}
+			if (PATH_MAX <= len) {
+				if (die_on_error)
+					die("symbolic link too long: %s", buf);
+				else
+					goto error_out;
+			}
 			next_buf[len] = '\0';
 			buf = next_buf;
 			buf_index = 1 - buf_index;
@@ -89,10 +132,18 @@ const char *real_path(const char *path)
 			break;
 	}
 
+	retval = buf;
+error_out:
+	free(last_elem);
 	if (*cwd && chdir(cwd))
 		die_errno ("Could not change back to '%s'", cwd);
 
-	return buf;
+	return retval;
+}
+
+const char *real_path(const char *path)
+{
+	return real_path_internal(path, 1);
 }
 
 static const char *get_pwd_cwd(void)
-- 
1.7.11.3

[PATCH v3 2/8] real_path_internal(): add comment explaining use of cwd

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 abspath.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/abspath.c b/abspath.c
index a7ab8e9..f8a526f 100644
--- a/abspath.c
+++ b/abspath.c
@@ -35,7 +35,14 @@ static const char *real_path_internal(const char *path, int die_on_error)
 {
 	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
 	char *retval = NULL;
+
+	/*
+	 * If we have to temporarily chdir(), store the original CWD
+	 * here so that we can chdir() back to it at the end of the
+	 * function:
+	 */
 	char cwd[1024] = "";
+
 	int buf_index = 1;
 
 	int depth = MAXDEPTH;
-- 
1.7.11.3

[PATCH v3 3/8] Introduce new function real_path_if_valid()

[Michael Haggerty]

The function is like real_path(), except that it returns NULL on error
instead of dying.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 abspath.c | 5 +++++
 cache.h   | 1 +
 2 files changed, 6 insertions(+)

diff --git a/abspath.c b/abspath.c
index f8a526f..40cdc46 100644
--- a/abspath.c
+++ b/abspath.c
@@ -153,6 +153,11 @@ const char *real_path(const char *path)
 	return real_path_internal(path, 1);
 }
 
+const char *real_path_if_valid(const char *path)
+{
+	return real_path_internal(path, 0);
+}
+
 static const char *get_pwd_cwd(void)
 {
 	static char cwd[PATH_MAX + 1];
diff --git a/cache.h b/cache.h
index a58df84..b0d75bc 100644
--- a/cache.h
+++ b/cache.h
@@ -714,6 +714,7 @@ static inline int is_absolute_path(const char *path)
 }
 int is_directory(const char *);
 const char *real_path(const char *path);
+const char *real_path_if_valid(const char *path);
 const char *absolute_path(const char *path);
 const char *relative_path(const char *abs, const char *base);
 int normalize_path_copy(char *dst, const char *src);
-- 
1.7.11.3

[PATCH v3 4/8] longest_ancestor_length(): use string_list_split()

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 path.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/path.c b/path.c
index cbbdf7d..f455e8e 100644
--- a/path.c
+++ b/path.c
@@ -12,6 +12,7 @@
  */
 #include "cache.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 static char bad_path[] = "/bad-path/";
 
@@ -582,20 +583,22 @@ int normalize_path_copy(char *dst, const char *src)
  */
 int longest_ancestor_length(const char *path, const char *prefix_list)
 {
+	struct string_list prefixes = STRING_LIST_INIT_DUP;
 	char buf[PATH_MAX+1];
-	const char *ceil, *colon;
-	int len, max_len = -1;
+	int i, max_len = -1;
 
 	if (prefix_list == NULL || !strcmp(path, "/"))
 		return -1;
 
-	for (colon = ceil = prefix_list; *colon; ceil = colon+1) {
-		for (colon = ceil; *colon && *colon != PATH_SEP; colon++);
-		len = colon - ceil;
+	string_list_split(&prefixes, prefix_list, PATH_SEP, -1);
+
+	for (i = 0; i < prefixes.nr; i++) {
+		const char *ceil = prefixes.items[i].string;
+		int len = strlen(ceil);
+
 		if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
 			continue;
-		strlcpy(buf, ceil, len+1);
-		if (normalize_path_copy(buf, buf) < 0)
+		if (normalize_path_copy(buf, ceil) < 0)
 			continue;
 		len = strlen(buf);
 		if (len > 0 && buf[len-1] == '/')
@@ -608,6 +611,7 @@ int longest_ancestor_length(const char *path, const char *prefix_list)
 		}
 	}
 
+	string_list_clear(&prefixes, 0);
 	return max_len;
 }
 
-- 
1.7.11.3

[PATCH v3 5/8] longest_ancestor_length(): take a string_list argument for prefixes

[Michael Haggerty]

Change longest_ancestor_length() to take the prefixes argument as a
string_list rather than as a PATH_SEP-separated string.  This will
make it easier to change the caller to alter the entries before
calling longest_ancestor_length().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h           |  2 +-
 path.c            | 22 +++++++++-------------
 setup.c           | 11 +++++++++--
 test-path-utils.c |  8 +++++++-
 4 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/cache.h b/cache.h
index b0d75bc..8103385 100644
--- a/cache.h
+++ b/cache.h
@@ -718,7 +718,7 @@ const char *real_path_if_valid(const char *path);
 const char *absolute_path(const char *path);
 const char *relative_path(const char *abs, const char *base);
 int normalize_path_copy(char *dst, const char *src);
-int longest_ancestor_length(const char *path, const char *prefix_list);
+int longest_ancestor_length(const char *path, struct string_list *prefixes);
 char *strip_path_suffix(const char *path, const char *suffix);
 int daemon_avoid_alias(const char *path);
 int offset_1st_component(const char *path);
diff --git a/path.c b/path.c
index f455e8e..b80d2e6 100644
--- a/path.c
+++ b/path.c
@@ -570,30 +570,27 @@ int normalize_path_copy(char *dst, const char *src)
 
 /*
  * path = Canonical absolute path
- * prefix_list = Colon-separated list of absolute paths
+ * prefixes = string_list containing absolute paths
  *
- * Determines, for each path in prefix_list, whether the "prefix" really
+ * Determines, for each path in prefixes, whether the "prefix" really
  * is an ancestor directory of path.  Returns the length of the longest
  * ancestor directory, excluding any trailing slashes, or -1 if no prefix
- * is an ancestor.  (Note that this means 0 is returned if prefix_list is
- * "/".) "/foo" is not considered an ancestor of "/foobar".  Directories
+ * is an ancestor.  (Note that this means 0 is returned if prefixes is
+ * ["/"].) "/foo" is not considered an ancestor of "/foobar".  Directories
  * are not considered to be their own ancestors.  path must be in a
  * canonical form: empty components, or "." or ".." components are not
- * allowed.  prefix_list may be null, which is like "".
+ * allowed.  Empty strings in prefixes are ignored.
  */
-int longest_ancestor_length(const char *path, const char *prefix_list)
+int longest_ancestor_length(const char *path, struct string_list *prefixes)
 {
-	struct string_list prefixes = STRING_LIST_INIT_DUP;
 	char buf[PATH_MAX+1];
 	int i, max_len = -1;
 
-	if (prefix_list == NULL || !strcmp(path, "/"))
+	if (!strcmp(path, "/"))
 		return -1;
 
-	string_list_split(&prefixes, prefix_list, PATH_SEP, -1);
-
-	for (i = 0; i < prefixes.nr; i++) {
-		const char *ceil = prefixes.items[i].string;
+	for (i = 0; i < prefixes->nr; i++) {
+		const char *ceil = prefixes->items[i].string;
 		int len = strlen(ceil);
 
 		if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
@@ -611,7 +608,6 @@ int longest_ancestor_length(const char *path, const char *prefix_list)
 		}
 	}
 
-	string_list_clear(&prefixes, 0);
 	return max_len;
 }
 
diff --git a/setup.c b/setup.c
index 3a1b2fd..b4cd356 100644
--- a/setup.c
+++ b/setup.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "dir.h"
+#include "string-list.h"
 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
@@ -627,10 +628,11 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
 static const char *setup_git_directory_gently_1(int *nongit_ok)
 {
 	const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
+	struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
 	static char cwd[PATH_MAX+1];
 	const char *gitdirenv, *ret;
 	char *gitfile;
-	int len, offset, offset_parent, ceil_offset;
+	int len, offset, offset_parent, ceil_offset = -1;
 	dev_t current_device = 0;
 	int one_filesystem = 1;
 
@@ -655,7 +657,12 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 	if (gitdirenv)
 		return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
 
-	ceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);
+	if (env_ceiling_dirs) {
+		string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
+		ceil_offset = longest_ancestor_length(cwd, &ceiling_dirs);
+		string_list_clear(&ceiling_dirs, 0);
+	}
+
 	if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
 		ceil_offset = 1;
 
diff --git a/test-path-utils.c b/test-path-utils.c
index 3bc20e9..acb0560 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "string-list.h"
 
 int main(int argc, char **argv)
 {
@@ -30,7 +31,12 @@ int main(int argc, char **argv)
 	}
 
 	if (argc == 4 && !strcmp(argv[1], "longest_ancestor_length")) {
-		int len = longest_ancestor_length(argv[2], argv[3]);
+		int len;
+		struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
+
+		string_list_split(&ceiling_dirs, argv[3], PATH_SEP, -1);
+		len = longest_ancestor_length(argv[2], &ceiling_dirs);
+		string_list_clear(&ceiling_dirs, 0);
 		printf("%d\n", len);
 		return 0;
 	}
-- 
1.7.11.3

[PATCH v3 6/8] longest_ancestor_length(): require prefix list entries to be normalized

[Michael Haggerty]

Move the responsibility for normalizing the prefixes passed to
longest_ancestor_length() to its caller.  In t0060, only test
longest_ancestor_lengths using normalized paths: remove empty entries
and non-absolute paths, strip trailing slashes from the paths, and
remove tests that thereby become redundant.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 path.c                | 26 +++++++++++---------------
 setup.c               | 23 +++++++++++++++++++++++
 t/t0060-path-utils.sh | 41 +++++++++++++----------------------------
 3 files changed, 47 insertions(+), 43 deletions(-)

diff --git a/path.c b/path.c
index b80d2e6..d3d3f8b 100644
--- a/path.c
+++ b/path.c
@@ -570,20 +570,20 @@ int normalize_path_copy(char *dst, const char *src)
 
 /*
  * path = Canonical absolute path
- * prefixes = string_list containing absolute paths
+ * prefixes = string_list containing normalized, absolute paths without
+ * trailing slashes (except for the root directory, which is denoted by "/").
  *
- * Determines, for each path in prefixes, whether the "prefix" really
+ * Determines, for each path in prefixes, whether the "prefix"
  * is an ancestor directory of path.  Returns the length of the longest
  * ancestor directory, excluding any trailing slashes, or -1 if no prefix
  * is an ancestor.  (Note that this means 0 is returned if prefixes is
  * ["/"].) "/foo" is not considered an ancestor of "/foobar".  Directories
  * are not considered to be their own ancestors.  path must be in a
  * canonical form: empty components, or "." or ".." components are not
- * allowed.  Empty strings in prefixes are ignored.
+ * allowed.
  */
 int longest_ancestor_length(const char *path, struct string_list *prefixes)
 {
-	char buf[PATH_MAX+1];
 	int i, max_len = -1;
 
 	if (!strcmp(path, "/"))
@@ -593,19 +593,15 @@ int longest_ancestor_length(const char *path, struct string_list *prefixes)
 		const char *ceil = prefixes->items[i].string;
 		int len = strlen(ceil);
 
-		if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
-			continue;
-		if (normalize_path_copy(buf, ceil) < 0)
-			continue;
-		len = strlen(buf);
-		if (len > 0 && buf[len-1] == '/')
-			buf[--len] = '\0';
+		if (len == 1 && ceil[0] == '/')
+			len = 0; /* root matches anything, with length 0 */
+		else if (!strncmp(path, ceil, len) && path[len] == '/')
+			; /* match of length len */
+		else
+			continue; /* no match */
 
-		if (!strncmp(path, buf, len) &&
-		    path[len] == '/' &&
-		    len > max_len) {
+		if (len > max_len)
 			max_len = len;
-		}
 	}
 
 	return max_len;
diff --git a/setup.c b/setup.c
index b4cd356..df97ad3 100644
--- a/setup.c
+++ b/setup.c
@@ -622,6 +622,28 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
 }
 
 /*
+ * A "string_list_each_func_t" function that normalizes an entry from
+ * GIT_CEILING_DIRECTORIES or discards it if unusable.
+ */
+static int normalize_ceiling_entry(struct string_list_item *item, void *unused)
+{
+	const char *ceil = item->string;
+	int len = strlen(ceil);
+	char buf[PATH_MAX+1];
+
+	if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
+		return 0;
+	if (normalize_path_copy(buf, ceil) < 0)
+		return 0;
+	len = strlen(buf);
+	if (len > 1 && buf[len-1] == '/')
+		buf[--len] = '\0';
+	free(item->string);
+	item->string = xstrdup(buf);
+	return 1;
+}
+
+/*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
  */
@@ -659,6 +681,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 
 	if (env_ceiling_dirs) {
 		string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
+		filter_string_list(&ceiling_dirs, 0, normalize_ceiling_entry, NULL);
 		ceil_offset = longest_ancestor_length(cwd, &ceiling_dirs);
 		string_list_clear(&ceiling_dirs, 0);
 	}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 4ef2345..09a42a4 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -93,47 +93,32 @@ norm_path /d1/s1//../s2/../../d2 /d2 POSIX
 norm_path /d1/.../d2 /d1/.../d2 POSIX
 norm_path /d1/..././../d2 /d1/d2 POSIX
 
-ancestor / "" -1
 ancestor / / -1
-ancestor /foo "" -1
-ancestor /foo : -1
-ancestor /foo ::. -1
-ancestor /foo ::..:: -1
 ancestor /foo / 0
 ancestor /foo /fo -1
 ancestor /foo /foo -1
-ancestor /foo /foo/ -1
 ancestor /foo /bar -1
-ancestor /foo /bar/ -1
 ancestor /foo /foo/bar -1
-ancestor /foo /foo:/bar/ -1
-ancestor /foo /foo/:/bar/ -1
-ancestor /foo /foo::/bar/ -1
-ancestor /foo /:/foo:/bar/ 0
-ancestor /foo /foo:/:/bar/ 0
-ancestor /foo /:/bar/:/foo 0
-ancestor /foo/bar "" -1
+ancestor /foo /foo:/bar -1
+ancestor /foo /:/foo:/bar 0
+ancestor /foo /foo:/:/bar 0
+ancestor /foo /:/bar:/foo 0
 ancestor /foo/bar / 0
 ancestor /foo/bar /fo -1
-ancestor /foo/bar foo -1
 ancestor /foo/bar /foo 4
-ancestor /foo/bar /foo/ 4
 ancestor /foo/bar /foo/ba -1
 ancestor /foo/bar /:/fo 0
 ancestor /foo/bar /foo:/foo/ba 4
 ancestor /foo/bar /bar -1
-ancestor /foo/bar /bar/ -1
-ancestor /foo/bar /fo: -1
-ancestor /foo/bar :/fo -1
-ancestor /foo/bar /foo:/bar/ 4
-ancestor /foo/bar /:/foo:/bar/ 4
-ancestor /foo/bar /foo:/:/bar/ 4
-ancestor /foo/bar /:/bar/:/fo 0
-ancestor /foo/bar /:/bar/ 0
-ancestor /foo/bar .:/foo/. 4
-ancestor /foo/bar .:/foo/.:.: 4
-ancestor /foo/bar /foo/./:.:/bar 4
-ancestor /foo/bar .:/bar -1
+ancestor /foo/bar /fo -1
+ancestor /foo/bar /foo:/bar 4
+ancestor /foo/bar /:/foo:/bar 4
+ancestor /foo/bar /foo:/:/bar 4
+ancestor /foo/bar /:/bar:/fo 0
+ancestor /foo/bar /:/bar 0
+ancestor /foo/bar /foo 4
+ancestor /foo/bar /foo:/bar 4
+ancestor /foo/bar /bar -1
 
 test_expect_success 'strip_path_suffix' '
 	test c:/msysgit = $(test-path-utils strip_path_suffix \
-- 
1.7.11.3

Re: [PATCH v3 6/8] longest_ancestor_length(): require prefix list entries to be normalized

[Johannes Sixt]

Am 21.10.2012 07:57, schrieb Michael Haggerty:
> Move the responsibility for normalizing the prefixes passed to
> longest_ancestor_length() to its caller.  In t0060, only test
> longest_ancestor_lengths using normalized paths: remove empty entries
> and non-absolute paths, strip trailing slashes from the paths, and
> remove tests that thereby become redundant.

t0060 does not pass on Windows with this change. Bash's path mangling
strikes again, but the real reason for the failure is that
test-path-util does not normalize its input before it calls into
longest_ancestor_length(). It is not sufficient to reduce the test cases
to normalized ones, because due to path mangling on Windows they are
turned into unnormalized paths (with backslashes instead of forward
slashes).

I suggest to export the new normalize_ceiling_entry() and use it from
test-path-util. Then don't change or remove existing tests.

-- Hannes

[PATCH v3 7/8] normalize_ceiling_entry(): resolve symlinks

[Michael Haggerty]

longest_ancestor_length() relies on a textual comparison of directory
parts to find the part of path that overlaps with one of the paths in
prefix_list.  But this doesn't work if any of the prefixes involves a
symbolic link, because the directories will look different even though
they might logically refer to the same directory.  So canonicalize the
paths listed in GIT_CEILING_DIRECTORIES using real_path_if_valid()
before passing them to longest_ancestor_length().

path is already in canonical form, so doesn't need to be canonicalized
again.

This fixes some problems with using GIT_CEILING_DIRECTORIES that
contains paths involving symlinks, including t4035 if run with --root
set to a path involving symlinks.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 setup.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/setup.c b/setup.c
index df97ad3..2eb5b75 100644
--- a/setup.c
+++ b/setup.c
@@ -622,24 +622,22 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
 }
 
 /*
- * A "string_list_each_func_t" function that normalizes an entry from
- * GIT_CEILING_DIRECTORIES or discards it if unusable.
+ * A "string_list_each_func_t" function that canonicalizes an entry
+ * from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or
+ * discards it if unusable.
  */
 static int normalize_ceiling_entry(struct string_list_item *item, void *unused)
 {
-	const char *ceil = item->string;
-	int len = strlen(ceil);
-	char buf[PATH_MAX+1];
+	char *ceil = item->string;
+	const char *real_path;
 
-	if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
+	if (!*ceil || !is_absolute_path(ceil))
 		return 0;
-	if (normalize_path_copy(buf, ceil) < 0)
+	real_path = real_path_if_valid(ceil);
+	if (!real_path)
 		return 0;
-	len = strlen(buf);
-	if (len > 1 && buf[len-1] == '/')
-		buf[--len] = '\0';
 	free(item->string);
-	item->string = xstrdup(buf);
+	item->string = xstrdup(real_path);
 	return 1;
 }
 
-- 
1.7.11.3

[PATCH v3 8/8] string_list_longest_prefix(): remove function

[Michael Haggerty]

This function was added in f103f95b11d087f07c0c48bf784cd9197e18f203 in
the erroneous expectation that it would be used in the
reimplementation of longest_ancestor_length().  But it turned out to
be easier to use a function specialized for comparing path prefixes
(i.e., one that knows about slashes and root paths) than to prepare
the paths in such a way that a generic string prefix comparison
function can be used.  So delete string_list_longest_prefix() and its
documentation and test cases.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/technical/api-string-list.txt |  8 --------
 string-list.c                               | 20 -------------------
 string-list.h                               |  8 --------
 t/t0063-string-list.sh                      | 30 -----------------------------
 test-string-list.c                          | 20 -------------------
 5 files changed, 86 deletions(-)

diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
index 94d7a2b..618400d 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -75,14 +75,6 @@ Functions
 	to be deleted.  Preserve the order of the items that are
 	retained.
 
-`string_list_longest_prefix`::
-
-	Return the longest string within a string_list that is a
-	prefix (in the sense of prefixcmp()) of the specified string,
-	or NULL if no such prefix exists.  This function does not
-	require the string_list to be sorted (it does a linear
-	search).
-
 `print_string_list`::
 
 	Dump a string_list to stdout, useful mainly for debugging purposes. It
diff --git a/string-list.c b/string-list.c
index c54b816..decfa74 100644
--- a/string-list.c
+++ b/string-list.c
@@ -136,26 +136,6 @@ void filter_string_list(struct string_list *list, int free_util,
 	list->nr = dst;
 }
 
-char *string_list_longest_prefix(const struct string_list *prefixes,
-				 const char *string)
-{
-	int i, max_len = -1;
-	char *retval = NULL;
-
-	for (i = 0; i < prefixes->nr; i++) {
-		char *prefix = prefixes->items[i].string;
-		if (!prefixcmp(string, prefix)) {
-			int len = strlen(prefix);
-			if (len > max_len) {
-				retval = prefix;
-				max_len = len;
-			}
-		}
-	}
-
-	return retval;
-}
-
 void string_list_clear(struct string_list *list, int free_util)
 {
 	if (list->items) {
diff --git a/string-list.h b/string-list.h
index 5efd07b..3a6a6dc 100644
--- a/string-list.h
+++ b/string-list.h
@@ -38,14 +38,6 @@ int for_each_string_list(struct string_list *list,
 void filter_string_list(struct string_list *list, int free_util,
 			string_list_each_func_t want, void *cb_data);
 
-/*
- * Return the longest string in prefixes that is a prefix (in the
- * sense of prefixcmp()) of string, or NULL if no such prefix exists.
- * This function does not require the string_list to be sorted (it
- * does a linear search).
- */
-char *string_list_longest_prefix(const struct string_list *prefixes, const char *string);
-
 
 /* Use these functions only on sorted lists: */
 int string_list_has_string(const struct string_list *list, const char *string);
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index 41c8826..dbfc05e 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -17,14 +17,6 @@ test_split () {
 	"
 }
 
-test_longest_prefix () {
-	test "$(test-string-list longest_prefix "$1" "$2")" = "$3"
-}
-
-test_no_longest_prefix () {
-	test_must_fail test-string-list longest_prefix "$1" "$2"
-}
-
 test_split "foo:bar:baz" ":" "-1" <<EOF
 3
 [0]: "foo"
@@ -96,26 +88,4 @@ test_expect_success "test remove_duplicates" '
 	test a:b:c = "$(test-string-list remove_duplicates a:a:a:b:b:b:c:c:c)"
 '
 
-test_expect_success "test longest_prefix" '
-	test_no_longest_prefix - '' &&
-	test_no_longest_prefix - x &&
-	test_longest_prefix "" x "" &&
-	test_longest_prefix x x x &&
-	test_longest_prefix "" foo "" &&
-	test_longest_prefix : foo "" &&
-	test_longest_prefix f foo f &&
-	test_longest_prefix foo foobar foo &&
-	test_longest_prefix foo foo foo &&
-	test_no_longest_prefix bar foo &&
-	test_no_longest_prefix bar:bar foo &&
-	test_no_longest_prefix foobar foo &&
-	test_longest_prefix foo:bar foo foo &&
-	test_longest_prefix foo:bar bar bar &&
-	test_longest_prefix foo::bar foo foo &&
-	test_longest_prefix foo:foobar foo foo &&
-	test_longest_prefix foobar:foo foo foo &&
-	test_longest_prefix foo: bar "" &&
-	test_longest_prefix :foo bar ""
-'
-
 test_done
diff --git a/test-string-list.c b/test-string-list.c
index 4693295..00ce6c9 100644
--- a/test-string-list.c
+++ b/test-string-list.c
@@ -97,26 +97,6 @@ int main(int argc, char **argv)
 		return 0;
 	}
 
-	if (argc == 4 && !strcmp(argv[1], "longest_prefix")) {
-		/* arguments: <colon-separated-prefixes>|- <string> */
-		struct string_list prefixes = STRING_LIST_INIT_DUP;
-		int retval;
-		const char *prefix_string = argv[2];
-		const char *string = argv[3];
-		const char *match;
-
-		parse_string_list(&prefixes, prefix_string);
-		match = string_list_longest_prefix(&prefixes, string);
-		if (match) {
-			printf("%s\n", match);
-			retval = 0;
-		}
-		else
-			retval = 1;
-		string_list_clear(&prefixes, 0);
-		return retval;
-	}
-
 	fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
 		argv[1] ? argv[1] : "(there was none)");
 	return 1;
-- 
1.7.11.3

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> This patch series has the side effect that all of the directories
> listed in GIT_CEILING_DIRECTORIES are accessed *unconditionally* to
> resolve any symlinks that are present in their paths.  It is
> admittedly odd that a feature intended to avoid accessing expensive
> directories would now *intentionally* access directories near the
> expensive ones.  In the above scenario this shouldn't be a problem,
> because /home would be the directory listed in
> GIT_CEILING_DIRECTORIES, and accessing /home itself shouldn't be
> expensive.

Interesting observation.  In the last sentence, "accessing /home"
does not exactly mean accessing /home, but accessing / to learn
about "home" in it, no?

> But there might be other scenarios for which this patch
> series causes a performance regression.

Yeah, after merging this to 'next', we should ask people who care
about CEILING to test it sufficiently.

Thanks for rerolling.

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[Michael Haggerty]

On 10/21/2012 08:51 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> This patch series has the side effect that all of the directories
>> listed in GIT_CEILING_DIRECTORIES are accessed *unconditionally* to
>> resolve any symlinks that are present in their paths.  It is
>> admittedly odd that a feature intended to avoid accessing expensive
>> directories would now *intentionally* access directories near the
>> expensive ones.  In the above scenario this shouldn't be a problem,
>> because /home would be the directory listed in
>> GIT_CEILING_DIRECTORIES, and accessing /home itself shouldn't be
>> expensive.
> 
> Interesting observation.  In the last sentence, "accessing /home"
> does not exactly mean accessing /home, but accessing / to learn
> about "home" in it, no?

This is the extra overhead on my system for using
GIT_CEILING_DIRECTORIES=/home:

stat("/home", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
getcwd("/home/mhagger", 1024)           = 14
chdir("/home")                          = 0
getcwd("/home", 4096)                   = 6
lstat("/home", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
chdir("/home/mhagger")                  = 0

If I use GIT_CEILING_DIRECTORIES=/dev/shm, which is a symlink to
/run/shm on my system, the overhead is comparable:

stat("/dev/shm", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=200, ...}) = 0
getcwd("/home/mhagger", 1024)           = 14
chdir("/dev/shm")                       = 0
getcwd("/run/shm", 4096)                = 9
lstat("/run/shm", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=200, ...}) = 0
chdir("/home/mhagger")                  = 0

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[David Aguilar]

On Sat, Oct 20, 2012 at 11:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> This patch series has the side effect that all of the directories
>> listed in GIT_CEILING_DIRECTORIES are accessed *unconditionally* to
>> resolve any symlinks that are present in their paths.  It is
>> admittedly odd that a feature intended to avoid accessing expensive
>> directories would now *intentionally* access directories near the
>> expensive ones.  In the above scenario this shouldn't be a problem,
>> because /home would be the directory listed in
>> GIT_CEILING_DIRECTORIES, and accessing /home itself shouldn't be
>> expensive.
>
> Interesting observation.  In the last sentence, "accessing /home"
> does not exactly mean accessing /home, but accessing / to learn
> about "home" in it, no?
>
>> But there might be other scenarios for which this patch
>> series causes a performance regression.
>
> Yeah, after merging this to 'next', we should ask people who care
> about CEILING to test it sufficiently.
>
> Thanks for rerolling.

GIT_CEILING_DIRECTORIES was always about trying to avoid
hitting them at all; they can be (busy) NFS volumes there.

Here's the description from the 1.6.0 release notes:

* A new environment variable GIT_CEILING_DIRECTORIES can be used to stop
  the discovery process of the toplevel of working tree; this may be useful
  when you are working in a slow network disk and are outside any working tree,
  as bash-completion and "git help" may still need to run in these places.

In 8030e44215fe8f34edd57d711a35f2f0f97a0423 Lars added
GIT_ONE_FILESYSTEM to fix a related issue.
Do you guys have GIT_CEILING_DIRECTORIES set too?

We use GIT_CEILING_DIRECTORIES and I'm pretty sure
we don't want every git command hitting them, so this would
be a regression when seen from the POV of our current usage
of this variable, which would be a bummer.

Is there another way to accomplish this without the performance hit?
Maybe something that can be solved with configuration?

I'd be happy to lend a hand if you guys have some ideas
on how we can help keep it fast.  Thoughts?

Original patches for those just joining us:
http://thread.gmane.org/gmane.comp.version-control.git/208102
-- 
David

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[Junio C Hamano]

David Aguilar <davvid@gmail.com> wrote:

>Is there another way to accomplish this without the performance hit?

Perhaps not canonicalize elements on the CEILING list ourselves? If we make it a user error to put symlinked alias in the variable, and document it clearly, wouldn't it suffice?

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[Michael Haggerty]

On 10/29/2012 01:15 AM, David Aguilar wrote:
> On Sat, Oct 20, 2012 at 11:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> This patch series has the side effect that all of the directories
>>> listed in GIT_CEILING_DIRECTORIES are accessed *unconditionally* to
>>> resolve any symlinks that are present in their paths.  It is
>>> admittedly odd that a feature intended to avoid accessing expensive
>>> directories would now *intentionally* access directories near the
>>> expensive ones.  In the above scenario this shouldn't be a problem,
>>> because /home would be the directory listed in
>>> GIT_CEILING_DIRECTORIES, and accessing /home itself shouldn't be
>>> expensive.
>>
>> Interesting observation.  In the last sentence, "accessing /home"
>> does not exactly mean accessing /home, but accessing / to learn
>> about "home" in it, no?
>>
>>> But there might be other scenarios for which this patch
>>> series causes a performance regression.
>>
>> Yeah, after merging this to 'next', we should ask people who care
>> about CEILING to test it sufficiently.
>>
>> Thanks for rerolling.
> 
> GIT_CEILING_DIRECTORIES was always about trying to avoid
> hitting them at all; they can be (busy) NFS volumes there.
> 
> Here's the description from the 1.6.0 release notes:
> 
> * A new environment variable GIT_CEILING_DIRECTORIES can be used to stop
>   the discovery process of the toplevel of working tree; this may be useful
>   when you are working in a slow network disk and are outside any working tree,
>   as bash-completion and "git help" may still need to run in these places.
> 
> In 8030e44215fe8f34edd57d711a35f2f0f97a0423 Lars added
> GIT_ONE_FILESYSTEM to fix a related issue.
> Do you guys have GIT_CEILING_DIRECTORIES set too?
> 
> We use GIT_CEILING_DIRECTORIES and I'm pretty sure
> we don't want every git command hitting them, so this would
> be a regression when seen from the POV of our current usage
> of this variable, which would be a bummer.

I would certainly withdraw the patch series if it causes a performance hit.

The log message of the original commit (0454dd93bf) described the
following scenario: a /home partition under which user home directories
are automounted, and setting GIT_CEILING_DIRECTORIES=/home to avoid
hitting /home/.git, /home/.git/objects, and /home/objects (which would
attempt to automount those directories).  I believe that this scenario
would not be slowed down by my patches.

How do you use GIT_CEILING_DIRECTORIES that the proposed changes cause a
slowdown?

> Is there another way to accomplish this without the performance hit?
> Maybe something that can be solved with configuration?

Without doing the symlink expansion there is no way for git to detect
that GIT_CEILING_DIRECTORIES contains symlinks and is therefore
ineffective.  So the user has no warning about the misconfiguration
(except that git runs slowly).

On 10/29/2012 02:42 AM, Junio C Hamano wrote:
> Perhaps not canonicalize elements on the CEILING list ourselves? If
> we make it a user error to put symlinked alias in the variable, and
> document it clearly, wouldn't it suffice?

There may be no other choice.  (That, and fix the test suite in another
way to tolerate a $PWD that involves symlinks.)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> The log message of the original commit (0454dd93bf) described the
> following scenario: a /home partition under which user home directories
> are automounted, and setting GIT_CEILING_DIRECTORIES=/home to avoid
> hitting /home/.git, /home/.git/objects, and /home/objects (which would
> attempt to automount those directories).  I believe that this scenario
> would not be slowed down by my patches.
>
> How do you use GIT_CEILING_DIRECTORIES that the proposed changes cause a
> slowdown?

Yeah, I was also wondering about that.

David?

>> Is there another way to accomplish this without the performance hit?
>> Maybe something that can be solved with configuration?
>
> Without doing the symlink expansion there is no way for git to detect
> that GIT_CEILING_DIRECTORIES contains symlinks and is therefore
> ineffective.  So the user has no warning about the misconfiguration
> (except that git runs slowly).
>
> On 10/29/2012 02:42 AM, Junio C Hamano wrote:
>> Perhaps not canonicalize elements on the CEILING list ourselves? If
>> we make it a user error to put symlinked alias in the variable, and
>> document it clearly, wouldn't it suffice?
>
> There may be no other choice.  (That, and fix the test suite in another
> way to tolerate a $PWD that involves symlinks.)

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[David Aguilar]

On Mon, Nov 12, 2012 at 9:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> The log message of the original commit (0454dd93bf) described the
>> following scenario: a /home partition under which user home directories
>> are automounted, and setting GIT_CEILING_DIRECTORIES=/home to avoid
>> hitting /home/.git, /home/.git/objects, and /home/objects (which would
>> attempt to automount those directories).  I believe that this scenario
>> would not be slowed down by my patches.
>>
>> How do you use GIT_CEILING_DIRECTORIES that the proposed changes cause a
>> slowdown?
>
> Yeah, I was also wondering about that.
>
> David?

I double-checked our configuration and all the parent directories
of those listed in GIT_CEILING_DIRECTORIES are local,
so our particular configuration would not have a performance hit.

We do have multiple directories listed there.  Some of them share
a parent directory.  I'm assuming the implementation is simple and
does not try and avoid repeating the check when the parent dir is
the same across multiple entries.

In any case, it won't be a problem in practice based on my
reading of the current code.



>>> Is there another way to accomplish this without the performance hit?
>>> Maybe something that can be solved with configuration?
>>
>> Without doing the symlink expansion there is no way for git to detect
>> that GIT_CEILING_DIRECTORIES contains symlinks and is therefore
>> ineffective.  So the user has no warning about the misconfiguration
>> (except that git runs slowly).
>>
>> On 10/29/2012 02:42 AM, Junio C Hamano wrote:
>>> Perhaps not canonicalize elements on the CEILING list ourselves? If
>>> we make it a user error to put symlinked alias in the variable, and
>>> document it clearly, wouldn't it suffice?
>>
>> There may be no other choice.  (That, and fix the test suite in another
>> way to tolerate a $PWD that involves symlinks.)
-- 
David

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[Michael Haggerty]

On 11/13/2012 09:50 PM, David Aguilar wrote:
> On Mon, Nov 12, 2012 at 9:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> The log message of the original commit (0454dd93bf) described the
>>> following scenario: a /home partition under which user home directories
>>> are automounted, and setting GIT_CEILING_DIRECTORIES=/home to avoid
>>> hitting /home/.git, /home/.git/objects, and /home/objects (which would
>>> attempt to automount those directories).  I believe that this scenario
>>> would not be slowed down by my patches.
>>>
>>> How do you use GIT_CEILING_DIRECTORIES that the proposed changes cause a
>>> slowdown?
>>
>> Yeah, I was also wondering about that.
>>
>> David?
> 
> I double-checked our configuration and all the parent directories
> of those listed in GIT_CEILING_DIRECTORIES are local,
> so our particular configuration would not have a performance hit.
> 
> We do have multiple directories listed there.  Some of them share
> a parent directory.  I'm assuming the implementation is simple and
> does not try and avoid repeating the check when the parent dir is
> the same across multiple entries.
> 
> In any case, it won't be a problem in practice based on my
> reading of the current code.

OK, so we're back to the following status: some people (including me)
are nervous that this change could cause a performance regression,
though it seems that the most sensible ways of using the
GIT_CEILING_DIRECTORIES feature would not be affected.

In favor: Currently, if a directory containing a symlink is added to
GIT_CEILING_DIRECTORIES, then GIT_CEILING_DIRECTORIES will not work, git
has no way of recognizing that there is a problem, and the only symptom
observable by the user is that the hoped-for performance improvement
from using GIT_CEILING_DIRECTORIES will not materialize (or will
disappear after a filesystem reorg) [1].

Against: The change will cause a performance regression if a
slow-to-stat directory is listed in GIT_CEILING_DIRECTORIES.  The
slowdown will occur whenever git is run outside of a true git-managed
project, most nastily in the case of using __git_ps1 in a shell prompt.

I don't have a preference either way about whether these patches should
be merged.

Michael

[1] It is also conceivable that GIT_CEILING_DIRECTORIES is being used to
*hide* an enclosing git project rather than to inform git that there are
no enclosing projects, in which case the enclosing project would *not*
be hidden.  This is in fact the mechanism by which the problem causes
failures in our test suite.  But I don't expect that this is a common
real-world scenario, and anyway such a failure would be obvious to the
user and quickly fixed.

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[Anders Kaseorg]

On 10/29/2012 01:10 AM, Michael Haggerty wrote:
> How do you use GIT_CEILING_DIRECTORIES that the proposed changes cause a
> slowdown?

Sorry to bring up this old thread again, but I just realized why my 
computer has been acting so slow when I’m not connected to the network. 
  I put various network filesystem paths in $GIT_CEILING_DIRECTORIES, 
such as /afs/athena.mit.edu/user/a/n/andersk (to avoid hitting its 
parents /afs/athena.mit.edu, /afs/athena.mit.edu/user/a, and 
/afs/athena.mit.edu/user/a/n which all live in different AFS volumes). 
Now when I’m not connected to the network, every invocation of Git, 
including the __git_ps1 in my shell prompt, waits for AFS to timeout.

Obviously I’m going to stop using $GIT_CEILING_DIRECTORIES now that I 
know what the problem is, but I figured you might want to know why this 
feature is now useless for me.

Anders

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[Junio C Hamano]

Anders Kaseorg <andersk@MIT.EDU> writes:

> On 10/29/2012 01:10 AM, Michael Haggerty wrote:
>> How do you use GIT_CEILING_DIRECTORIES that the proposed changes cause a
>> slowdown?
>
> Sorry to bring up this old thread again, but I just realized why my
> computer has been acting so slow when I’m not connected to the
> network. I put various network filesystem paths in
> $GIT_CEILING_DIRECTORIES, such as /afs/athena.mit.edu/user/a/n/andersk
> (to avoid hitting its parents /afs/athena.mit.edu,
> /afs/athena.mit.edu/user/a, and /afs/athena.mit.edu/user/a/n which all
> live in different AFS volumes). Now when I’m not connected to the
> network, every invocation of Git, including the __git_ps1 in my shell
> prompt, waits for AFS to timeout.

Thanks for a report.

Assuming that this says "yes":

	D=/afs/athena.mit.edu/user/a/n/andersk/my/dir
        cd "$D"
        test "$(/bin/pwd)" = "$D" && echo yes

iow, AFS mounting does not have funny symbolic link issues that make
the logical and physical PWD to be different like some automounter
implementation used to have, perhaps we can introduce a way for the
user to say "The element of this CEILING list do not have any
alias due to funny symlinks" to solve this.  Perhaps existing of an
empty element in the list would do?  E.g.

	GIT_CEILING_DIRECTORIES=:/afs/athena.mit.edu/users/a/n/andesk

or something like that.  And in such a case, we do not run realpath
on the elements on the list before comparing them with what we get
from getcwd(3).

Of course, you could condionally unset the environment while
offline, but that feels like an ugly hack.

[RFC] Provide a mechanism to turn off symlink resolution in ceiling paths

[Michael Haggerty]

Commit 1b77d83cab 'setup_git_directory_gently_1(): resolve symlinks in
ceiling paths' changed the setup code to resolve symlinks in the
entries in GIT_CEILING_DIRECTORIES.  Because those entries are
compared textually to the symlink-resolved current directory, an entry
in GIT_CEILING_DIRECTORIES that contained a symlink would have no
effect.  It was known that this could cause performance problems if
the symlink resolution *itself* touched slow filesystems, but it was
thought that such use cases would be unlikely.

After this change was released, Anders Kaseorg <andersk@mit.edu>
reported:

> [...] my computer has been acting so slow when I’m not connected to
> the network.  I put various network filesystem paths in
> $GIT_CEILING_DIRECTORIES, such as
> /afs/athena.mit.edu/user/a/n/andersk (to avoid hitting its parents
> /afs/athena.mit.edu, /afs/athena.mit.edu/user/a, and
> /afs/athena.mit.edu/user/a/n which all live in different AFS
> volumes).  Now when I’m not connected to the network, every
> invocation of Git, including the __git_ps1 in my shell prompt, waits
> for AFS to timeout.

So provide the following mechanism to turn off symlink resolution in
GIT_CEILING_DIRECTORIES entries: if that environment variable contains
an empty entry (e.g., GIT_CEILING_DIRECTORIES=:/foo/bar:/xyzzy or
GIT_CEILING_DIRECTORIES=/foo/bar::/xyzzy), then do not resolve
symlinks in paths that follow the empty entry.

---

This is a possible implementation (untested!) of Junio's suggestion,
with the slight twist that the empty entry can appear anywhere in
GIT_CEILING_DIRECTORIES and only turns off symlink expansion for
subsequent entries.  (The original suggestion would be similarly easy
to implement if it is preferred.)

Unfortunately I am swamped with other work right now so I don't have
time to test the code and might not be able to respond promptly to
feedback.

Another alternative (not implemented here) would be to support a
second environment variable with an ugly name like
GIT_CEILING_DIRECTORIES_NO_SYMLINKS which, when set, tells Git not to
resolve symlinks in GIT_CEILING_DIRECTORIES.  Hopefully the variable
name itself would warn the user that symlinks are an issue.

The ugliness of the situation unfortunately seems to preclude a
non-ugly solution of one form or another.

 Documentation/git.txt | 18 ++++++++++++------
 setup.c               | 32 ++++++++++++++++++++++----------
 2 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index da0115f..35c0517 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -674,12 +674,18 @@ git so take care if using Cogito etc.
 	The '--namespace' command-line option also sets this value.
 
 'GIT_CEILING_DIRECTORIES'::
-	This should be a colon-separated list of absolute paths.
-	If set, it is a list of directories that git should not chdir
-	up into while looking for a repository directory.
-	It will not exclude the current working directory or
-	a GIT_DIR set on the command line or in the environment.
-	(Useful for excluding slow-loading network directories.)
+	This should be a colon-separated list of absolute paths.  If
+	set, it is a list of directories that git should not chdir up
+	into while looking for a repository directory (useful for
+	excluding slow-loading network directories).  It will not
+	exclude the current working directory or a GIT_DIR set on the
+	command line or in the environment.  Normally, Git has to read
+	the entries in this list are read to resolve any symlinks that
+	might be present.  However, if even this access is slow, you
+	can add an empty entry to the list to tell Git that the
+	subsequent entries are not symlinks and needn't be resolved;
+	e.g.,
+	'GIT_CEILING_DIRECTORIES=/maybe/symlink::/very/slow/non/symlink'.
 
 'GIT_DISCOVERY_ACROSS_FILESYSTEM'::
 	When run in a directory that does not have ".git" repository
diff --git a/setup.c b/setup.c
index f108c4b..1b12017 100644
--- a/setup.c
+++ b/setup.c
@@ -624,22 +624,32 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
 /*
  * A "string_list_each_func_t" function that canonicalizes an entry
  * from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or
- * discards it if unusable.
+ * discards it if unusable.  The presence of an empty entry in
+ * GIT_CEILING_DIRECTORIES turns off canonicalization for all
+ * subsequent entries.
  */
 static int canonicalize_ceiling_entry(struct string_list_item *item,
-				      void *unused)
+				      void *cb_data)
 {
+	int *empty_entry_found = cb_data;
 	char *ceil = item->string;
-	const char *real_path;
 
-	if (!*ceil || !is_absolute_path(ceil))
+	if (!*ceil) {
+		*empty_entry_found = 1;
 		return 0;
-	real_path = real_path_if_valid(ceil);
-	if (!real_path)
+	} else if (!is_absolute_path(ceil)) {
 		return 0;
-	free(item->string);
-	item->string = xstrdup(real_path);
-	return 1;
+	} else if (*empty_entry_found) {
+		/* Keep entry but do not canonicalize it */
+		return 1;
+	} else {
+		const char *real_path = real_path_if_valid(ceil);
+		if (!real_path)
+			return 0;
+		free(item->string);
+		item->string = xstrdup(real_path);
+		return 1;
+	}
 }
 
 /*
@@ -679,9 +689,11 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 		return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
 
 	if (env_ceiling_dirs) {
+		int empty_entry_found = 0;
+
 		string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
 		filter_string_list(&ceiling_dirs, 0,
-				   canonicalize_ceiling_entry, NULL);
+				   canonicalize_ceiling_entry, &empty_entry_found);
 		ceil_offset = longest_ancestor_length(cwd, &ceiling_dirs);
 		string_list_clear(&ceiling_dirs, 0);
 	}
-- 
1.8.1.1

Re: [RFC] Provide a mechanism to turn off symlink resolution in ceiling paths

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> This is a possible implementation (untested!) of Junio's suggestion,
> with the slight twist that the empty entry can appear anywhere in
> GIT_CEILING_DIRECTORIES and only turns off symlink expansion for
> subsequent entries.

Sounds like a good way to go, I think.

Originally I thought that checking with the elements literally and
stop when we have a match, and then falling back to call realpath()
on them to compare, might be a solution, but I do not think it is.
I haven't thought things through to convince myself that is the best
approach to require the users to explicitly tell us to omit calls to
realpath().  Perhaps it is the best we could do, but there might be
even better ways somebody can think up.

Re: [RFC] Provide a mechanism to turn off symlink resolution in ceiling paths

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Unfortunately I am swamped with other work right now so I don't have
> time to test the code and might not be able to respond promptly to
> feedback.

A note like the above is a good way to give a cue to others so that
we can work together to pick up, tie the loose ends and move us
closer to the goal, and is very much appreciated.

I think the patch makes sense; I expanded on the part that has
Anders's report in the log message and added a trivial test.

Testing and eyeballing by others would help very much.  We'd
obviously need our sign-off as well ;-)

Thanks.

-- >8 --
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Wed, 20 Feb 2013 10:09:24 +0100
Subject: [PATCH] Provide a mechanism to turn off symlink resolution in ceiling paths

Commit 1b77d83cab 'setup_git_directory_gently_1(): resolve symlinks
in ceiling paths' changed the setup code to resolve symlinks in the
entries in GIT_CEILING_DIRECTORIES.  Because those entries are
compared textually to the symlink-resolved current directory, an
entry in GIT_CEILING_DIRECTORIES that contained a symlink would have
no effect.  It was known that this could cause performance problems
if the symlink resolution *itself* touched slow filesystems, but it
was thought that such use cases would be unlikely.  The intention of
the earlier change was to deal with a case when the user has this:

	GIT_CEILING_DIRECTORIES=/home/gitster

but in reality, /home/gitster is a symbolic link to somewhere else,
e.g. /net/machine/home4/gitster. A textual comparison between the
specified value /home/gitster and the location getcwd(3) returns
would not help us, but readlink("/home/gitster") would still be
fast.

After this change was released, Anders Kaseorg <andersk@mit.edu>
reported:

> [...] my computer has been acting so slow when I’m not connected to
> the network.  I put various network filesystem paths in
> $GIT_CEILING_DIRECTORIES, such as
> /afs/athena.mit.edu/user/a/n/andersk (to avoid hitting its parents
> /afs/athena.mit.edu, /afs/athena.mit.edu/user/a, and
> /afs/athena.mit.edu/user/a/n which all live in different AFS
> volumes).  Now when I’m not connected to the network, every
> invocation of Git, including the __git_ps1 in my shell prompt, waits
> for AFS to timeout.

To allow users to work this around, give them a mechanism to turn
off symlink resolution in GIT_CEILING_DIRECTORIES entries.  All the
entries that follow an empty entry will not be checked for symbolic
links and used literally in comparison.  E.g. with these:

	GIT_CEILING_DIRECTORIES=:/foo/bar:/xyzzy or
	GIT_CEILING_DIRECTORIES=/foo/bar::/xyzzy

we will not readlink("/xyzzy"), and with the former, we will not
readlink("/foo/bar"), either.
---
 Documentation/git.txt   | 19 +++++++++++++------
 setup.c                 | 32 ++++++++++++++++++++++----------
 t/t1504-ceiling-dirs.sh | 17 +++++++++++++++++
 3 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 6710cb0..5c03616 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -653,12 +653,19 @@ git so take care if using Cogito etc.
 	The '--namespace' command-line option also sets this value.
 
 'GIT_CEILING_DIRECTORIES'::
-	This should be a colon-separated list of absolute paths.
-	If set, it is a list of directories that git should not chdir
-	up into while looking for a repository directory.
-	It will not exclude the current working directory or
-	a GIT_DIR set on the command line or in the environment.
-	(Useful for excluding slow-loading network directories.)
+	This should be a colon-separated list of absolute paths.  If
+	set, it is a list of directories that git should not chdir up
+	into while looking for a repository directory (useful for
+	excluding slow-loading network directories).  It will not
+	exclude the current working directory or a GIT_DIR set on the
+	command line or in the environment.  Normally, Git has to read
+	the entries in this list are read to resolve any symlinks that
+	might be present in order to compare them with the current
+	directory.  However, if even this access is slow, you
+	can add an empty entry to the list to tell Git that the
+	subsequent entries are not symlinks and needn't be resolved;
+	e.g.,
+	'GIT_CEILING_DIRECTORIES=/maybe/symlink::/very/slow/non/symlink'.
 
 'GIT_DISCOVERY_ACROSS_FILESYSTEM'::
 	When run in a directory that does not have ".git" repository
diff --git a/setup.c b/setup.c
index f108c4b..1b12017 100644
--- a/setup.c
+++ b/setup.c
@@ -624,22 +624,32 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
 /*
  * A "string_list_each_func_t" function that canonicalizes an entry
  * from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or
- * discards it if unusable.
+ * discards it if unusable.  The presence of an empty entry in
+ * GIT_CEILING_DIRECTORIES turns off canonicalization for all
+ * subsequent entries.
  */
 static int canonicalize_ceiling_entry(struct string_list_item *item,
-				      void *unused)
+				      void *cb_data)
 {
+	int *empty_entry_found = cb_data;
 	char *ceil = item->string;
-	const char *real_path;
 
-	if (!*ceil || !is_absolute_path(ceil))
+	if (!*ceil) {
+		*empty_entry_found = 1;
 		return 0;
-	real_path = real_path_if_valid(ceil);
-	if (!real_path)
+	} else if (!is_absolute_path(ceil)) {
 		return 0;
-	free(item->string);
-	item->string = xstrdup(real_path);
-	return 1;
+	} else if (*empty_entry_found) {
+		/* Keep entry but do not canonicalize it */
+		return 1;
+	} else {
+		const char *real_path = real_path_if_valid(ceil);
+		if (!real_path)
+			return 0;
+		free(item->string);
+		item->string = xstrdup(real_path);
+		return 1;
+	}
 }
 
 /*
@@ -679,9 +689,11 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 		return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
 
 	if (env_ceiling_dirs) {
+		int empty_entry_found = 0;
+
 		string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
 		filter_string_list(&ceiling_dirs, 0,
-				   canonicalize_ceiling_entry, NULL);
+				   canonicalize_ceiling_entry, &empty_entry_found);
 		ceil_offset = longest_ancestor_length(cwd, &ceiling_dirs);
 		string_list_clear(&ceiling_dirs, 0);
 	}
diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
index cce87a5..3d51615 100755
--- a/t/t1504-ceiling-dirs.sh
+++ b/t/t1504-ceiling-dirs.sh
@@ -44,6 +44,10 @@ test_prefix ceil_at_sub ""
 GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub/"
 test_prefix ceil_at_sub_slash ""
 
+if test_have_prereq SYMLINKS
+then
+	ln -s sub top
+fi
 
 mkdir -p sub/dir || exit 1
 cd sub/dir || exit 1
@@ -68,6 +72,19 @@ test_fail subdir_ceil_at_sub
 GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub/"
 test_fail subdir_ceil_at_sub_slash
 
+if test_have_prereq SYMLINKS
+then
+	GIT_CEILING_DIRECTORIES="$TRASH_ROOT/top"
+	test_fail subdir_ceil_at_top
+	GIT_CEILING_DIRECTORIES="$TRASH_ROOT/top/"
+	test_fail subdir_ceil_at_top_slash
+
+	GIT_CEILING_DIRECTORIES=":$TRASH_ROOT/top"
+	test_prefix subdir_ceil_at_top_no_resolve "sub/dir/"
+	GIT_CEILING_DIRECTORIES=":$TRASH_ROOT/top/"
+	test_prefix subdir_ceil_at_top_slash_no_resolve "sub/dir/"
+fi
+
 GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub/dir"
 test_prefix subdir_ceil_at_subdir "sub/dir/"
 
-- 
1.8.2.rc0.152.g52dbac4

Re: [RFC] Provide a mechanism to turn off symlink resolution in ceiling paths

[Michael Haggerty]

On 02/21/2013 11:53 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Unfortunately I am swamped with other work right now so I don't have
>> time to test the code and might not be able to respond promptly to
>> feedback.
> 
> A note like the above is a good way to give a cue to others so that
> we can work together to pick up, tie the loose ends and move us
> closer to the goal, and is very much appreciated.
> 
> I think the patch makes sense; I expanded on the part that has
> Anders's report in the log message and added a trivial test.
> 
> Testing and eyeballing by others would help very much.  We'd
> obviously need our sign-off as well ;-)

Thanks for following up on this.  Your tests look OK by eyeball and they
run successfully here whether the testing --root is under a symlink or
not.  I did notice some minor niggles in the text (including one in my
original submission); see below.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Michael

> -- >8 --
> From: Michael Haggerty <mhagger@alum.mit.edu>
> Date: Wed, 20 Feb 2013 10:09:24 +0100
> Subject: [PATCH] Provide a mechanism to turn off symlink resolution in ceiling paths
> 
> Commit 1b77d83cab 'setup_git_directory_gently_1(): resolve symlinks
> in ceiling paths' changed the setup code to resolve symlinks in the
> entries in GIT_CEILING_DIRECTORIES.  Because those entries are
> compared textually to the symlink-resolved current directory, an
> entry in GIT_CEILING_DIRECTORIES that contained a symlink would have
> no effect.  It was known that this could cause performance problems
> if the symlink resolution *itself* touched slow filesystems, but it
> was thought that such use cases would be unlikely.  The intention of
> the earlier change was to deal with a case when the user has this:
> 
> 	GIT_CEILING_DIRECTORIES=/home/gitster
> 
> but in reality, /home/gitster is a symbolic link to somewhere else,
> e.g. /net/machine/home4/gitster. A textual comparison between the
> specified value /home/gitster and the location getcwd(3) returns
> would not help us, but readlink("/home/gitster") would still be
> fast.
> 
> After this change was released, Anders Kaseorg <andersk@mit.edu>
> reported:
> 
>> [...] my computer has been acting so slow when I’m not connected to
>> the network.  I put various network filesystem paths in
>> $GIT_CEILING_DIRECTORIES, such as
>> /afs/athena.mit.edu/user/a/n/andersk (to avoid hitting its parents
>> /afs/athena.mit.edu, /afs/athena.mit.edu/user/a, and
>> /afs/athena.mit.edu/user/a/n which all live in different AFS
>> volumes).  Now when I’m not connected to the network, every
>> invocation of Git, including the __git_ps1 in my shell prompt, waits
>> for AFS to timeout.
> 
> To allow users to work this around, give them a mechanism to turn

s/this around/around this problem/

> off symlink resolution in GIT_CEILING_DIRECTORIES entries.  All the
> entries that follow an empty entry will not be checked for symbolic
> links and used literally in comparison.  E.g. with these:

Make it clear that "not" doesn't apply to both sides of the "and", since
the operator precedence in English is undocumented:

s/and/but rather will be/

> 
> 	GIT_CEILING_DIRECTORIES=:/foo/bar:/xyzzy or
> 	GIT_CEILING_DIRECTORIES=/foo/bar::/xyzzy
> 
> we will not readlink("/xyzzy"), and with the former, we will not
> readlink("/foo/bar"), either.
> ---
>  Documentation/git.txt   | 19 +++++++++++++------
>  setup.c                 | 32 ++++++++++++++++++++++----------
>  t/t1504-ceiling-dirs.sh | 17 +++++++++++++++++
>  3 files changed, 52 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 6710cb0..5c03616 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -653,12 +653,19 @@ git so take care if using Cogito etc.
>  	The '--namespace' command-line option also sets this value.
>  
>  'GIT_CEILING_DIRECTORIES'::
> -	This should be a colon-separated list of absolute paths.
> -	If set, it is a list of directories that git should not chdir
> -	up into while looking for a repository directory.
> -	It will not exclude the current working directory or
> -	a GIT_DIR set on the command line or in the environment.
> -	(Useful for excluding slow-loading network directories.)
> +	This should be a colon-separated list of absolute paths.  If
> +	set, it is a list of directories that git should not chdir up
> +	into while looking for a repository directory (useful for
> +	excluding slow-loading network directories).  It will not
> +	exclude the current working directory or a GIT_DIR set on the
> +	command line or in the environment.  Normally, Git has to read
> +	the entries in this list are read to resolve any symlinks that

"read" is duplicated:

s/are read to//

> +	might be present in order to compare them with the current
> +	directory.  However, if even this access is slow, you
> +	can add an empty entry to the list to tell Git that the
> +	subsequent entries are not symlinks and needn't be resolved;
> +	e.g.,
> +	'GIT_CEILING_DIRECTORIES=/maybe/symlink::/very/slow/non/symlink'.
>  
>  'GIT_DISCOVERY_ACROSS_FILESYSTEM'::
>  	When run in a directory that does not have ".git" repository
> diff --git a/setup.c b/setup.c
> index f108c4b..1b12017 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -624,22 +624,32 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
>  /*
>   * A "string_list_each_func_t" function that canonicalizes an entry
>   * from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or
> - * discards it if unusable.
> + * discards it if unusable.  The presence of an empty entry in
> + * GIT_CEILING_DIRECTORIES turns off canonicalization for all
> + * subsequent entries.
>   */
>  static int canonicalize_ceiling_entry(struct string_list_item *item,
> -				      void *unused)
> +				      void *cb_data)
>  {
> +	int *empty_entry_found = cb_data;
>  	char *ceil = item->string;
> -	const char *real_path;
>  
> -	if (!*ceil || !is_absolute_path(ceil))
> +	if (!*ceil) {
> +		*empty_entry_found = 1;
>  		return 0;
> -	real_path = real_path_if_valid(ceil);
> -	if (!real_path)
> +	} else if (!is_absolute_path(ceil)) {
>  		return 0;
> -	free(item->string);
> -	item->string = xstrdup(real_path);
> -	return 1;
> +	} else if (*empty_entry_found) {
> +		/* Keep entry but do not canonicalize it */
> +		return 1;
> +	} else {
> +		const char *real_path = real_path_if_valid(ceil);
> +		if (!real_path)
> +			return 0;
> +		free(item->string);
> +		item->string = xstrdup(real_path);
> +		return 1;
> +	}
>  }
>  
>  /*
> @@ -679,9 +689,11 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
>  		return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
>  
>  	if (env_ceiling_dirs) {
> +		int empty_entry_found = 0;
> +
>  		string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
>  		filter_string_list(&ceiling_dirs, 0,
> -				   canonicalize_ceiling_entry, NULL);
> +				   canonicalize_ceiling_entry, &empty_entry_found);
>  		ceil_offset = longest_ancestor_length(cwd, &ceiling_dirs);
>  		string_list_clear(&ceiling_dirs, 0);
>  	}
> diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
> index cce87a5..3d51615 100755
> --- a/t/t1504-ceiling-dirs.sh
> +++ b/t/t1504-ceiling-dirs.sh
> @@ -44,6 +44,10 @@ test_prefix ceil_at_sub ""
>  GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub/"
>  test_prefix ceil_at_sub_slash ""
>  
> +if test_have_prereq SYMLINKS
> +then
> +	ln -s sub top
> +fi
>  
>  mkdir -p sub/dir || exit 1
>  cd sub/dir || exit 1
> @@ -68,6 +72,19 @@ test_fail subdir_ceil_at_sub
>  GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub/"
>  test_fail subdir_ceil_at_sub_slash
>  
> +if test_have_prereq SYMLINKS
> +then
> +	GIT_CEILING_DIRECTORIES="$TRASH_ROOT/top"
> +	test_fail subdir_ceil_at_top
> +	GIT_CEILING_DIRECTORIES="$TRASH_ROOT/top/"
> +	test_fail subdir_ceil_at_top_slash
> +
> +	GIT_CEILING_DIRECTORIES=":$TRASH_ROOT/top"
> +	test_prefix subdir_ceil_at_top_no_resolve "sub/dir/"
> +	GIT_CEILING_DIRECTORIES=":$TRASH_ROOT/top/"
> +	test_prefix subdir_ceil_at_top_slash_no_resolve "sub/dir/"
> +fi
> +
>  GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub/dir"
>  test_prefix subdir_ceil_at_subdir "sub/dir/"
>  
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[Anders Kaseorg]

On Tue, 19 Feb 2013, Junio C Hamano wrote:
> Assuming that this says "yes":
> 
> 	D=/afs/athena.mit.edu/user/a/n/andersk/my/dir
>         cd "$D"
>         test "$(/bin/pwd)" = "$D" && echo yes

Correct.

> Perhaps existing of an empty element in the list would do?  E.g.
> 
> 	GIT_CEILING_DIRECTORIES=:/afs/athena.mit.edu/users/a/n/andesk
> 
> or something like that.  And in such a case, we do not run realpath on 
> the elements on the list before comparing them with what we get from 
> getcwd(3).

That seems reasonable, and has the advantage of backwards compatibility 
with versions before 1.8.1.2, I guess.

Anders

Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

[Lars Damerow]

>From David Aguilar <davvid@gmail.com>, Sun, Oct 28, 2012 at 05:15:29PM -0700:
> 
> In 8030e44215fe8f34edd57d711a35f2f0f97a0423 Lars added
> GIT_ONE_FILESYSTEM to fix a related issue.
> Do you guys have GIT_CEILING_DIRECTORIES set too?

Once GIT_DISCOVERY_ACROSS_FILESYSTEM (the eventual name for GIT_ONE_FILESYSTEM) was accepted, we stopped setting GIT_CEILING_DIRECTORIES in our environment.

Even so, the way our filesystems work, stat() or lstat() calls on the paths that we used to have in GIT_CEILING_DIRECTORIES wouldn't have caused a problem.

I don't personally have an objection to patches that explicitly access the GIT_CEILING_DIRECTORIES paths, but it seems like other folks' configurations could be harmed by them.

-lars

--
lars r. damerow :: button pusher :: pixar animation studios