git-mv with absolute path derefereces symlinks

If git-mv is provided absolute paths when moving symlinks, it tries to dereference them and (attempts to) move the symlink target rather than the symlink itself, this seems like a quite odd behaviour since it's inconsistent with how git-mv works with symlinks if given relative paths, and I'm thinking it might be a bug, since it not documented in the git-mv manpage.

The prefix_path_gently() function currently applies real_path to everything if given an absolute path, dereferencing symlinks both outside and inside the work tree. In order to manipulate symliks in the work tree using absolute paths, symlinks should only be dereferenced outside the work tree.

Modify prefix_path_gently() to first normalize the path in order to make sure path levels are separated by '/', then use this separator to check the real path of each level of the path until it has found the length that corresponds to the work tree.

For absolute paths, the function did not, nor does now do, any actual prefixing, hence we simply remove the path corresponding to the work tree and return the remaining in-tree part of the path.

Fixes t0060-82.

Signed-off-by Martin Erik Werner <[email protected]>

---
 setup.c               | 54 ++++++++++++++++++++++++++++++++-------------------
 t/t0060-path-utils.sh |  2 +-
 2 files changed, 35 insertions(+), 21 deletions(-)

setup.c +0 -0

index 6c3f85f..bec587e 100644

Quoted 23 lines · Martin Erik Werner

The prefix_path_gently() function currently applies real_path to everything if given an absolute path, dereferencing symlinks both outside and inside the work tree. In order to manipulate symliks in the work tree using absolute paths, symlinks should only be dereferenced outside the work tree.

Modify prefix_path_gently() to first normalize the path in order to make sure path levels are separated by '/', then use this separator to check the real path of each level of the path until it has found the length that corresponds to the work tree.

For absolute paths, the function did not, nor does now do, any actual prefixing, hence we simply remove the path corresponding to the work tree and return the remaining in-tree part of the path.

Fixes t0060-82.

Signed-off-by Martin Erik Werner <[email protected]>

---
 setup.c               | 54 ++++++++++++++++++++++++++++++++-------------------
 t/t0060-path-utils.sh |  2 +-
 2 files changed, 35 insertions(+), 21 deletions(-)

