git-mv with absolute path derefereces symlinks

git-mv with absolute path derefereces symlinks

[Martin Erik Werner]

Hi,

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.

###
$ git init linktest
Initialized empty Git repository in /home/arand/tmp/linktest/.git/
$ cd linktest/
$ touch target
$ ln -s target link
$ ln -s /tmp/target link2
$ git add .
$ git commit -m1
[master (root-commit) 3cfea66] 1
 3 files changed, 2 insertions(+)
 create mode 120000 link
 create mode 120000 link2
 create mode 100644 target
$ git mv "$(pwd)/link" "$(pwd)/moved"
$ git status
On branch master
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        renamed:    target -> moved

$ git mv "$(pwd)/link2" "$(pwd)/moved2"
fatal: /home/arand/tmp/linktest/link2: '/home/arand/tmp/linktest/link2' is outside repository
###

--
Martin Erik Werner <martinerikwerner@gmail.com>

[PATCH 0/2] in-tree symlink handling with absolute paths

[Martin Erik Werner]

On Wed, 2014-01-15 at 13:48 +0100, Martin Erik Werner wrote:
> 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.

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.

I've written a test and a fix which seems to do the right thing.

Martin Erik Werner (2):
      t0060: Add test for manipulating symlinks via absolute paths
      setup: Don't dereference in-tree symlinks for absolute paths

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

[PATCH 1/2] t0060: Add test for manipulating symlinks via absolute paths

[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.

This is a regression introduced by 18e051a:
  setup: translate symlinks in filename when using absolute paths
(which did not take symlinks in the work tree into consideration).

Add a known-breakage tests using the prefix_path function, which
currently uses real_path, causing the dereference.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t0060-path-utils.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 07c10c8..3a0677a 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,6 +190,13 @@ 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' '
+
+	ln -s target symlink &&
+	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
+
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
-- 
1.8.5.2

[PATCH 2/2] setup: Don't dereference in-tree symlinks for absolute paths

[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 <martinerikwerner@gmail.com>
---
 setup.c               | 54 ++++++++++++++++++++++++++++++++-------------------
 t/t0060-path-utils.sh |  2 +-
 2 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/setup.c b/setup.c
index 6c3f85f..bec587e 100644
--- a/setup.c
+++ b/setup.c
@@ -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)];
 		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();
+		if (!work_tree)
+			return NULL;
+		struct string_list list = STRING_LIST_INIT_DUP;
+		string_list_split(&list, npath, '/', -1);
+
+		char wtpart[strlen(npath) + 1];
+		int i = 0;
+		int match = 0;
+		strcpy(wtpart, list.items[i++].string);
+		strcpy(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)
+			return NULL;
+
+		size_t wtpartlen = strlen(wtpart);
+		sanitized = xmalloc(strlen(npath) - wtpartlen);
+		strcpy(sanitized, npath + wtpartlen + 1);
 	} else {
 		sanitized = xmalloc(len + strlen(path) + 1);
 		if (len)
@@ -34,26 +64,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;
 }
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 3a0677a..03a12ac 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -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"
-- 
1.8.5.2

Re: [PATCH 2/2] setup: Don't dereference in-tree symlinks for absolute paths

[Torsten Bögershausen]

On 2014-01-26 15.22, Martin Erik Werner wrote:
> 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 <martinerikwerner@gmail.com>
> ---
>  setup.c               | 54 ++++++++++++++++++++++++++++++++-------------------
>  t/t0060-path-utils.sh |  2 +-
>  2 files changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/setup.c b/setup.c
> index 6c3f85f..bec587e 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -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
>  		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)

Re: [PATCH 2/2] setup: Don't dereference in-tree symlinks for absolute paths

[Martin Erik Werner]

On Sun, Jan 26, 2014 at 06:19:25PM +0100, Torsten Bögershausen wrote:
> On 2014-01-26 15.22, Martin Erik Werner wrote:
> > 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 <martinerikwerner@gmail.com>
> > ---
> >  setup.c               | 54 ++++++++++++++++++++++++++++++++-------------------
> >  t/t0060-path-utils.sh |  2 +-
> >  2 files changed, 35 insertions(+), 21 deletions(-)
> > 
> > diff --git a/setup.c b/setup.c
> > index 6c3f85f..bec587e 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -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
Ah, right, that looks bad now that you mention it.
> >  		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)
> 
Indeed, I somehow guessed that declaration-after-statement was ok and
tried to keep them close to usage, bad guess, evidently.

I've rerolled the last v1 patch accordingly:
* Use xmalloc() when initializing char* from strlen()
* Separate and move declarations to beginning of scope
* Fix a strcpy that should've been a strcat (which would've nuked DOS
  prefixes, I think)

  [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths

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

--
Martin Erik Werner <martinerikwerner@gmail.com>

[PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths

[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 <martinerikwerner@gmail.com>
---
 setup.c               | 64 +++++++++++++++++++++++++++++++++++----------------
 t/t0060-path-utils.sh |  2 +-
 2 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/setup.c b/setup.c
index 5432a31..0789a96 100644
--- a/setup.c
+++ b/setup.c
@@ -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;
 }
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 3a0677a..03a12ac 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -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"
-- 
1.8.5.2

Re: [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths

[Duy Nguyen]

On Mon, Jan 27, 2014 at 7:07 AM, Martin Erik Werner
<martinerikwerner@gmail.com> wrote:
> diff --git a/setup.c b/setup.c
> index 5432a31..0789a96 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -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 {

Could we remove this part and let the while loop handle the first path
component too? The only difference I see is if this code matches, we
have a trailing slash, while the "while" loop does not have a trailing
slash in wtpart.

> +                       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);

This "+ 1" is to ignore '/', isn't it? If so we should not do if match
is set 1 outside "while" loop.

> +               free(npath);
> +               free(wtpart);

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.

>         } 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;
>  }
-- 
Duy

Re: [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths

[Junio C Hamano]

Martin Erik Werner <martinerikwerner@gmail.com> writes:

> 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 ;-)

> diff --git a/setup.c b/setup.c
> index 5432a31..0789a96 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -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;
>  }
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index 3a0677a..03a12ac 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -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"

