Re: [PATCH 7/8] longest_ancestor_length(): resolve symlinks before comparing paths

On 09/28/2012 12:51 AM, Junio C Hamano wrote:

Michael Haggerty [off-list ref] writes:

quoted

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 prefix_list using real_path_if_valid() before trying
to find matches.

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.

Remove a number of tests of longest_ancestor_length().  It is awkward
to test longest_ancestor_length() now, because its new path
normalization behavior depends on the contents of the whole
filesystem.  But we can live without the tests, because
longest_ancestor_length() is now built of reusable components that are
themselves tested separately: string_list_split(),
string_list_longest_prefix(), and real_path_if_valid().

Errr, components may be correct but the way to combine and construct
could go faulty, so...

I don't see a realistic alternative.  Testing real_path() is itself is
already quite awkward (see t0060), so testing longest_ancestor_length()
would be even more so.  Of course, the GIT_CEILING_DIRECTORIES tests
indirectly test longest_ancestor_length(), though not systematically.

If you have a better suggestion, please let me know.

quoted

Signed-off-by: Michael Haggerty <redacted>
---
 path.c                | 17 ++++++++------
 t/t0060-path-utils.sh | 64 ---------------------------------------------------
 2 files changed, 10 insertions(+), 71 deletions(-)

diff --git a/path.c b/path.c
index 5cace83..981bb06 100644
--- a/path.c
+++ b/path.c

@@ -570,22 +570,25 @@ int normalize_path_copy(char *dst, const char *src)
 
 static int normalize_path_callback(struct string_list_item *item, void *cb_data)
 {
-	char buf[PATH_MAX+2];
+	char *buf;
 	const char *ceil = item->string;
-	int len = strlen(ceil);
+	const char *realpath;
+	int len;
 
-	if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
+	if (!*ceil || !is_absolute_path(ceil))
 		return 0;
-	memcpy(buf, ceil, len+1);
-	if (normalize_path_copy(buf, buf) < 0)
+	realpath = real_path_if_valid(ceil);
+	if (!realpath)
 		return 0;
-	len = strlen(buf);
+	len = strlen(realpath);
+	buf = xmalloc(len + 2); /* Leave space for possible trailing slash */
+	strcpy(buf, realpath);
 	if (len == 0 || buf[len-1] != '/') {
 		buf[len++] = '/';
 		buf[len++] = '\0';
 	}

Nice.

I just noticed that the second "len++" in the final "if" is misleading.
 I will fix that in v2.

Michael

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