setup.c char *prefix_path_gently(const char *prefix, int len, · line 25

Quoted 12 lines

setup.c +1 -3

index 6c3f85f..bec587e 100644
@@ -22,11 +22,41 @@ char *prefix_path_gently(const char *prefix, int len,
 	const char *orig = path;
 	char *sanitized;
 	if (is_absolute_path(orig)) {
-		const char *temp = real_path(path);
-		sanitized = xmalloc(len + strlen(temp) + 1);
-		strcpy(sanitized, temp);
+		char npath[strlen(path)];

Is this portable ?
This is variable-length array, isn't it ?
Using xmalloc() may be better

setup.c char *prefix_path_gently(const char *prefix, int len, · line 30

Quoted 5 lines

setup.c +3 -0

 		if (remaining_prefix)
 			*remaining_prefix = 0;
+		if (normalize_path_copy_len(npath, path, remaining_prefix))
+			return NULL;
+		const char *work_tree = get_git_work_tree();

declaration after statements should be avoided (not only here)

setup.c +13 -3

index 5432a31..0789a96 100644
@@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,
        const char *orig = path;
        char *sanitized;
        if (is_absolute_path(orig)) {
-               const char *temp = real_path(path);
-               sanitized = xmalloc(len + strlen(temp) + 1);
-               strcpy(sanitized, temp);
+               int i, match;
+               size_t wtpartlen;
+               char *npath, *wtpart;
+               struct string_list list = STRING_LIST_INIT_DUP;
+               const char *work_tree = get_git_work_tree();
+               if (!work_tree)
+                       return NULL;
+               npath = xmalloc(strlen(path) + 1);
                if (remaining_prefix)
                        *remaining_prefix = 0;
+               if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+                       free(npath);
+                       return NULL;
+               }
+

setup.c +9 -0

+                       while (i < list.nr) {
+                               strcat(wtpart, list.items[i++].string);
+                               if (strcmp(real_path(wtpart), work_tree) == 0) {
+                                       match = 1;
+                                       break;
+                               }
+                               strcat(wtpart, "/");
+                       }
+               }

setup.c +0 -0

        } else {
                sanitized = xmalloc(len + strlen(path) + 1);

if (len)

setup.c +1 -17

@@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,
                strcpy(sanitized + len, path);
                if (remaining_prefix)
                        *remaining_prefix = len;
-       }
-       if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
-               goto error_out;
-       if (is_absolute_path(orig)) {
-               size_t root_len, len, total;
-               const char *work_tree = get_git_work_tree();
-               if (!work_tree)
-                       goto error_out;
-               len = strlen(work_tree);
-               root_len = offset_1st_component(work_tree);
-               total = strlen(sanitized) + 1;
-               if (strncmp(sanitized, work_tree, len) ||
-                   (len > root_len && sanitized[len] != '\0' && sanitized[len] != '/')) {
-               error_out:
+               if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {
                        free(sanitized);
                        return NULL;
                }
-               if (sanitized[len] == '/')
-                       len++;
-               memmove(sanitized, sanitized + len, total - len);
        }
        return sanitized;
 }

In order to manipulate symliks in the
work tree using absolute paths, symlinks should only be dereferenced outside the work tree.

setup.c +44 -20

index 5432a31..0789a96 100644
@@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,
 	const char *orig = path;
 	char *sanitized;
 	if (is_absolute_path(orig)) {
-		const char *temp = real_path(path);
-		sanitized = xmalloc(len + strlen(temp) + 1);
-		strcpy(sanitized, temp);
+		int i, match;
+		size_t wtpartlen;
+		char *npath, *wtpart;
+		struct string_list list = STRING_LIST_INIT_DUP;
+		const char *work_tree = get_git_work_tree();
+		if (!work_tree)
+			return NULL;
+		npath = xmalloc(strlen(path) + 1);
 		if (remaining_prefix)
 			*remaining_prefix = 0;
+		if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+			free(npath);
+			return NULL;
+		}
+
+		string_list_split(&list, npath, '/', -1);
+		wtpart = xmalloc(strlen(npath) + 1);
+		i = 0;
+		match = 0;
+		strcpy(wtpart, list.items[i++].string);
+		strcat(wtpart, "/");
+		if (strcmp(real_path(wtpart), work_tree) == 0) {
+			match = 1;
+		} else {
+			while (i < list.nr) {
+				strcat(wtpart, list.items[i++].string);
+				if (strcmp(real_path(wtpart), work_tree) == 0) {
+					match = 1;
+					break;
+				}
+				strcat(wtpart, "/");
+			}
+		}
+		string_list_clear(&list, 0);
+		if (!match) {
+			free(npath);
+			free(wtpart);
+			return NULL;
+		}
+
+		wtpartlen = strlen(wtpart);
+		sanitized = xmalloc(strlen(npath) - wtpartlen);
+		strcpy(sanitized, npath + wtpartlen + 1);
+		free(npath);
+		free(wtpart);
 	} else {
 		sanitized = xmalloc(len + strlen(path) + 1);
 		if (len)

@@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,
 		strcpy(sanitized + len, path);
 		if (remaining_prefix)
 			*remaining_prefix = len;
-	}
-	if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
-		goto error_out;
-	if (is_absolute_path(orig)) {
-		size_t root_len, len, total;
-		const char *work_tree = get_git_work_tree();
-		if (!work_tree)
-			goto error_out;
-		len = strlen(work_tree);
-		root_len = offset_1st_component(work_tree);
-		total = strlen(sanitized) + 1;
-		if (strncmp(sanitized, work_tree, len) ||
-		    (len > root_len && sanitized[len] != '\0' && sanitized[len] != '/')) {
-		error_out:
+		if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {
 			free(sanitized);
 			return NULL;
 		}
-		if (sanitized[len] == '/')
-			len++;
-		memmove(sanitized, sanitized + len, total - len);
 	}
 	return sanitized;
 }

t/t0060-path-utils.sh +1 -1

index 3a0677a..03a12ac 100755
@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
 	test "$sym" = "$(test-path-utils real_path "$dir2/syml")"
 '
 
-test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with work tree symlinks' '
 
 	ln -s target symlink &&
 	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"

Quoted 3 lines · Martin Erik Werner

In order to manipulate symliks in the
work tree using absolute paths, symlinks should only be dereferenced outside the work tree.

I agree 100% with this reasoning (modulo s/symliks/symlinks/).

As to the implementation, it looks a bit overly complicated, though. I haven't tried writing the same myself, but

* I suspect that strbuf would help simplifying the allocation and deallocation;

* Also I suspect that use of string-list to split and then join is making the code unnecessarily complex. In Python/Perl, that would be a normal approach, but in C, it would be a lot simpler if you prepare a writable temporary in wtpart[], walk from left to right finding '/' and replacing it temporarily with NUL to terminate in order to check with real_path(), restore the NUL back to '/' to check deeper, and rinse and repeat.

Having said that, I am not absolutely sure if the repeated calls to real_path() are doing the right thing, though ;-)

Quoted 95 lines

setup.c +44 -20

