Merge branch 'mh/ceiling' into maint

An element on GIT_CEILING_DIRECTORIES list that does not name the
real path to a directory (i.e. a symbolic link) could have caused
the GIT_DIR discovery logic to escape the ceiling.

* mh/ceiling:
  string_list_longest_prefix(): remove function
  setup_git_directory_gently_1(): resolve symlinks in ceiling paths
  longest_ancestor_length(): require prefix list entries to be normalized
  longest_ancestor_length(): take a string_list argument for prefixes
  longest_ancestor_length(): use string_list_split()
  Introduce new function real_path_if_valid()
  real_path_internal(): add comment explaining use of cwd
  Introduce new static function real_path_internal()

Documentation/technical/api-string-list.txt

@@ -82,14 +82,6 @@ Functions
 	call free() on the util members of any items that have 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

abspath.c

@@ -15,16 +15,34 @@ int is_directory(const char *path)
 #define MAXDEPTH 5
 
 /*
- * Use this to get the real path, i.e. resolve links. If you want an
+ * Return the real path (i.e., absolute path, with symlinks resolved
- * absolute path but don't mind links, use absolute_path.
+ * 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;
+
+	/*
+	 * 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;
@@ -35,11 +53,19 @@ const char *real_path(const char *path)
 	if (path == buf || path == next_buf)
 		return path;
 
-	if (!*path)
+	if (!*path) {
-		die("The empty string is not a valid 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)
+	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) {
-		die ("Too long path: %.*s", 60, path);
+		if (die_on_error)
+			die("Too long path: %.*s", 60, path);
+		else
+			goto error_out;
+	}
 
 	while (depth--) {
 		if (!is_directory(buf)) {
@@ -54,20 +80,36 @@ const char *real_path(const char *path)
 		}
 
 		if (*buf) {
-			if (!*cwd && !getcwd(cwd, sizeof(cwd)))
+			if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
-				die_errno ("Could not get current working directory");
+				if (die_on_error)
+					die_errno("Could not get current working directory");
+				else
+					goto error_out;
+			}
 
-			if (chdir(buf))
+			if (chdir(buf)) {
-				die_errno ("Could not switch to '%s'", 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)
+			if (len + strlen(last_elem) + 2 > PATH_MAX) {
-				die ("Too long path name: '%s/%s'",
+				if (die_on_error)
-						buf, last_elem);
+					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 +119,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)
+			if (len < 0) {
-				die_errno ("Invalid symlink '%s'", buf);
+				if (die_on_error)
-			if (PATH_MAX <= len)
+					die_errno("Invalid symlink '%s'", buf);
-				die("symbolic link too long: %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 +139,23 @@ 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);
+}
+
+const char *real_path_if_valid(const char *path)
+{
+	return real_path_internal(path, 0);
 }
 
 static const char *get_pwd_cwd(void)

cache.h

@@ -714,10 +714,11 @@ 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);
-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);

path.c

@@ -12,6 +12,7 @@
  */
 #include "cache.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 static char bad_path[] = "/bad-path/";
 
@@ -569,43 +570,38 @@ 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 normalized, absolute paths without
+ * trailing slashes (except for the root directory, which is denoted by "/").
  *
- * Determines, for each path in prefix_list, 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 prefix_list is
+ * is an ancestor.  (Note that this means 0 is returned if prefixes is
- * "/".) "/foo" is not considered an ancestor of "/foobar".  Directories
+ * ["/"].) "/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.
  */
-int longest_ancestor_length(const char *path, const char *prefix_list)
+int longest_ancestor_length(const char *path, struct string_list *prefixes)
 {
-	char buf[PATH_MAX+1];
+	int i, max_len = -1;
-	const char *ceil, *colon;
-	int len, max_len = -1;
 
-	if (prefix_list == NULL || !strcmp(path, "/"))
+	if (!strcmp(path, "/"))
 		return -1;
 
-	for (colon = ceil = prefix_list; *colon; ceil = colon+1) {
+	for (i = 0; i < prefixes->nr; i++) {
-		for (colon = ceil; *colon && *colon != PATH_SEP; colon++);
+		const char *ceil = prefixes->items[i].string;
-		len = colon - ceil;
+		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)
-			continue;
-		len = strlen(buf);
-		if (len > 0 && buf[len-1] == '/')
-			buf[--len] = '\0';
 
-		if (!strncmp(path, buf, len) &&
+		if (len == 1 && ceil[0] == '/')
-		    path[len] == '/' &&
+			len = 0; /* root matches anything, with length 0 */
-		    len > max_len) {
+		else if (!strncmp(path, ceil, len) && path[len] == '/')
+			; /* match of length len */
+		else
+			continue; /* no match */
+
+		if (len > max_len)
 			max_len = len;
-		}
 	}
 
 	return max_len;

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;
@@ -620,6 +621,27 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
 	return buf.st_dev;
 }
 