[PATCH v3 0/4] setup: Don't dereference in-tree symlinks for absolute paths

[Martin Erik Werner]

On Mon, Jan 27, 2014 at 08:31:37AM -0800, Junio C Hamano wrote:
> Martin Erik Werner <martinerikwerner@gmail.com> writes:
> 
> > 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 ;-)
> 
> > diff --git a/setup.c b/setup.c
> > index 5432a31..0789a96 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -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;
> >  }
> > diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> > index 3a0677a..03a12ac 100755
> > --- a/t/t0060-path-utils.sh
> > +++ b/t/t0060-path-utils.sh
> > @@ -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"

On Mon, Jan 27, 2014 at 07:49:42AM +0700, Duy Nguyen wrote:
(...)
> 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.
(...)

I've reworked my patchset to add a specialised 'abspath_part_inside_repo'
function which does traversing and temporary NUL-separation as suggested.

I've also added a new test for a regression which I managed to hit.

I'm not particularly happy with the name of the function, but since
path_inside_repo already exists for something quite different, it seemed
important to distinguish it.

I've not added any test cases specific to the function, since some of it is
already covered thought prefix_path, and some things I'm unsure if possible to
test sanely (e.g. work tree is /).

Repeatedly calling 'real_path' feels somewhat clunky, but I haven't managed
to come up with an alternative method that would work, since symlinks change
the path arbitrarily there's no way to separate the result of dereferencing two
symlinks at once.

Martin Erik Werner (4):
  t0060: Add test for manipulating symlinks via absolute paths
  t0060: Add test for prefix_path when path == work tree
  setup: Add 'abspath_part_inside_repo' function
  setup: Don't dereference in-tree symlinks for absolute paths

 cache.h               |  1 +
 setup.c               | 99 ++++++++++++++++++++++++++++++++++++++++-----------
 t/t0060-path-utils.sh | 11 ++++++
 3 files changed, 91 insertions(+), 20 deletions(-)

-- 
1.8.5.2

[PATCH v3 1/4] t0060: Add test for manipulating symlinks via absolute paths

[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.

This is a regression introduced by 18e051a:
  setup: translate symlinks in filename when using absolute paths
(which did not take symlinks in the work tree into consideration).

Add a known-breakage tests using the prefix_path function, which
currently uses real_path, causing the dereference.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t0060-path-utils.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 07c10c8..0bba988 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,6 +190,11 @@ 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 absolute paths to work tree symlinks' '
+	ln -s target symlink &&
+	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
-- 
1.8.5.2

[PATCH v3 2/4] t0060: Add test for prefix_path when path == work tree

[Martin Erik Werner]

The current behaviour of prefix_path is to return an empty string if
prefixing and absolute path that only contains exactly the work tree.
This behaviour is a potential regression point.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t0060-path-utils.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 0bba988..b8e92e1 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree
 	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
 '
 
+test_expect_success 'prefix_path works with only absolute path to work tree' '
+	echo "" >expected &&
+	test-path-utils prefix_path prefix "$(pwd)" >actual &&
+	test_cmp expected actual
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
-- 
1.8.5.2

[PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function

[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 <martinerikwerner@gmail.com>
---
 cache.h |  1 +
 setup.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/cache.h b/cache.h
index ce377e1..242f27d 100644
--- a/cache.h
+++ b/cache.h
@@ -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);
 
 #define INIT_DB_QUIET 0x0001
 
diff --git a/setup.c b/setup.c
index 5432a31..e606846 100644
--- a/setup.c
+++ b/setup.c
@@ -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;
+	char *dst0;
+	char temp;
+
+	const char* work_tree = get_git_work_tree();
+	if (!work_tree)
+		return -1;
+	len = strlen(src);
+	dst0 = dst;
+
+	// check root level
+	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

[PATCH v3 4/4] setup: Don't dereference in-tree symlinks for absolute paths

[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 symlinks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.

Modify the 'prefix_path_gently' to first normalize the path in order to
make sure path levels are separated by '/', then pass the result to
'abspath_part_inside_repo' to find the in-repo part (without dereferencing
any symlinks inside the work tree).

For absolute paths, 'prefix_path_gently' did not, nor does now do, any
actual prefixing, hence the result from 'abspath_part_in_repo' is
returned as-is.

Fixes t0060-82.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 setup.c               | 36 ++++++++++++++++--------------------
 t/t0060-path-utils.sh |  2 +-
 2 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/setup.c b/setup.c
index e606846..2e65b48 100644
--- a/setup.c
+++ b/setup.c
@@ -22,11 +22,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);
+			return NULL;
+		}
+		if (abspath_part_inside_repo(npath, npath)) {
+			free(npath);
+			return NULL;
+		}
+
+		sanitized = xmalloc(strlen(npath) + 1);
+		strcpy(sanitized, npath);
+		free(npath);
 	} else {
 		sanitized = xmalloc(len + strlen(path) + 1);
 		if (len)
@@ -34,26 +46,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;
 }
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index b8e92e1..f6f378b 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -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 absolute paths to work tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '
 	ln -s target symlink &&
 	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
 '
-- 
1.8.5.2

Re: [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function

[Torsten Bögershausen]

On 2014-01-31 21.22, Martin Erik Werner wrote:
> 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 <martinerikwerner@gmail.com>
> ---
>  cache.h |  1 +
>  setup.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index ce377e1..242f27d 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -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)
[
]