index 5432a31..0789a96 100644
@@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,
    const char *orig = path;
    char *sanitized;
    if (is_absolute_path(orig)) {
-           const char *temp = real_path(path);
-           sanitized = xmalloc(len + strlen(temp) + 1);
-           strcpy(sanitized, temp);
+           int i, match;
+           size_t wtpartlen;
+           char *npath, *wtpart;
+           struct string_list list = STRING_LIST_INIT_DUP;
+           const char *work_tree = get_git_work_tree();
+           if (!work_tree)
+                   return NULL;
+           npath = xmalloc(strlen(path) + 1);
            if (remaining_prefix)
                    *remaining_prefix = 0;
+           if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+                   free(npath);
+                   return NULL;
+           }
+
+           string_list_split(&list, npath, '/', -1);
+           wtpart = xmalloc(strlen(npath) + 1);
+           i = 0;
+           match = 0;
+           strcpy(wtpart, list.items[i++].string);
+           strcat(wtpart, "/");
+           if (strcmp(real_path(wtpart), work_tree) == 0) {
+                   match = 1;
+           } else {
+                   while (i < list.nr) {
+                           strcat(wtpart, list.items[i++].string);
+                           if (strcmp(real_path(wtpart), work_tree) == 0) {
+                                   match = 1;
+                                   break;
+                           }
+                           strcat(wtpart, "/");
+                   }
+           }
+           string_list_clear(&list, 0);
+           if (!match) {
+                   free(npath);
+                   free(wtpart);
+                   return NULL;
+           }
+
+           wtpartlen = strlen(wtpart);
+           sanitized = xmalloc(strlen(npath) - wtpartlen);
+           strcpy(sanitized, npath + wtpartlen + 1);
+           free(npath);
+           free(wtpart);
    } else {
            sanitized = xmalloc(len + strlen(path) + 1);
            if (len)

@@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,
            strcpy(sanitized + len, path);
            if (remaining_prefix)
                    *remaining_prefix = len;
-   }
-   if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
-           goto error_out;
-   if (is_absolute_path(orig)) {
-           size_t root_len, len, total;
-           const char *work_tree = get_git_work_tree();
-           if (!work_tree)
-                   goto error_out;
-           len = strlen(work_tree);
-           root_len = offset_1st_component(work_tree);
-           total = strlen(sanitized) + 1;
-           if (strncmp(sanitized, work_tree, len) ||
-               (len > root_len && sanitized[len] != '\0' && sanitized[len] != '/')) {
-           error_out:
+           if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {
                    free(sanitized);
                    return NULL;
            }
-           if (sanitized[len] == '/')
-                   len++;
-           memmove(sanitized, sanitized + len, total - len);
    }
    return sanitized;
 }

t/t0060-path-utils.sh +0 -0

index 3a0677a..03a12ac 100755
@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
    test "$sym" = "$(test-path-utils real_path "$dir2/syml")"
 '
 

t/t0060-path-utils.sh test_expect_success SYMLINKS 'real path works on symlinks' ' · line 196

Quoted 5 lines

t/t0060-path-utils.sh +1 -1

-test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with work tree symlinks' '
 
    ln -s target symlink &&
    test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"

All this new code looks long enough to be a separate function with a meaningful name. And we could travese through each path component in npath without wtpart (replacing '/' with '\0' to terminate the string temporarily for real_path()). But it's up to you. Whichever way is easier to read to you.

Quoted 1 line · Torsten Bögershausen

On 2014-01-31 21.22, Martin Erik Werner wrote:

Quoted 23 lines