+/*
+ * 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 canonicalize_ceiling_entry(struct string_list_item *item,
+				      void *unused)
+{
+	char *ceil = item->string;
+	const char *real_path;
+
+	if (!*ceil || !is_absolute_path(ceil))
+		return 0;
+	real_path = real_path_if_valid(ceil);
+	if (!real_path)
+		return 0;
+	free(item->string);
+	item->string = xstrdup(real_path);
+	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.
@@ -627,10 +649,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 +678,14 @@ 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);
+		filter_string_list(&ceiling_dirs, 0,
+				   canonicalize_ceiling_entry, NULL);
+		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;
 

string-list.c

@@ -145,26 +145,6 @@ void string_list_remove_empty_items(struct string_list *list, int free_util) {
 	filter_string_list(list, free_util, item_is_not_empty, NULL);
 }
 
-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) {

string-list.h

@@ -45,15 +45,6 @@ void filter_string_list(struct string_list *list, int free_util,
  */
 void string_list_remove_empty_items(struct string_list *list, int free_util);
 
-/*
- * 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);
 int string_list_find_insert_index(const struct string_list *list, const char *string,

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/ -1
+ancestor /foo /foo:/:/bar 0
-ancestor /foo /:/foo:/bar/ 0
+ancestor /foo /:/bar:/foo 0
-ancestor /foo /foo:/:/bar/ 0
-ancestor /foo /:/bar/:/foo 0
-ancestor /foo/bar "" -1
 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 :/fo -1
+ancestor /foo/bar /:/foo:/bar 4
-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 /foo:/:/bar/ 4
+ancestor /foo/bar /:/bar 0
-ancestor /foo/bar /:/bar/:/fo 0
+ancestor /foo/bar /foo 4
-ancestor /foo/bar /:/bar/ 0
+ancestor /foo/bar /foo:/bar 4
-ancestor /foo/bar .:/foo/. 4
+ancestor /foo/bar /bar -1
-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 \

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

test-path-utils.c

@@ -1,4 +1,32 @@
 #include "cache.h"
+#include "string-list.h"
+
+/*
+ * A "string_list_each_func_t" function that normalizes an entry from
+ * GIT_CEILING_DIRECTORIES.  If the path is unusable for some reason,
+ * die with an explanation.
+ */
+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)
+		die("Empty path is not supported");
+	if (len > PATH_MAX)
+		die("Path \"%s\" is too long", ceil);
+	if (!is_absolute_path(ceil))
+		die("Path \"%s\" is not absolute", ceil);
+	if (normalize_path_copy(buf, ceil) < 0)
+		die("Path \"%s\" could not be normalized", ceil);
+	len = strlen(buf);
+	if (len > 1 && buf[len-1] == '/')
+		die("Normalized path \"%s\" ended with slash", buf);
+	free(item->string);
+	item->string = xstrdup(buf);
+	return 1;
+}
 
 int main(int argc, char **argv)
 {
@@ -30,7 +58,28 @@ 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;
+		char *path = xstrdup(argv[2]);
+
+		/*
+		 * We have to normalize the arguments because under
+		 * Windows, bash mangles arguments that look like
+		 * absolute POSIX paths or colon-separate lists of
+		 * absolute POSIX paths into DOS paths (e.g.,
+		 * "/foo:/foo/bar" might be converted to
+		 * "D:\Src\msysgit\foo;D:\Src\msysgit\foo\bar"),
+		 * whereas longest_ancestor_length() requires paths
+		 * that use forward slashes.
+		 */
+		if (normalize_path_copy(path, path))
+			die("Path \"%s\" could not be normalized", argv[2]);
+		string_list_split(&ceiling_dirs, argv[3], PATH_SEP, -1);
+		filter_string_list(&ceiling_dirs, 0,
+				   normalize_ceiling_entry, NULL);
+		len = longest_ancestor_length(path, &ceiling_dirs);
+		string_list_clear(&ceiling_dirs, 0);
+		free(path);
 		printf("%d\n", len);
 		return 0;
 	}

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;