Re: [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function

[Martin Erik Werner]

On Fri, Jan 31, 2014 at 11:37:29PM +0100, Torsten Bögershausen wrote:
> On 2014-01-31 21.22, Martin Erik Werner wrote:
> > 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 <martinerikwerner@gmail.com>
> > ---
> >  cache.h |  1 +
> >  setup.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 64 insertions(+)
> >
> > diff --git a/cache.h b/cache.h
> > index ce377e1..242f27d 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -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)
> [
> ]

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"?

--
Martin Erik Werner <martinerikwerner@gmail.com>

Re: [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function

[Duy Nguyen]

On Sat, Feb 1, 2014 at 3:22 AM, Martin Erik Werner
<martinerikwerner@gmail.com> wrote:
> diff --git a/setup.c b/setup.c
> index 5432a31..e606846 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -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;
> +       char *dst0;
> +       char temp;
> +
> +       const char* work_tree = get_git_work_tree();
> +       if (!work_tree)
> +               return -1;
> +       len = strlen(src);
> +       dst0 = dst;
> +
> +       // 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.

> +       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



-- 
Duy

[PATCH v4 0/4] Handling of in-tree symlinks for absolute paths

[Martin Erik Werner]

Hmm, maybe fourth time's the ch...nevermind.

On Sat, Feb 01, 2014 at 02:31:21AM +0100, Martin Erik Werner wrote:
> On Fri, Jan 31, 2014 at 11:37:29PM +0100, Torsten Bögershausen wrote:
> > On 2014-01-31 21.22, Martin Erik Werner wrote:
(...)
> > > diff --git a/cache.h b/cache.h
> > > index ce377e1..242f27d 100644
> > > --- a/cache.h
> > > +++ b/cache.h
> > > @@ -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)
> > [
> > ]
> 
> 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"?

As per Torsten's suggestions I've re-worked abspath_part_inside_repo function
to only take one parameter and also put it as 'static inline' before
'prefix_path_gently' since currently only used once. (The change turned
out larger/nicer than I first guessed, since the 'src' pointer and copying
could be dropped completely.)

On Sat, Feb 01, 2014 at 09:31:26AM +0700, Duy Nguyen wrote:
> On Sat, Feb 1, 2014 at 3:22 AM, Martin Erik Werner
> <martinerikwerner@gmail.com> wrote:
(...)
> > +       // 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.
(...)

Oops, comments fixed.

I've added the check for work tree as existing prefix, which also had the nice
side-effect of checking the case of the work tree being the root of the
filesystem as a bonus.

This new single-buffer version also uses 'offset_1st_component' to move past
the root (since not having to worry about copying).

Martin Erik Werner (4):
  t0060: Add test for manipulating symlinks via absolute paths
  t0060: Add test for prefix_path when path == work tree
  setup: Add 'abspath_part_inside_repo' function
  setup: Don't dereference in-tree symlinks for absolute paths

 setup.c               | 93 ++++++++++++++++++++++++++++++++++++++++-----------
 t/t0060-path-utils.sh | 11 ++++++
 2 files changed, 84 insertions(+), 20 deletions(-)

-- 
1.8.5.2

[PATCH v4 1/4] t0060: Add test for manipulating symlinks via absolute paths