cache.h extern void verify_filename(const char *prefix, · line 429

Quoted 9 lines

cache.h +1 -0

index ce377e1..242f27d 100644
@@ -426,6 +426,7 @@ extern void verify_filename(const char *prefix,
                      int diagnose_misspelt_rev);
 extern void verify_non_filename(const char *prefix, const char *name);
 extern int path_inside_repo(const char *prefix, const char *path);
+extern int abspath_part_inside_repo(char *dst, const char *path);

abspath_part_inside_repo() is only used in setup.c, isn't it?
In this case we don't need it in cache.h, it can be declared inside setup.c as

static int abspath_part_inside_repo(char *dst, const char *path); (or "static inline" )

-----------------
(And not in this patch: see the final setup.c:)

        if (g) {
            free(npath);
            return NULL;
        }

Quoted 7 lines

If this is the only caller of abspath_part_inside_repo(), then do we need npath 2 times as a parameter ? Or can we re-write it to look like this:

static inline int abspath_part_inside_repo(char *path) [
]

I guess I've over-generalised it a bit too much, that should rather be done if-and-when, I presume?

It is indeed only used in setup.c and only by the prefix_path_gently function so static inline then?

Hmm, for single-parameter it should suffice to simply move the parameter down into the function, like so?:
const char* src;
src = dst;
and carry on as before (obviously also renaming the variables sensibly), or did you have something else in mind?

(I added two parameters since I was glancing at 'normalize_path_copy_len' for inspiration, and was thinking about (purely theoretical) re-use in other cases rather than minimizing it for the time being.)

What do you mean with the "(And not in this patch"... bit; what "final setup.c"?

On Sat, Feb 1, 2014 at 3:22 AM, Martin Erik Werner <[email protected]> wrote:

Quoted 1 line

+ // check root level

Um.. no C++ style comments. And there should be a test that work_tree is the prefix of src (common case). If so we can return early and do not need to do real_path() on every path component.

setup.c +5 -0

+               if (*path == '/') {
+                       *path = '\0';
+                       if (strcmp(real_path(path0), work_tree) == 0) {
+                               memmove(path0, path + 1, len - (path - path0));
+                               return 0;

setup.c +5 -1

+
+       return -1;
+}
+
+/*
  * Normalize "path", prepending the "prefix" for relative paths. If
  * remaining_prefix is not NULL, return the actual prefix still
  * remains in the path. For example, prefix = sub1/sub2/ and path is
--

1.8.5.2

Review

Quoted 9 lines · Martin Erik Werner

(quoted diff) +9 -0

+       /* check if work tree is already the prefix */
+       if (strncmp(path, work_tree, wtlen) == 0) {
+               if (path[wtlen] == '/')
+                       memmove(path, path + wtlen + 1, len - wtlen);
+               else
+                       /* work tree is the root, or the whole path */
+                       memmove(path, path + wtlen, len - wtlen + 1);
+               return 0;
+       }

No the 4th time is not the charm yet :) if path is "/abc/defghi" and work_tree is "/abc/def" you don't want to return "ghi" as the prefix here.

Review

Quoted 6 lines

(quoted diff) +6 -0

+       path0 = path;
+       path += offset_1st_component(path);
+
+       /* check each level */
+       while (*path != '\0') {
+               path++;

To me it looks like we could write

for (; *path; path++) {

or even

for (path += offset_1st_component(path); *path; path++) {

but it's personal taste..

Quoted 12 lines · Duy Nguyen

Review

Quoted 9 lines · Martin Erik Werner

(quoted diff) +9 -0

+       /* check if work tree is already the prefix */
+       if (strncmp(path, work_tree, wtlen) == 0) {
+               if (path[wtlen] == '/')
+                       memmove(path, path + wtlen + 1, len - wtlen);
+               else
+                       /* work tree is the root, or the whole path */
+                       memmove(path, path + wtlen, len - wtlen + 1);
+               return 0;
+       }

Quoted 3 lines

No the 4th time is not the charm yet :) if path is "/abc/defghi" and work_tree is "/abc/def" you don't want to return "ghi" as the prefix here.

Ah indeed, this should catch that:

setup.c +0 -0

index 2270bd4..5817875 100644

Quoted 19 lines · Martin Erik Werner

Quoted 12 lines · Duy Nguyen

Review

Quoted 9 lines · Martin Erik Werner

(quoted diff) +9 -0

+       /* check if work tree is already the prefix */
+       if (strncmp(path, work_tree, wtlen) == 0) {
+               if (path[wtlen] == '/')
+                       memmove(path, path + wtlen + 1, len - wtlen);
+               else
+                       /* work tree is the root, or the whole path */
+                       memmove(path, path + wtlen, len - wtlen + 1);
+               return 0;
+       }

Quoted 3 lines

No the 4th time is not the charm yet :) if path is "/abc/defghi" and work_tree is "/abc/def" you don't want to return "ghi" as the prefix here.

Ah indeed, this should catch that:

setup.c static inline int abspath_part_inside_repo(char *path) · line 35

Quoted 10 lines

setup.c +1 -1

index 2270bd4..5817875 100644
@@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)
 	if (strncmp(path, work_tree, wtlen) == 0) {
 		if (path[wtlen] == '/')
 			memmove(path, path + wtlen + 1, len - wtlen);
-		else
+		else if (path[wtlen - 1] == '/' || path[wtlen] == '\0')

Is wtlen guaranteed to be nonzero?

Quoted 21 lines · David Kastrup

Quoted 19 lines · Martin Erik Werner

Quoted 12 lines · Duy Nguyen

Review

Quoted 9 lines · Martin Erik Werner

(quoted diff) +9 -0

+       /* check if work tree is already the prefix */
+       if (strncmp(path, work_tree, wtlen) == 0) {
+               if (path[wtlen] == '/')
+                       memmove(path, path + wtlen + 1, len - wtlen);
+               else
+                       /* work tree is the root, or the whole path */
+                       memmove(path, path + wtlen, len - wtlen + 1);
+               return 0;
+       }

Quoted 3 lines

No the 4th time is not the charm yet :) if path is "/abc/defghi" and work_tree is "/abc/def" you don't want to return "ghi" as the prefix here.

Ah indeed, this should catch that:

Quoted 4 lines

Quoted 4 lines

setup.c +0 -0

index 2270bd4..5817875 100644

Quoted 8 lines

setup.c static inline int abspath_part_inside_repo(char *path) · line 35

Quoted 6 lines

setup.c +1 -1

@@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)
 	if (strncmp(path, work_tree, wtlen) == 0) {
 		if (path[wtlen] == '/')
 			memmove(path, path + wtlen + 1, len - wtlen);
-		else
+		else if (path[wtlen - 1] == '/' || path[wtlen] == '\0')

Is wtlen guaranteed to be nonzero?

Another comment:
The "raw" comparison with '/' is probably working well on all POSIX/Linux/Unix systems.

To be more portable, the macro
is_dir_sep()
can be used:

if (is_dir_sep(path[wtlen]))

Quoted 9 lines

Another comment:
The "raw" comparison with '/' is probably working well on all POSIX/Linux/Unix systems.

To be more portable, the macro
is_dir_sep()
can be used:

Quoted 1 line

if (is_dir_sep(path[wtlen]))

Since the path is already normalized by 'normalize_path_copy_len' which seems to guarantee '/'-separation, I have assumed that this was unnecessary?

setup.c +0 -0

index 2270bd4..5817875 100644

setup.c static int inside_git_dir = -1; · line 60

Quoted 5 lines

setup.c +5 -0

+       /* check whole path */
+       if (strcmp(real_path(path0), work_tree) == 0) {
+               *path0 = '\0';
+               return 0;
+       }

I think this is already handled by the "check if work tree is already the prefix" block.

When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead.

Quoted 3 lines · Martin Erik Werner

When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead.

The above may a very good description of the root cause, but can we have description of a symptom that is visible by end-users that is caused by the bug being fixed with the series here, by ending the above like so:

... link target instead. This causes "git foo bar" to misbehave in this and that way.

Testing the low-level underlying machinery like this patch does is not wrong per-se, but I suspect that this series was triggered by somebody noticing breakage in a end-user command "git foo $path" with $path being a full pathname to an in-tree symbolic link. It wouldn't be like somebody who was bored and ran "test-path-utils" for fun noticed the root cause without realizing how the fix would affect the behaviour that would be visible to end-users, right?

Can we have that "git foo $path" to the testsuite as well? That is the breakage we do not want to repeat in the future by regressing.

I am guessing "foo" is "add", but I wasn't closely following the progression of the series.

Thanks.

I've done a bit more digging into this: The issue applies to pretty much all commands which can be given paths to files which are present in the work tree, so add, cat-file, rev-list, etc.

From 910d8c9f51c3b3f2c03dbf15ce3cf7ea94de8d27 Mon Sep 17 00:00:00 2001

Can we have that "git foo $path" to the testsuite as well? That is the breakage we do not want to repeat in the future by regressing.

The path being exactly equal to the work tree is handled separately, since then there is no directory separator between the work tree and in-repo part.

/*
+ * No checking if the path is in fact an absolute path is done, and it must
+ * already be normalized.

+ * Find the part of an absolute path that lies inside the work tree by
+ * dereferencing symlinks outside the work tree, for example:

+ * /dir1/repo/dir2/file   (work tree is /dir1/repo)      -> dir2/file
+ * /dir/file              (work tree is /)               -> dir/file

+ * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2
+ * /dir/repolink/file (repolink points to /dir/repo) -> file
+ * /dir/repo (exactly equal to work tree) -> (empty string)
+ */
+static inline int abspath_part_inside_repo(char *path)

setup.c +31 -0

+			return 0;
+		}
+		/* work tree might match beginning of a symlink to work tree */
+		off = wtlen;
+	}
+	path0 = path;
+	path += offset_1st_component(path) + off;
+
+	/* check each level */
+	while (*path) {
+		path++;
+		if (*path == '/') {
+			*path = '\0';
+			if (strcmp(real_path(path0), work_tree) == 0) {
+				memmove(path0, path + 1, len - (path - path0));
+				return 0;
+			}
+			*path = '/';
+		}
+	}
+
+	/* check whole path */
+	if (strcmp(real_path(path0), work_tree) == 0) {
+		*path0 = '\0';
+		return 0;
+	}
+
+	return -1;
+}
+
+/*
  * Normalize "path", prepending the "prefix" for relative paths. If
  * remaining_prefix is not NULL, return the actual prefix still
  * remains in the path. For example, prefix = sub1/sub2/ and path is

Quoted 2 lines · Junio C Hamano

Can we have that "git foo $path" to the testsuite as well? That is the breakage we do not want to repeat in the future by regressing.

Something like this, perhaps?

 t/t3004-ls-files-basic.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

t/t3004-ls-files-basic.sh +8 -0

index 8d9bc3c..d93089d 100755
@@ -36,4 +36,21 @@ test_expect_success 'ls-files -h in corrupt repository' '
 	test_i18ngrep "[Uu]sage: git ls-files " broken/usage
 '
 
+test_expect_success SYMLINKS 'ls-files with symlinks' '
+	mkdir subs &&
+	ln -s nosuch link &&
+	ln -s ../nosuch subs/link &&
+	git add link subs/link &&
+	git ls-files -s link subs/link >expect &&
+	git ls-files -s "$(pwd)/link" "$(pwd)/subs/link" >actual &&
+	test_cmp expect actual &&

Quoted 3 lines · Martin Erik Werner

The path being exactly equal to the work tree is handled separately, since then there is no directory separator between the work tree and in-repo part.

What is an "in-repo part"? Whatever it is, I am not sure if I follow that logic. After the while (*path) loop checks each level, you check the whole path---wouldn't that code handle that special case already?

Because it is probably the normal case not to have any symbolic link in the leading part (hence not having to dereference them), I think checking "is work_tree[] a prefix of path[]" early is justified from performance point of view, though.

Quoted 3 lines

/*
+ * No checking if the path is in fact an absolute path is done, and it must
+ * already be normalized.

This is not quite parsable to me.

Quoted 9 lines

+ * Find the part of an absolute path that lies inside the work tree by
+ * dereferencing symlinks outside the work tree, for example:

+ * /dir1/repo/dir2/file   (work tree is /dir1/repo)      -> dir2/file
+ * /dir/file              (work tree is /)               -> dir/file

+ * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2
+ * /dir/repolink/file (repolink points to /dir/repo) -> file
+ * /dir/repo (exactly equal to work tree) -> (empty string)
+ */
+static inline int abspath_part_inside_repo(char *path)

