[PATCH] read-tree -m -u: avoid getting confused by intermediate symlinks.

[PATCH] read-tree -m -u: avoid getting confused by intermediate symlinks.

[Junio C Hamano]

When switching from a branch with both x86_64/boot/Makefile and
i386/boot/Makefile to another branch that has x86_64/boot as a
symlink pointing at ../i386/boot, the code incorrectly removed
i386/boot/Makefile.

This was because we first removed everything under x86_64/boot
to make room to create a symbolic link x86_64/boot, then removed
x86_64/boot/Makefile which no longer exists but now is pointing
at i386/boot/Makefile, thanks to the symlink we just created.

This fixes it by using the has_symlink_leading_path() function
introduced previously for git-apply in the checkout codepath.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * This comes on top of the previous git-apply patch.

 Makefile                        |    2 +-
 builtin-apply.c                 |   36 +-----------------------------------
 cache.h                         |    1 +
 symlinks.c                      |   35 +++++++++++++++++++++++++++++++++++
 t/t4122-apply-symlink-inside.sh |    1 -
 unpack-trees.c                  |    2 ++
 6 files changed, 40 insertions(+), 37 deletions(-)
 create mode 100644 symlinks.c

diff --git a/Makefile b/Makefile
index 7cf146b..29243c6 100644
--- a/Makefile
+++ b/Makefile
@@ -318,7 +318,7 @@ LIB_OBJS = \
 	write_or_die.o trace.o list-objects.o grep.o match-trees.o \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
 	color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
-	convert.o attr.o decorate.o progress.o mailmap.o
+	convert.o attr.o decorate.o progress.o mailmap.o symlinks.o
 
 BUILTIN_OBJS = \
 	builtin-add.o \
diff --git a/builtin-apply.c b/builtin-apply.c
index 38d20ef..01acba8 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2009,40 +2009,6 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
 	return 0;
 }
 
-static int has_symlink_component(const char *new_name)
-{
-	char path[PATH_MAX];
-	const char *sp, *ep;
-	char *dp;
-
-	sp = new_name;
-	dp = path;
-
-	while (1) {
-		size_t len;
-		struct stat st;
-
-		ep = strchr(sp, '/');
-		if (!ep)
-			break;
-		len = ep - sp;
-		if (PATH_MAX <= dp + len - path + 2)
-			return 0; /* new name is longer than that??? */
-		memcpy(dp, sp, len);
-		dp[len] = 0;
-
-		if (lstat(path, &st))
-			return 0; /* why? we already lstat() new_name successfully. */
-		if (S_ISLNK(st.st_mode))
-			return 1;
-
-		dp[len++] = '/';
-		dp = dp + len;
-		sp = ep + 1;
-	}
-	return 0;
-}
-
 static int check_to_create_blob(const char *new_name, int ok_if_exists)
 {
 	struct stat nst;
@@ -2056,7 +2022,7 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)
 		 * In such a case, path "new_name" does not exist as
 		 * far as git is concerned.
 		 */
-		if (has_symlink_component(new_name))
+		if (has_symlink_leading_path(new_name))
 			return 0;
 
 		return error("%s: already exists in working directory", new_name);
diff --git a/cache.h b/cache.h
index 8e76152..ab66263 100644
--- a/cache.h
+++ b/cache.h
@@ -410,6 +410,7 @@ struct checkout {
 };
 
 extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