[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.

This is a regression introduced by 18e051a:
  setup: translate symlinks in filename when using absolute paths
(which did not take symlinks in the work tree into consideration).

Add a known-breakage tests using the prefix_path function, which
currently uses real_path, causing the dereference.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t0060-path-utils.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 07c10c8..0bba988 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,6 +190,11 @@ 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 absolute paths to work tree symlinks' '
+	ln -s target symlink &&
+	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
-- 
1.8.5.2

[PATCH v4 2/4] t0060: Add test for prefix_path when path == work tree

[Martin Erik Werner]

The current behaviour of prefix_path is to return an empty string if
prefixing and absolute path that only contains exactly the work tree.
This behaviour is a potential regression point.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t0060-path-utils.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 0bba988..b8e92e1 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree
 	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
 '
 
+test_expect_success 'prefix_path works with only absolute path to work tree' '
+	echo "" >expected &&
+	test-path-utils prefix_path prefix "$(pwd)" >actual &&
+	test_cmp expected actual
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
-- 
1.8.5.2

[PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function

[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 first checks if the
work tree is already the prefix, then incrementally checks each path
level by temporarily NUL-terminating at each '/' and comparing against
the work tree path. If a match is found, it overwrites the input path
with the remainder past the work tree (which will be the in-repo part).

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.

This function is currently only intended for use in
'prefix_path_gently'.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 setup.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/setup.c b/setup.c
index 5432a31..e938785 100644
--- a/setup.c
+++ b/setup.c
@@ -6,6 +6,63 @@ static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
 /*
+ * 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)
+ */
+static inline int abspath_part_inside_repo(char *path)
+{
+	size_t len;
+	size_t wtlen;
+	char *path0;
+
+	const char* work_tree = get_git_work_tree();
+	if (!work_tree)
+		return -1;
+	wtlen = strlen(work_tree);
+	len = strlen(path);
+
+	/* 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;
+	}
+	path0 = path;
+	path += offset_1st_component(path);
+
+	/* check each level */
+	while (*path != '\0') {
+		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
-- 
1.8.5.2

[PATCH v4 4/4] setup: Don't dereference in-tree symlinks for absolute paths

[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 symlinks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.

Modify the 'prefix_path_gently' to first normalize the path in order to
make sure path levels are separated by '/', then pass the result to
'abspath_part_inside_repo' to find the in-repo part (without dereferencing
any symlinks inside the work tree).

For absolute paths, 'prefix_path_gently' did not, nor does now do, any
actual prefixing, hence the result from 'abspath_part_in_repo' is
returned as-is.

Fixes t0060-82.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 setup.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/setup.c b/setup.c
index e938785..2270bd4 100644
--- a/setup.c
+++ b/setup.c
@@ -79,11 +79,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);
+			return NULL;
+		}
+		if (abspath_part_inside_repo(npath)) {
+			free(npath);
+			return NULL;
+		}
+
+		sanitized = xmalloc(strlen(npath) + 1);
+		strcpy(sanitized, npath);
+		free(npath);
 	} else {
 		sanitized = xmalloc(len + strlen(path) + 1);
 		if (len)
@@ -91,26 +103,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;
 }
-- 
1.8.5.2

Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function

[Duy Nguyen]

On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
<martinerikwerner@gmail.com> wrote:
> +       /* 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.

> +       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..

> +               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;
> +       }

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

> +
> +       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
>



-- 
Duy

Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function

[Duy Nguyen]

On Sun, Feb 2, 2014 at 9:19 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> +       /* 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.

No, the "check if.." block does not do real_path() so we still need it
here. Sorry for the noise.
-- 
Duy

Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function

[Martin Erik Werner]

On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:
> On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
> <martinerikwerner@gmail.com> wrote:
> > +       /* 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.

Ah indeed, this should catch that:

diff --git a/setup.c b/setup.c
index 2270bd4..5817875 100644
--- a/setup.c
+++ b/setup.c
@@ -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')
 			/* work tree is the root, or the whole path */
 			memmove(path, path + wtlen, len - wtlen + 1);
+		else
+			return -1;
 		return 0;
 	}
 	path0 = path;

Is it worth adding a test for this as well?:

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index f6f378b..05d3366 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -201,6 +201,10 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '
 	test_cmp expected actual
 '
 
+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"
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX

> > +       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..

Yeah, I think aesthetically I don't like cramming too much into the for loop:

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

neither leaving the init expression unused. So as long as it's just personal
taste I think I'll stick with the current while loop format. But I'll exchange
(*path == '\0') for (*path) though.

--
Martin Erik Werner <martinerikwerner@gmail.com>

Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function

[David Kastrup]

Martin Erik Werner <martinerikwerner@gmail.com> writes:

> On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:
>> On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
>> <martinerikwerner@gmail.com> wrote:
>> > +       /* 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.
>
> Ah indeed, this should catch that:
>
> diff --git a/setup.c b/setup.c
> index 2270bd4..5817875 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -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?

-- 
David Kastrup

Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function

[Torsten Bögershausen]

On 2014-02-02 12.21, David Kastrup wrote:
> Martin Erik Werner <martinerikwerner@gmail.com> writes:
> 
>> On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:
>>> On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
>>> <martinerikwerner@gmail.com> wrote:
>>>> +       /* 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.
>>
>> Ah indeed, this should catch that:
>>
>> diff --git a/setup.c b/setup.c
>> index 2270bd4..5817875 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -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]))

Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function

[Martin Erik Werner]

On Sun, Feb 02, 2014 at 12:37:16PM +0100, Torsten Bögershausen wrote:
> On 2014-02-02 12.21, David Kastrup wrote:
> > Martin Erik Werner <martinerikwerner@gmail.com> writes:
> > 
> >> On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:
> >>> On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
> >>> <martinerikwerner@gmail.com> wrote:
> >>>> +       /* 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.
> >>
> >> Ah indeed, this should catch that:
> >>
> >> diff --git a/setup.c b/setup.c
> >> index 2270bd4..5817875 100644
> >> --- a/setup.c
> >> +++ b/setup.c
> >> @@ -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?

Hmm, am I incorrect in thinking
if (!work_tree)
takes care of that?

> 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]))

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

--
Martin Erik Werner <martinerikwerner@gmail.com>

Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function

[Duy Nguyen]

On Sun, Feb 2, 2014 at 6:13 PM, Martin Erik Werner
<martinerikwerner@gmail.com> wrote:
> diff --git a/setup.c b/setup.c
> index 2270bd4..5817875 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -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')
>                         /* work tree is the root, or the whole path */
>                         memmove(path, path + wtlen, len - wtlen + 1);
> +               else
> +                       return -1;

No, you can't return here. "/abc/defghi" might actually be a symlink
to "/abc/def". If it does not match, let the following loop handle it.
-- 
Duy

Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function

[Torsten Bögershausen]

> 
>> 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]))
> 
> Since the path is already normalized by 'normalize_path_copy_len' which
> seems to guarantee '/'-separation, I have assumed that this was
> unnecessary?

So true, sorry for the noise.

[PATCH v5 0/5] Handling of in-tree symlinks for absolute paths

[Martin Erik Werner]

Again!

(It seems you missed to CC me in your first reply David, please do :)

New reroll, fixing the /dir/repoa and /dir/repolink -> /dir/repo issues noted by
Duy, and adding corresponding tests. If work tree matches beginning of path but
needs further checking, it starts from the end of the work tree length, so as
minimize the 'real_path' calls.

Martin Erik Werner (5):
  t0060: Add test for manipulating symlinks via absolute paths
  t0060: Add test for prefix_path when path == work tree
  t0060: Add tests for prefix_path when path begins with work tree
  setup: Add 'abspath_part_inside_repo' function
  setup: Don't dereference in-tree symlinks for absolute paths

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

-- 
1.8.5.2

[PATCH v5 1/5] t0060: Add test for manipulating symlinks via absolute paths