It looks a bit too big to be marked "inline"; better leave it to the compiler to notice that there is only a single callsite and decide to (or not to) inline it.

Quoted 6 lines

setup.c +6 -0

+{
+	size_t len;
+	size_t wtlen;
+	char *path0;
+	int off;
+

setup.c static int inside_git_dir = -1; · line 31

Quoted 5 lines

setup.c +5 -0

+	const char* work_tree = get_git_work_tree();
+	if (!work_tree)
+		return -1;
+	wtlen = strlen(work_tree);
+	len = strlen(path);

I expect that this is called from a callsite that _knows_ how long path[] is. Shouldn't len be a parameter to this function instead?

setup.c static int inside_git_dir = -1; · line 35

Quoted 4 lines

setup.c +4 -0

+	off = 0;
+
+	/* check if work tree is already the prefix */
+	if (strncmp(path, work_tree, wtlen) == 0) {

I wonder if we want to be explicit and compare wtlen and len before even attempting strncmp(). If work_tree is longer than path, it cannot be a prefix of path, right?

In other words, also with a style-fix to prefer !XXcmp() over XXcmp() == 0:

	if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {

perhaps? That would make it clear why referring to path[wtlen] on the next line is permissible (it is obvious that it won't access past the end of path[]):

setup.c static int inside_git_dir = -1; · line 41

Quoted 6 lines

setup.c +6 -0

+		if (path[wtlen] == '/') {
+			memmove(path, path + wtlen + 1, len - wtlen);
+			return 0;
+		} else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
+			/* work tree is the root, or the whole path */
+			memmove(path, path + wtlen, len - wtlen + 1);

If work_tree[] == "/", path[] == "/a", then len == 2, wtlen == 1, path[wtlen - 1] == '/' and this shifts path[] to the left by one, leaving path[] = "a", which is what we want. OK.

If work_tree[] == path[] == "/a", then len == wtlen == 2, path[wtlen] == '\0', and this becomes equivalent to path[0] = '\0'. Hmph.... How do our callers treat an empty path? In other words, should these three be equivalent?

	cd /a && git ls-files /a
	cd /a && git ls-files ""
	cd /a && git ls-files .

Quoted 34 lines

setup.c +31 -0

+			return 0;
+		}
+		/* work tree might match beginning of a symlink to work tree */
+		off = wtlen;
+	}
+	path0 = path;
+	path += offset_1st_component(path) + off;
+
+	/* check each level */
+	while (*path) {
+		path++;
+		if (*path == '/') {
+			*path = '\0';
+			if (strcmp(real_path(path0), work_tree) == 0) {
+				memmove(path0, path + 1, len - (path - path0));
+				return 0;
+			}
+			*path = '/';
+		}
+	}
+
+	/* check whole path */
+	if (strcmp(real_path(path0), work_tree) == 0) {
+		*path0 = '\0';
+		return 0;
+	}
+
+	return -1;
+}
+
+/*
  * Normalize "path", prepending the "prefix" for relative paths. If
  * remaining_prefix is not NULL, return the actual prefix still
  * remains in the path. For example, prefix = sub1/sub2/ and path is

Will you add that test or should I place it in the series with you as author?

Quoted 6 lines

Quoted 3 lines · Martin Erik Werner

The path being exactly equal to the work tree is handled separately, since then there is no directory separator between the work tree and in-repo part.

Quoted 4 lines

What is an "in-repo part"? Whatever it is, I am not sure if I follow that logic. After the while (*path) loop checks each level, you check the whole path---wouldn't that code handle that special case already?

Given "/dir1/repo/dir2/file" I've used 'in-repo' to refer to "dir2/file", sometimes interchangably with "part inside the work tree" which might be better terminology, and should replace it?

I was trying to convey that if path is simply "/dir/repo", then the while loop method of replacing a '/' and checking from the beginning won't work for the last level, since it has no terminating '/' to replace, so hence it's a special case, mentioning the "part inside the work tree" is arguably confusing in that case, since there isn't really one, maybe it should be left out completely, since the "check each level" explanation covers it already?

Quoted 5 lines

Because it is probably the normal case not to have any symbolic link in the leading part (hence not having to dereference them), I think checking "is work_tree[] a prefix of path[]" early is justified from performance point of view, though.

Quoted 5 lines

Quoted 3 lines

/*
+ * No checking if the path is in fact an absolute path is done, and it must
+ * already be normalized.

This is not quite parsable to me.

Hmm, what about
The input parameter must contain an absolute path, and it must already be normalized.

Quoted 8 lines

setup.c static int inside_git_dir = -1; · line 31

Quoted 5 lines

setup.c +5 -0

+	const char* work_tree = get_git_work_tree();
+	if (!work_tree)
+		return -1;
+	wtlen = strlen(work_tree);
+	len = strlen(path);

I expect that this is called from a callsite that _knows_ how long path[] is. Shouldn't len be a parameter to this function instead?

Currently, it actually doesn't, since 'normalize_path_copy_len' is called on it prior, which can mess with the string length.

Quoted 6 lines

Hmph.... How do our callers treat an empty path? In other words, should these three be equivalent?

	cd /a && git ls-files /a
	cd /a && git ls-files ""
	cd /a && git ls-files .

Here I have only gone by the assumption that previous code did the right thing,...

Martin Erik Werner <[email protected]> writes:

Quoted 7 lines

I was trying to convey that if path is simply "/dir/repo", then the while loop method of replacing a '/' and checking from the beginning won't work for the last level, since it has no terminating '/' to replace, so hence it's a special case, mentioning the "part inside the work tree" is arguably confusing in that case, since there isn't really one, maybe it should be left out completely, since the "check each level" explanation covers it already?

I dunno about the explanation, but it still looks strange to have the special case to deal with "/dir/repo" before you enter the while loop, and then also have code immediately after the loop that seems to handle the same case. Isn't the latter one redundant?

setup.c +5 -3

index a2e60ab..230505c 100644
@@ -86,11 +86,23 @@ char *prefix_path_gently(const char *prefix, int len,
        const char *orig = path;
        char *sanitized;
        if (is_absolute_path(orig)) {
-               const char *temp = real_path(path);
-               sanitized = xmalloc(len + strlen(temp) + 1);
-               strcpy(sanitized, temp);
+               char *npath;
+
+               npath = xmalloc(strlen(path) + 1);
                if (remaining_prefix)
                        *remaining_prefix = 0;
+               if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+                       free(npath);

setup.c +0 -0

        } else {
                sanitized = xmalloc(len + strlen(path) + 1);
                if (len)

Quoted 18 lines · Martin Erik Werner

setup.c +5 -3

index a2e60ab..230505c 100644
@@ -86,11 +86,23 @@ char *prefix_path_gently(const char *prefix, int len,
        const char *orig = path;
        char *sanitized;
        if (is_absolute_path(orig)) {
-               const char *temp = real_path(path);
-               sanitized = xmalloc(len + strlen(temp) + 1);
-               strcpy(sanitized, temp);
+               char *npath;
+
+               npath = xmalloc(strlen(path) + 1);
                if (remaining_prefix)
                        *remaining_prefix = 0;
+               if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+                       free(npath);

setup.c char *prefix_path_gently(const char *prefix, int len, · line 105

Quoted 10 lines

setup.c +10 -0

+                       return NULL;
+               }
+               if (abspath_part_inside_repo(npath)) {
+                       free(npath);
+                       return NULL;
+               }
+
+               sanitized = xmalloc(strlen(npath) + 1);
+               strcpy(sanitized, npath);
+               free(npath);

We could replace these three lines with "sanitized = npath;". But it's not a big deal imo. The rest of the series looks good.

Reviewed-by Duy Nguyen <[email protected]>

Quoted 3 lines

setup.c +0 -0

        } else {
                sanitized = xmalloc(len + strlen(path) + 1);
                if (len)

Signature · 1 line

Duy

Then it seems like one could get rid of npath completely:

setup.c +5 -9

index 230505c..dd120cd 100644
@@ -88,21 +88,17 @@ char *prefix_path_gently(const char *prefix, int len,
 	if (is_absolute_path(orig)) {
 		char *npath;
 
-		npath = xmalloc(strlen(path) + 1);
+		sanitized = xmalloc(strlen(path) + 1);
 		if (remaining_prefix)
 			*remaining_prefix = 0;
-		if (normalize_path_copy_len(npath, path, remaining_prefix)) {
-			free(npath);
+		if (normalize_path_copy_len(sanitized, path, remaining_prefix)) {
+			free(sanitized);
 			return NULL;
 		}
-		if (abspath_part_inside_repo(npath)) {
-			free(npath);
+		if (abspath_part_inside_repo(sanitized)) {
+			free(sanitized);
 			return NULL;
 		}
-
-		sanitized = xmalloc(strlen(npath) + 1);
-		strcpy(sanitized, npath);
-		free(npath);
 	} else {
 		sanitized = xmalloc(len + strlen(path) + 1);
 		if (len)

at the cost of 'sanitized' always being the length of path, regardless if it's shorter, or even a NUL string.

--
Martin Erik Werner <[email protected]>

 t/t0060-path-utils.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

t/t0060-path-utils.sh +0 -0

index b8e92e1..c0a14f6 100755
@@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '
 	test_cmp expected actual
 '

Quoted 11 lines · Martin Erik Werner

 t/t0060-path-utils.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

t/t0060-path-utils.sh +0 -0

index b8e92e1..c0a14f6 100755
@@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '
 	test_cmp expected actual
 '
 

t/t0060-path-utils.sh test_expect_success 'prefix_path works with only absolute path to work tree' ' · line 212

Quoted 9 lines

t/t0060-path-utils.sh +9 -0

+test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' '
+	test_must_fail test-path-utils prefix_path prefix "$(pwd)a"
+'
+
+test_expect_success 'prefix_path works with absolute path to a symlink to work tree having  same beginning as work tree' '
+	git init repo &&
+	ln -s repo repolink &&
+	test "a" = "$(cd repo && test-path-utils prefix_path prefix "$(pwd)/../repolink/a")"
+'

I think we need SYMLINKS here.

In order to extract the part of an absolute path which lies inside the repo, it is not possible to directly use real_path, since that would dereference symlinks both outside and inside the work tree.

Add an 'abspath_part_inside_repo' function which incrementally checks each path level by temporarily NUL-terminating at each '/' and comparing against the work tree path. When a match is found, it copies the remainder (which will be the in-repo part) to a destination buffer.

The path being the filesystem root or exactly equal to the work tree are special cases handled separately, since then there is no directory separator between the work tree and in-repo part.

Signed-off-by Martin Erik Werner <[email protected]>

---
 cache.h |  1 +
 setup.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

cache.h +0 -0

index ce377e1..242f27d 100644

Quoted 25 lines · Martin Erik Werner

In order to extract the part of an absolute path which lies inside the repo, it is not possible to directly use real_path, since that would dereference symlinks both outside and inside the work tree.

Add an 'abspath_part_inside_repo' function which incrementally checks each path level by temporarily NUL-terminating at each '/' and comparing against the work tree path. When a match is found, it copies the remainder (which will be the in-repo part) to a destination buffer.

The path being the filesystem root or exactly equal to the work tree are special cases handled separately, since then there is no directory separator between the work tree and in-repo part.

Signed-off-by Martin Erik Werner <[email protected]>

---
 cache.h |  1 +
 setup.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

cache.h +0 -0

index ce377e1..242f27d 100644

cache.h extern void verify_filename(const char *prefix, · line 429

Quoted 4 lines

cache.h +1 -0

@@ -426,6 +426,7 @@ extern void verify_filename(const char *prefix,
 			    int diagnose_misspelt_rev);
 extern void verify_non_filename(const char *prefix, const char *name);
 extern int path_inside_repo(const char *prefix, const char *path);
+extern int abspath_part_inside_repo(char *dst, const char *path);

abspath_part_inside_repo() is only used in setup.c, isn't it?
In this case we don't need it in cache.h, it can be declared inside setup.c as

static int abspath_part_inside_repo(char *dst, const char *path); (or "static inline" )

-----------------
(And not in this patch: see the final setup.c:)

        if (g) {
            free(npath);
            return NULL;
        }

If this is the only caller of abspath_part_inside_repo(), then do we need npath 2 times as a parameter ? Or can we re-write it to look like this:

static inline int abspath_part_inside_repo(char *path) [
]

setup.c +15 -0

index 5432a31..e606846 100644
@@ -77,6 +77,69 @@ int path_inside_repo(const char *prefix, const char *path)
        return 0;
 }
 
+/*
+ * It is ok if dst == src, but they should not overlap otherwise.
+ * No checking if the path is in fact an absolute path is done, and it must
+ * already be normalized.
+ *
+ * Find the part of an absolute path that lies inside the work tree by
+ * dereferencing symlinks outside the work tree, for example:
+ * /dir1/repo/dir2/file   (work tree is /dir1/repo)      -> dir2/file
+ * /dir/file              (work tree is /)               -> dir/file
+ * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2
+ * /dir/repo              (exactly equal to work tree)   -> (empty string)
+ */
+int abspath_part_inside_repo(char *dst, const char* src)
+{
+       size_t len;

setup.c +38 -1

+       if (has_dos_drive_prefix(src)) {
+               *dst++ = *src++;
+               *dst++ = *src++;
+               *dst++ = *src++;
+       } else {
+               *dst++ = *src++;
+       }
+       temp = *dst;
+       *dst = '\0';
+       if (strcmp(real_path(dst0), work_tree) == 0) {
+               *dst = temp;
+               memmove(dst0, src, len - (dst - dst0) + 1);
+               return 0;
+       }
+       *dst = temp;
+
+       // check each level
+       while (*dst != '\0') {
+               *dst++ = *src++;
+               if (*dst == '/') {
+                       *dst = '\0';
+                       if (strcmp(real_path(dst0), work_tree) == 0) {
+                               memmove(dst0, src + 1, len - (dst - dst0));
+                               return 0;
+                       }
+                       *dst = '/';
+               }
+       }
+
+       // check whole path
+       if (strcmp(real_path(dst0), work_tree) == 0) {
+               *dst0 = '\0';
+               return 0;
+       }
+
+       return -1;
+}
+
 int check_filename(const char *prefix, const char *arg)
 {
        const char *name;
--

1.8.5.2