+extern int has_symlink_leading_path(const char *name);
 
 extern struct alternate_object_database {
 	struct alternate_object_database *next;
diff --git a/symlinks.c b/symlinks.c
new file mode 100644
index 0000000..cfecfcf
--- /dev/null
+++ b/symlinks.c
@@ -0,0 +1,35 @@
+#include "cache.h"
+
+int has_symlink_leading_path(const char *name)
+{
+	char path[PATH_MAX];
+	const char *sp, *ep;
+	char *dp;
+
+	sp = name;
+	dp = path;
+
+	while (1) {
+		size_t len;
+		struct stat st;
+
+		ep = strchr(sp, '/');
+		if (!ep)
+			break;
+		len = ep - sp;
+		if (PATH_MAX <= dp + len - path + 2)
+			return 0; /* new name is longer than that??? */
+		memcpy(dp, sp, len);
+		dp[len] = 0;
+
+		if (lstat(path, &st))
+			return 0;
+		if (S_ISLNK(st.st_mode))
+			return 1;
+
+		dp[len++] = '/';
+		dp = dp + len;
+		sp = ep + 1;
+	}
+	return 0;
+}
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 37c9a9f..3ddfe64 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -34,7 +34,6 @@ test_expect_success setup '
 test_expect_success apply '
 
 	git checkout test &&
-	git reset --hard && #### checkout seems to be buggy
 	git diff --exit-code test &&
 	git diff --exit-code --cached test &&
 	git apply --index test.patch
diff --git a/unpack-trees.c b/unpack-trees.c
index 675a999..a6fa32f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -268,6 +268,8 @@ static void unlink_entry(char *name)
 {
 	char *cp, *prev;
 
+	if (has_symlink_leading_path(name))
+		return;
 	if (unlink(name))
 		return;
 	prev = NULL;
-- 
1.5.2.rc3.706.g498a

Re: [PATCH] read-tree -m -u: avoid getting confused by intermediate symlinks.

[Alex Riesen]

On 5/11/07, Junio C Hamano <junkio@cox.net> wrote:
> @@ -268,6 +268,8 @@ static void unlink_entry(char *name)
>  {
>         char *cp, *prev;
>
> +       if (has_symlink_leading_path(name))
> +               return;

This can slow down the unlink case quiet considerably.
Maybe the symlink paths can be cached?

Re: [PATCH] read-tree -m -u: avoid getting confused by intermediate symlinks.

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> On 5/11/07, Junio C Hamano <junkio@cox.net> wrote:
>> @@ -268,6 +268,8 @@ static void unlink_entry(char *name)
>>  {
>>         char *cp, *prev;
>>
>> +       if (has_symlink_leading_path(name))
>> +               return;
>
> This can slow down the unlink case quiet considerably.
> Maybe the symlink paths can be cached?

Yes it can, and probably doable.

This is called once per each path that disappears from the
result, relative to the current tree.  The number of calls to
this function is potentially quite large.

Re: [PATCH] read-tree -m -u: avoid getting confused by intermediate symlinks.

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> "Alex Riesen" <raa.lkml@gmail.com> writes:
>
>> On 5/11/07, Junio C Hamano <junkio@cox.net> wrote:
>>> @@ -268,6 +268,8 @@ static void unlink_entry(char *name)
>>>  {
>>>         char *cp, *prev;
>>>
>>> +       if (has_symlink_leading_path(name))
>>> +               return;
>>
>> This can slow down the unlink case quiet considerably.
>> Maybe the symlink paths can be cached?
>
> Yes it can, and probably doable.
>
> This is called once per each path that disappears from the
> result, relative to the current tree.  The number of calls to
> this function is potentially quite large.

-- >8 --
has_symlink_leading_path(): cache the last lookup

This is on top of the previous one to implement a single-entry
cache for symlinks.  The idea is that:

 * The caller optionally allocates a buffer to hold the symlink
   that caused the check to succeed and passes it in.  This is
   an in-out parameter that the check reuses the result from the
   previous round;

 * Because we call things in the index order, removed entries
   under what used to be a directory but now is a symlink
   cluster together.  The has_symlink_leading_path() function
   will return true because they are under the same symlink;

 * When we see a "this is updated/created" entry, we know we are
   no longer inside the directory that previous round of
   last_symlink is useful, so we can clear the cached value.

So the calling sequence becomes:

	char last_symlink[PATH_MAX];

        *last_symlink = '\0';
        for each index entry { 
		if (lose)
                	unlink_entry(it, last_symlink);
		else if (update) {
			checkout_entry(it);
                        *last_symlink = '\0';
	}


---

diff --git a/builtin-apply.c b/builtin-apply.c
index 01acba8..8b8705a 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2022,7 +2022,7 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)
 		 * In such a case, path "new_name" does not exist as
 		 * far as git is concerned.
 		 */
-		if (has_symlink_leading_path(new_name))
+		if (has_symlink_leading_path(new_name, NULL))
 			return 0;
 
 		return error("%s: already exists in working directory", new_name);
diff --git a/cache.h b/cache.h
index ab66263..aaeb04a 100644
--- a/cache.h
+++ b/cache.h
@@ -410,7 +410,7 @@ struct checkout {
 };
 
 extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
-extern int has_symlink_leading_path(const char *name);
+extern int has_symlink_leading_path(const char *name, char *last_symlink);
 
 extern struct alternate_object_database {
 	struct alternate_object_database *next;
diff --git a/symlinks.c b/symlinks.c
index cfecfcf..ee3f914 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -1,6 +1,6 @@
 #include "cache.h"
 
-int has_symlink_leading_path(const char *name)
+int has_symlink_leading_path(const char *name, char *last_symlink)
 {
 	char path[PATH_MAX];
 	const char *sp, *ep;
@@ -9,6 +9,16 @@ int has_symlink_leading_path(const char *name)
 	sp = name;
 	dp = path;
 
+	if (last_symlink && *last_symlink) {
+		size_t last_len = strlen(last_symlink);
+		size_t len = strlen(name); 
+		if (last_len < len &&
+		    !strncmp(name, last_symlink, last_len) &&
+		    name[last_len] == '/')
+			return 1;
+		*last_symlink = '\0';
+	}
+
 	while (1) {
 		size_t len;
 		struct stat st;
@@ -24,8 +34,11 @@ int has_symlink_leading_path(const char *name)
 
 		if (lstat(path, &st))
 			return 0;
-		if (S_ISLNK(st.st_mode))
+		if (S_ISLNK(st.st_mode)) {
+			if (last_symlink)
+				strcpy(last_symlink, path);
 			return 1;
+		}
 
 		dp[len++] = '/';
 		dp = dp + len;
diff --git a/unpack-trees.c b/unpack-trees.c
index a6fa32f..906ce69 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -264,11 +264,11 @@ static int unpack_trees_rec(struct tree_entry_list **posns, int len,
  * directories, in case this unlink is the removal of the
  * last entry in the directory -- empty directories are removed.
  */
-static void unlink_entry(char *name)
+static void unlink_entry(char *name, char *last_symlink)
 {
 	char *cp, *prev;
 
-	if (has_symlink_leading_path(name))
+	if (has_symlink_leading_path(name, last_symlink))
 		return;
 	if (unlink(name))
 		return;
@@ -293,11 +293,12 @@ static void unlink_entry(char *name)
 
 static struct checkout state;
 static void check_updates(struct cache_entry **src, int nr,
-		struct unpack_trees_options *o)
+			struct unpack_trees_options *o)
 {
 	unsigned short mask = htons(CE_UPDATE);
 	unsigned cnt = 0, total = 0;
 	struct progress progress;
+	char last_symlink[PATH_MAX];
 
 	if (o->update && o->verbose_update) {
 		for (total = cnt = 0; cnt < nr; cnt++) {
@@ -311,6 +312,7 @@ static void check_updates(struct cache_entry **src, int nr,
 		cnt = 0;
 	}
 
+	*last_symlink = '\0';
 	while (nr--) {
 		struct cache_entry *ce = *src++;
 
@@ -319,13 +321,15 @@ static void check_updates(struct cache_entry **src, int nr,
 				display_progress(&progress, ++cnt);
 		if (!ce->ce_mode) {
 			if (o->update)
-				unlink_entry(ce->name);
+				unlink_entry(ce->name, last_symlink);
 			continue;
 		}
 		if (ce->ce_flags & mask) {
 			ce->ce_flags &= ~mask;
-			if (o->update)
+			if (o->update) {
 				checkout_entry(ce, &state, NULL);
+				*last_symlink = '\0';
+			}
 		}
 	}
 	if (total)