[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.

This is a regression introduced by 18e051a:
  setup: translate symlinks in filename when using absolute paths
(which did not take symlinks in the work tree into consideration).

Add a known-breakage tests using the prefix_path function, which
currently uses real_path, causing the dereference.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t0060-path-utils.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 07c10c8..0bba988 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,6 +190,11 @@ 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 absolute paths to work tree symlinks' '
+	ln -s target symlink &&
+	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
-- 
1.8.5.2

[PATCH v5 2/5] t0060: Add test for prefix_path when path == work tree

[Martin Erik Werner]

The current behaviour of prefix_path is to return an empty string if
prefixing and absolute path that only contains exactly the work tree.
This behaviour is a potential regression point.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t0060-path-utils.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 0bba988..b8e92e1 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree
 	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
 '
 
+test_expect_success 'prefix_path works with only absolute path to work tree' '
+	echo "" >expected &&
+	test-path-utils prefix_path prefix "$(pwd)" >actual &&
+	test_cmp expected actual
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
-- 
1.8.5.2

[PATCH v5 3/5] t0060: Add tests for prefix_path when path begins with work tree

[Martin Erik Werner]

One edge-case that isn't currently checked in the tests is the beginning
of the path matching the work tree, despite the target not actually
being the work tree, for example:

  path = /dir/repoa
  work_tree = /dir/repo

should fail since the path is outside the repo. However, if /dir/repoa
is in fact a symlink that points to /dir/repo, it should instead
succeed.

Add two tests covering these cases, since they might be potential
regression points.
---
 t/t0060-path-utils.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index b8e92e1..c0a14f6 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '
 	test_cmp expected actual
 '
 
+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")"
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
-- 
1.8.5.2

[PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function

[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 first checks if the
work tree is already the prefix, then incrementally checks each path
level by temporarily NUL-terminating at each '/' and comparing against
the work tree path. If a match is found, it overwrites the input path
with the remainder past the work tree (which will be the in-repo part).

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.

This function is currently only intended for use in
'prefix_path_gently'.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 setup.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/setup.c b/setup.c
index 5432a31..a2e60ab 100644
--- a/setup.c
+++ b/setup.c
@@ -6,6 +6,70 @@ static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
 /*
+ * 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)
+{
+	size_t len;
+	size_t wtlen;
+	char *path0;
+	int off;
+
+	const char* work_tree = get_git_work_tree();
+	if (!work_tree)
+		return -1;
+	wtlen = strlen(work_tree);
+	len = strlen(path);
+	off = 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);
+			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);
+			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
-- 
1.8.5.2

[PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths

[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 symlinks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.

Modify the 'prefix_path_gently' to first normalize the path in order to
make sure path levels are separated by '/', then pass the result to
'abspath_part_inside_repo' to find the in-repo part (without dereferencing
any symlinks inside the work tree).

For absolute paths, 'prefix_path_gently' did not, nor does now do, any
actual prefixing, hence the result from 'abspath_part_in_repo' is
returned as-is.

Fixes t0060-82.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 setup.c               | 36 ++++++++++++++++--------------------
 t/t0060-path-utils.sh |  2 +-
 2 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/setup.c b/setup.c
index a2e60ab..230505c 100644
--- a/setup.c
+++ b/setup.c
@@ -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);
+			return NULL;
+		}
+		if (abspath_part_inside_repo(npath)) {
+			free(npath);
+			return NULL;
+		}
+
+		sanitized = xmalloc(strlen(npath) + 1);
+		strcpy(sanitized, npath);
+		free(npath);
 	} else {
 		sanitized = xmalloc(len + strlen(path) + 1);
 		if (len)
@@ -98,26 +110,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;
 }
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index c0a14f6..f04b82d 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -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 absolute paths to work tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '
 	ln -s target symlink &&
 	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
 '
-- 
1.8.5.2

Re: [PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths

[Duy Nguyen]

On Sun, Feb 2, 2014 at 11:35 PM, Martin Erik Werner
<martinerikwerner@gmail.com> wrote:
> diff --git a/setup.c b/setup.c
> index a2e60ab..230505c 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -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);
> +                       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 <pclouds@gmail.com>

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

Re: [PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths

[Martin Erik Werner]

On Mon, Feb 03, 2014 at 11:15:57AM +0700, Duy Nguyen wrote:
> On Sun, Feb 2, 2014 at 11:35 PM, Martin Erik Werner
> <martinerikwerner@gmail.com> wrote:
> > diff --git a/setup.c b/setup.c
> > index a2e60ab..230505c 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -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);
> > +                       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 <pclouds@gmail.com>
> 
> >         } else {
> >                 sanitized = xmalloc(len + strlen(path) + 1);
> >                 if (len)
> -- 
> Duy

Thank you for reviewing! And thanks Torsten and Junio Likewise. (And
thanks Richard for initial triggering and brief discussion of the bug :)

Hmm, yeah I don't really know what to prefer out of a. Two mallocs with
only a minimal one returned or 2. Single, potentially non-minimal, malloc
returned, if it makes little difference, for simplicity the latter seems nicer.

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

diff --git a/setup.c b/setup.c
index 230505c..dd120cd 100644
--- a/setup.c
+++ b/setup.c
@@ -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 <martinerikwerner@gmail.com>

Re: [PATCH v5 1/5] t0060: Add test for manipulating symlinks via absolute paths

[Junio C Hamano]

Martin Erik Werner <martinerikwerner@gmail.com> writes:

> 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.

Re: [PATCH v5 1/5] t0060: Add test for manipulating symlinks via absolute paths

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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(+)

diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh
index 8d9bc3c..d93089d 100755
--- a/t/t3004-ls-files-basic.sh
+++ b/t/t3004-ls-files-basic.sh
@@ -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 &&
+
+	(
+		cd subs &&
+		git ls-files -s link >../expect &&
+		git ls-files -s "$(pwd)/link" >../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_done

Re: [PATCH v5 1/5] t0060: Add test for manipulating symlinks via absolute paths

[Martin Erik Werner]

On Mon, Feb 03, 2014 at 10:50:17AM -0800, Junio C Hamano wrote:
> Martin Erik Werner <martinerikwerner@gmail.com> writes:
> 
> > 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.

Indeed, it was first discovered via git-mv, (by Richard, using
git-annex) and me reproducing and reporting it was the start of the
thread: http://thread.gmane.org/gmane.comp.version-control.git/240467

In going further (PATCHv0):
> 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.

At this stage I kind of dropped the reference to any specific top-level
command since it seemed to apply to all of them in some way, and I
figured it made more sense with a generic explanation that would apply
to all commands. But it might definitely be worth to mention it in order
for the commit messages to be less technical, and add at least one test
which would actually trigger it in a user-manner. So for the
explanation, something like that?:

	This causes for example 'git add /dir/repo/symlink' to attempt
	to add the target of the symlink rather than the symlink itself,
	which is usually not what the user intends to do.

Hmm, come to think of it, I even made some of those tests back before I
found it could be narrowly tested via prefix_path... I guess I'll pick
out the git-add one since it's the simplest, should that be added to
t0060-path-utils.sh as well, or would it fit better in t3700-add.sh?:

>From 910d8c9f51c3b3f2c03dbf15ce3cf7ea94de8d27 Mon Sep 17 00:00:00 2001
From: Martin Erik Werner <martinerikwerner@gmail.com>
Date: Thu, 16 Jan 2014 00:24:43 +0100
Subject: [PATCH] Add test for manipulating symlinks via absolute paths

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

Add three known-breakage tests using add, mv, and rev-list which
checks this behaviour.

The failure of the git-add test is a regression introduced by 18e051a:
  setup: translate symlinks in filename when using absolute paths
(which did not take symlinks in the work tree into consideration).
---
 t/t0054-symlinks-via-abs-path.sh | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100755 t/t0054-symlinks-via-abs-path.sh

diff --git a/t/t0054-symlinks-via-abs-path.sh b/t/t0054-symlinks-via-abs-path.sh
new file mode 100755
index 0000000..0b3c91e
--- /dev/null
+++ b/t/t0054-symlinks-via-abs-path.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='symlinks via sbsolute paths
+
+This test checks the behavior when symlinks in the working tree are manipulated
+via absolute path arguments.
+'
+. ./test-lib.sh
+
+test_expect_failure SYMLINKS 'git add symlink with absolute path' '
+
+	ln -s target symlink &&
+	git add "$(pwd)/symlink"
+
+'
+
+rm -f symlink
+
+test_expect_failure SYMLINKS 'git mv symlink with absolute path' '
+
+	ln -s target symlink &&
+	git add symlink &&
+	git mv "$(pwd)"/symlink moved
+
+'
+
+rm -f symlink moved
+
+test_expect_failure 'git rev-list symlink with absolute path' '
+
+	ln -s target symlink &&
+	git add symlink &&
+	git commit -m show &&
+	test "$(git rev-list HEAD -- symlink)" = "$(git rev-list HEAD -- $(pwd)/symlink)"
+
+'
+
+test_done
-- 
1.8.5.2

Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function

[Junio C Hamano]

Martin Erik Werner <martinerikwerner@gmail.com> writes:

> 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.

>  /*
> + * 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.

> + * 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.

> +{
> +	size_t len;
> +	size_t wtlen;
> +	char *path0;
> +	int off;
> +
> +	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?

> +	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[]):

> +		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 .

> +			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

Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function

[Martin Erik Werner]

On Mon, Feb 03, 2014 at 11:52:33AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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(+)
> 
> diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh
> index 8d9bc3c..d93089d 100755
> --- a/t/t3004-ls-files-basic.sh
> +++ b/t/t3004-ls-files-basic.sh
> @@ -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 &&
> +
> +	(
> +		cd subs &&
> +		git ls-files -s link >../expect &&
> +		git ls-files -s "$(pwd)/link" >../actual
> +	) &&
> +	test_cmp expect actual
> +'
> +
>  test_done

Your test looks preferrable to the simple git-add one that I proposed,
since it covers some more ground than the simple:

test_expect_success SYMLINKS 'add with abs path to link...' '
	ln -s target link &&
	git add link
'

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

On Mon, Feb 03, 2014 at 01:00:48PM -0800, Junio C Hamano wrote:
> Martin Erik Werner <martinerikwerner@gmail.com> writes:
> 
> > 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?

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?

> 
> 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.
> 
> >  /*
> > + * 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.
?

> > + * 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.

Ok, will do.

> > +{
> > +	size_t len;
> > +	size_t wtlen;
> > +	char *path0;
> > +	int off;
> > +
> > +	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. Do you think
the 'strlen' call should still be moved up a step into
'prefix_path_gently'?

> > +	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[]):

Definitely sounds like a good idea.

> > +		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 .

Here I have only gone by the assumption that previous code did the right
thing, whether or not it *did*, I'm not sure I'm able to figure out at
the moment.

> > +			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

Re: [PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths

[Junio C Hamano]

Martin Erik Werner <martinerikwerner@gmail.com> writes:

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

Yes.  And you need to remove its definition as well to avoid "unused
variable" warning.

Will queue with an obvious fix-up.

Thanks.

>
> diff --git a/setup.c b/setup.c
> index 230505c..dd120cd 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -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 <martinerikwerner@gmail.com>

[PATCH v6 0/6] Handling of in-tree symlinks for absolute paths

[Martin Erik Werner]

The amount of tweaks seemed deserving of a reroll, so here it is:

* Added your test Junio, with what I figured was an appropriate commit
  message describing the user-visible effect (to git-add since I think it's the
  simplest to explain), the commit message for the second commit has been
  reduced somewhat, to not duplicate the message completely.

* Duplicated quite a bit of the explanation from this first commit into
  the last commit, in order to be more obvious about the issue it fixes, I'm
  not sure if it's a bit too much?

* Reworded the last commit to not mention the full-path special case, and
  replaced all occurences of in-repo with "inside work tree" or similar.

* Content-wise compared to 'pu' I've tweaked a few comments, un-inlined and
  added the wtlen <= len check (and the ls-files test is new of course):

diff --git a/setup.c b/setup.c
index 32a6f6b..ba08885 100644
--- a/setup.c
+++ b/setup.c
@@ -6,8 +6,8 @@ static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
 /*
- * No checking if the path is in fact an absolute path is done, and it must
- * already be normalized.
+ * The input parameter must contain an absolute path, 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:
@@ -17,7 +17,7 @@ static int inside_work_tree = -1;
  * /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)
+static int abspath_part_inside_repo(char *path)
 {
 	size_t len;
 	size_t wtlen;
@@ -32,7 +32,7 @@ static inline int abspath_part_inside_repo(char *path)
 	off = 0;
 
 	/* check if work tree is already the prefix */
-	if (strncmp(path, work_tree, wtlen) == 0) {
+	if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
 		if (path[wtlen] == '/') {
 			memmove(path, path + wtlen + 1, len - wtlen);
 			return 0;
@@ -47,7 +47,7 @@ static inline int abspath_part_inside_repo(char *path)
 	path0 = path;
 	path += offset_1st_component(path) + off;
 
-	/* check each level */
+	/* check each '/'-terminated level */
 	while (*path) {
 		path++;
 		if (*path == '/') {


Junio C Hamano (1):
  t3004: Add test for ls-files on symlinks via absolute paths

Martin Erik Werner (5):
  t0060: Add test for prefix_path on symlinks via absolute paths
  t0060: Add test for prefix_path when path == work tree
  t0060: Add tests for prefix_path when path begins with work tree
  setup: Add 'abspath_part_inside_repo' function
  setup: Don't dereference in-tree symlinks for absolute paths

 setup.c                   | 94 +++++++++++++++++++++++++++++++++++++----------
 t/t0060-path-utils.sh     | 21 +++++++++++
 t/t3004-ls-files-basic.sh | 17 +++++++++
 3 files changed, 112 insertions(+), 20 deletions(-)

-- 
1.8.5.2

[PATCH v6 1/6] t3004: Add test for ls-files on symlinks via absolute paths

[Martin Erik Werner]

From: Junio C Hamano <gitster@pobox.com>

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

This causes most high-level functions to misbehave when acting on
symlinks given via absolute paths. For example

  $ git add /dir/repo/symlink

attempts to add the target of the symlink rather than the symlink
itself, which is usually not what the user intends to do.

This is a regression introduced by 18e051a:
  setup: translate symlinks in filename when using absolute paths
(which did not take symlinks inside the work tree into consideration).

Add a known-breakage test using the ls-files function, checking both if
the symlink leads to a target in the same directory, and a target in the
above directory.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
Tested-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t3004-ls-files-basic.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh
index 8d9bc3c..e20c077 100755
--- a/t/t3004-ls-files-basic.sh
+++ b/t/t3004-ls-files-basic.sh
@@ -36,4 +36,21 @@ test_expect_success 'ls-files -h in corrupt repository' '
 	test_i18ngrep "[Uu]sage: git ls-files " broken/usage
 '
 
+test_expect_failure SYMLINKS 'ls-files with absolute paths to 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 &&
+
+	(
+		cd subs &&
+		git ls-files -s link >../expect &&
+		git ls-files -s "$(pwd)/link" >../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.5.2

[PATCH v6 2/6] t0060: Add test for prefix_path on symlinks via absolute paths

[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.

This applies to most high-level commands but prefix_path is the common
denominator for all of them.

Add a known-breakage tests using the prefix_path function, which
currently uses real_path, causing the dereference.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
Reviewed-by: Duy Nguyen <pclouds@gmail.com>
---
 t/t0060-path-utils.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 07c10c8..0bba988 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,6 +190,11 @@ 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 absolute paths to work tree symlinks' '
+	ln -s target symlink &&
+	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
-- 
1.8.5.2

[PATCH v6 3/6] t0060: Add test for prefix_path when path == work tree

[Martin Erik Werner]

The current behaviour of prefix_path is to return an empty string if
prefixing and absolute path that only contains exactly the work tree.
This behaviour is a potential regression point.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
Reviewed-by: Duy Nguyen <pclouds@gmail.com>
---
 t/t0060-path-utils.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 0bba988..b8e92e1 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree
 	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
 '
 
+test_expect_success 'prefix_path works with only absolute path to work tree' '
+	echo "" >expected &&
+	test-path-utils prefix_path prefix "$(pwd)" >actual &&
+	test_cmp expected actual
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
-- 
1.8.5.2

[PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree

[Martin Erik Werner]

One edge-case that isn't currently checked in the tests is the beginning
of the path matching the work tree, despite the target not actually
being the work tree, for example:

  path = /dir/repoa
  work_tree = /dir/repo

should fail since the path is outside the repo. However, if /dir/repoa
is in fact a symlink that points to /dir/repo, it should instead
succeed.

Add two tests covering these cases, since they might be potential
regression points.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
Reviewed-by: Duy Nguyen <pclouds@gmail.com>
---
 t/t0060-path-utils.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index b8e92e1..c0a14f6 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '
 	test_cmp expected actual
 '
 
+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")"
+'
+
 relative_path /foo/a/b/c/	/foo/a/b/	c/
 relative_path /foo/a/b/c/	/foo/a/b	c/
 relative_path /foo/a//b//c/	///foo/a/b//	c/		POSIX
-- 
1.8.5.2

[PATCH v6 5/6] setup: Add 'abspath_part_inside_repo' function

[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 first checks if the
work tree is already the prefix, then incrementally checks each path
level by temporarily NUL-terminating at each '/' and comparing against
the work tree path. If a match is found, it overwrites the input path
with the remainder past the work tree (which will be the part inside the
work tree).

This function is currently only intended for use in
'prefix_path_gently'.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
Reviewed-by: Duy Nguyen <pclouds@gmail.com>
---
 setup.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/setup.c b/setup.c
index 5432a31..906c8e2 100644
--- a/setup.c
+++ b/setup.c
@@ -6,6 +6,70 @@ static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
 /*
+ * The input parameter must contain an absolute path, 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 int abspath_part_inside_repo(char *path)
+{
+	size_t len;
+	size_t wtlen;
+	char *path0;
+	int off;
+
+	const char* work_tree = get_git_work_tree();
+	if (!work_tree)
+		return -1;
+	wtlen = strlen(work_tree);
+	len = strlen(path);
+	off = 0;
+
+	/* check if work tree is already the prefix */
+	if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
+		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);
+			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 '/'-terminated 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
-- 
1.8.5.2

[PATCH v6 6/6] setup: Don't dereference in-tree symlinks for absolute paths

[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.

This causes most high-level functions to misbehave when acting on
symlinks given via absolute paths. For example

	$ git add /dir/repo/symlink

attempts to add the target of the symlink rather than the symlink
itself, which is usually not what the user intends to do.

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

Modify the 'prefix_path_gently' to first normalize the path in order to
make sure path levels are separated by '/', then pass the result to
'abspath_part_inside_repo' to find the part inside the work tree
(without dereferencing any symlinks inside the work tree).

For absolute paths, 'prefix_path_gently' did not, nor does now do, any
actual prefixing, hence the result from 'abspath_part_in_repo' is
returned as-is.

Fixes t0060-82 and t3004-5.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
Reviewed-by: Duy Nguyen <pclouds@gmail.com>
---
 setup.c                   | 30 ++++++++++--------------------
 t/t0060-path-utils.sh     |  2 +-
 t/t3004-ls-files-basic.sh |  2 +-
 3 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/setup.c b/setup.c
index 906c8e2..ba08885 100644
--- a/setup.c
+++ b/setup.c
@@ -86,11 +86,17 @@ 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);
+		sanitized = xmalloc(strlen(path) + 1);
 		if (remaining_prefix)
 			*remaining_prefix = 0;
+		if (normalize_path_copy_len(sanitized, path, remaining_prefix)) {
+			free(sanitized);
+			return NULL;
+		}
+		if (abspath_part_inside_repo(sanitized)) {
+			free(sanitized);
+			return NULL;
+		}
 	} else {
 		sanitized = xmalloc(len + strlen(path) + 1);
 		if (len)
@@ -98,26 +104,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;
 }
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index c0a14f6..f04b82d 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -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 absolute paths to work tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '
 	ln -s target symlink &&
 	test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"
 '
diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh
index e20c077..9c7adbd 100755
--- a/t/t3004-ls-files-basic.sh
+++ b/t/t3004-ls-files-basic.sh
@@ -36,7 +36,7 @@ test_expect_success 'ls-files -h in corrupt repository' '
 	test_i18ngrep "[Uu]sage: git ls-files " broken/usage
 '
 
-test_expect_failure SYMLINKS 'ls-files with absolute paths to symlinks' '
+test_expect_success SYMLINKS 'ls-files with absolute paths to symlinks' '
 	mkdir subs &&
 	ln -s nosuch link &&
 	ln -s ../nosuch subs/link &&
-- 
1.8.5.2

Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function

[Junio C Hamano]

Martin Erik Werner <martinerikwerner@gmail.com> writes:

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

Either is fine.  Thanks.

> On Mon, Feb 03, 2014 at 01:00:48PM -0800, Junio C Hamano wrote:
>> Martin Erik Werner <martinerikwerner@gmail.com> writes:
>> 
>> > 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?
>
> 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?

Yes, "inside the working tree" is much clearer than "inside repo",
because the word "repository" often is used to mean only the stuff
inside $GIT_DIR, i.e. what controls the working tree files.

> 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?

>> 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.
>> 
>> >  /*
>> > + * 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.

OK.

>> > +	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.

OK, strlen() here is perfectly fine, then.

>> 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,...

Good to know.  And the answer to the above question I think is yes,
these should be equivalent.

Thanks.

Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function

[Martin Erik Werner]

On Tue, 2014-02-04 at 10:09 -0800, Junio C Hamano wrote:
> Martin Erik Werner <martinerikwerner@gmail.com> writes:
(...)
> > 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?

The check before the loop doesn't use 'real_path', the one after does:
"/dir/repo" vs "/dir/repolink"

-- 
Martin Erik Werner <martinerikwerner@gmail.com>

Re: [PATCH v6 5/6] setup: Add 'abspath_part_inside_repo' function

[Junio C Hamano]

Martin Erik Werner <martinerikwerner@gmail.com> writes:

> +	const char* work_tree = get_git_work_tree();

Style: asterisk sticks to the variable, not type.

No need to resend---all patches looked reasonable.

Thanks, will queue.

Re: [PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree

[Torsten Bögershausen]

On 2014-02-04 15.25, Martin Erik Werner wrote:

>  t/t0060-path-utils.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index b8e92e1..c0a14f6 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '
>  	test_cmp expected actual
>  '
>  
> +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.

Re: [PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On 2014-02-04 15.25, Martin Erik Werner wrote:
>
>>  t/t0060-path-utils.sh | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>> 
>> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
>> index b8e92e1..c0a14f6 100755
>> --- a/t/t0060-path-utils.sh
>> +++ b/t/t0060-path-utils.sh
>> @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '
>>  	test_cmp expected actual
>>  '
>>  
>> +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.

Yes.