{"thread":{"id":"8079","subject":"[PATCH] read-tree -m -u: avoid getting confused by intermediate symlinks.","startedAt":"2007-05-11T07:13:56Z","lastAt":"2007-05-11T20:38:58Z","messageCount":4,"participants":["Junio C Hamano","Alex Riesen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"41816","messageId":"7vveezde8b.fsf@assigned-by-dhcp.cox.net","threadId":"8079","inReplyTo":null,"subject":"[PATCH] read-tree -m -u: avoid getting confused by intermediate symlinks.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-11T07:13:56Z","receivedAt":"2007-05-11T07:13:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When switching from a branch with both x86_64/boot/Makefile and\ni386/boot/Makefile to another branch that has x86_64/boot as a\nsymlink pointing at ../i386/boot, the code incorrectly removed\ni386/boot/Makefile.\n\nThis was because we first removed everything under x86_64/boot\nto make room to create a symbolic link x86_64/boot, then removed\nx86_64/boot/Makefile which no longer exists but now is pointing\nat i386/boot/Makefile, thanks to the symlink we just created.\n\nThis fixes it by using the has_symlink_leading_path() function\nintroduced previously for git-apply in the checkout codepath.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n * This comes on top of the previous git-apply patch.\n\n Makefile                        |    2 +-\n builtin-apply.c                 |   36 +-----------------------------------\n cache.h                         |    1 +\n symlinks.c                      |   35 +++++++++++++++++++++++++++++++++++\n t/t4122-apply-symlink-inside.sh |    1 -\n unpack-trees.c                  |    2 ++\n 6 files changed, 40 insertions(+), 37 deletions(-)\n create mode 100644 symlinks.c\n\ndiff --git a/Makefile b/Makefile\nindex 7cf146b..29243c6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -318,7 +318,7 @@ LIB_OBJS = \\\n \twrite_or_die.o trace.o list-objects.o grep.o match-trees.o \\\n \talloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \\\n \tcolor.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \\\n-\tconvert.o attr.o decorate.o progress.o mailmap.o\n+\tconvert.o attr.o decorate.o progress.o mailmap.o symlinks.o\n \n BUILTIN_OBJS = \\\n \tbuiltin-add.o \\\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 38d20ef..01acba8 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2009,40 +2009,6 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *\n \treturn 0;\n }\n \n-static int has_symlink_component(const char *new_name)\n-{\n-\tchar path[PATH_MAX];\n-\tconst char *sp, *ep;\n-\tchar *dp;\n-\n-\tsp = new_name;\n-\tdp = path;\n-\n-\twhile (1) {\n-\t\tsize_t len;\n-\t\tstruct stat st;\n-\n-\t\tep = strchr(sp, '/');\n-\t\tif (!ep)\n-\t\t\tbreak;\n-\t\tlen = ep - sp;\n-\t\tif (PATH_MAX <= dp + len - path + 2)\n-\t\t\treturn 0; /* new name is longer than that??? */\n-\t\tmemcpy(dp, sp, len);\n-\t\tdp[len] = 0;\n-\n-\t\tif (lstat(path, &st))\n-\t\t\treturn 0; /* why? we already lstat() new_name successfully. */\n-\t\tif (S_ISLNK(st.st_mode))\n-\t\t\treturn 1;\n-\n-\t\tdp[len++] = '/';\n-\t\tdp = dp + len;\n-\t\tsp = ep + 1;\n-\t}\n-\treturn 0;\n-}\n-\n static int check_to_create_blob(const char *new_name, int ok_if_exists)\n {\n \tstruct stat nst;\n@@ -2056,7 +2022,7 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)\n \t\t * In such a case, path \"new_name\" does not exist as\n \t\t * far as git is concerned.\n \t\t */\n-\t\tif (has_symlink_component(new_name))\n+\t\tif (has_symlink_leading_path(new_name))\n \t\t\treturn 0;\n \n \t\treturn error(\"%s: already exists in working directory\", new_name);\ndiff --git a/cache.h b/cache.h\nindex 8e76152..ab66263 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -410,6 +410,7 @@ struct checkout {\n };\n \n extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);\n+extern int has_symlink_leading_path(const char *name);\n \n extern struct alternate_object_database {\n \tstruct alternate_object_database *next;\ndiff --git a/symlinks.c b/symlinks.c\nnew file mode 100644\nindex 0000000..cfecfcf\n--- /dev/null\n+++ b/symlinks.c\n@@ -0,0 +1,35 @@\n+#include \"cache.h\"\n+\n+int has_symlink_leading_path(const char *name)\n+{\n+\tchar path[PATH_MAX];\n+\tconst char *sp, *ep;\n+\tchar *dp;\n+\n+\tsp = name;\n+\tdp = path;\n+\n+\twhile (1) {\n+\t\tsize_t len;\n+\t\tstruct stat st;\n+\n+\t\tep = strchr(sp, '/');\n+\t\tif (!ep)\n+\t\t\tbreak;\n+\t\tlen = ep - sp;\n+\t\tif (PATH_MAX <= dp + len - path + 2)\n+\t\t\treturn 0; /* new name is longer than that??? */\n+\t\tmemcpy(dp, sp, len);\n+\t\tdp[len] = 0;\n+\n+\t\tif (lstat(path, &st))\n+\t\t\treturn 0;\n+\t\tif (S_ISLNK(st.st_mode))\n+\t\t\treturn 1;\n+\n+\t\tdp[len++] = '/';\n+\t\tdp = dp + len;\n+\t\tsp = ep + 1;\n+\t}\n+\treturn 0;\n+}\ndiff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\nindex 37c9a9f..3ddfe64 100755\n--- a/t/t4122-apply-symlink-inside.sh\n+++ b/t/t4122-apply-symlink-inside.sh\n@@ -34,7 +34,6 @@ test_expect_success setup '\n test_expect_success apply '\n \n \tgit checkout test &&\n-\tgit reset --hard && #### checkout seems to be buggy\n \tgit diff --exit-code test &&\n \tgit diff --exit-code --cached test &&\n \tgit apply --index test.patch\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 675a999..a6fa32f 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -268,6 +268,8 @@ static void unlink_entry(char *name)\n {\n \tchar *cp, *prev;\n \n+\tif (has_symlink_leading_path(name))\n+\t\treturn;\n \tif (unlink(name))\n \t\treturn;\n \tprev = NULL;\n-- \n1.5.2.rc3.706.g498a\n"},{"id":"41834","messageId":"81b0412b0705110708y7e3a9024p9e117e2da84ae916@mail.gmail.com","threadId":"8079","inReplyTo":"7vveezde8b.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] read-tree -m -u: avoid getting confused by intermediate symlinks.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-11T14:08:15Z","receivedAt":"2007-05-11T14:08:15Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/11/07, Junio C Hamano <junkio@cox.net> wrote:\n> @@ -268,6 +268,8 @@ static void unlink_entry(char *name)\n>  {\n>         char *cp, *prev;\n>\n> +       if (has_symlink_leading_path(name))\n> +               return;\n\nThis can slow down the unlink case quiet considerably.\nMaybe the symlink paths can be cached?\n"},{"id":"41841","messageId":"7vwszfb80y.fsf@assigned-by-dhcp.cox.net","threadId":"8079","inReplyTo":"81b0412b0705110708y7e3a9024p9e117e2da84ae916@mail.gmail.com","subject":"Re: [PATCH] read-tree -m -u: avoid getting confused by intermediate symlinks.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-11T17:10:53Z","receivedAt":"2007-05-11T17:10:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" <raa.lkml@gmail.com> writes:\n\n> On 5/11/07, Junio C Hamano <junkio@cox.net> wrote:\n>> @@ -268,6 +268,8 @@ static void unlink_entry(char *name)\n>>  {\n>>         char *cp, *prev;\n>>\n>> +       if (has_symlink_leading_path(name))\n>> +               return;\n>\n> This can slow down the unlink case quiet considerably.\n> Maybe the symlink paths can be cached?\n\nYes it can, and probably doable.\n\nThis is called once per each path that disappears from the\nresult, relative to the current tree.  The number of calls to\nthis function is potentially quite large.\n"},{"id":"41857","messageId":"7vps579jtp.fsf@assigned-by-dhcp.cox.net","threadId":"8079","inReplyTo":"7vwszfb80y.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] read-tree -m -u: avoid getting confused by intermediate symlinks.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-11T20:38:58Z","receivedAt":"2007-05-11T20:38:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> \"Alex Riesen\" <raa.lkml@gmail.com> writes:\n>\n>> On 5/11/07, Junio C Hamano <junkio@cox.net> wrote:\n>>> @@ -268,6 +268,8 @@ static void unlink_entry(char *name)\n>>>  {\n>>>         char *cp, *prev;\n>>>\n>>> +       if (has_symlink_leading_path(name))\n>>> +               return;\n>>\n>> This can slow down the unlink case quiet considerably.\n>> Maybe the symlink paths can be cached?\n>\n> Yes it can, and probably doable.\n>\n> This is called once per each path that disappears from the\n> result, relative to the current tree.  The number of calls to\n> this function is potentially quite large.\n\n-- >8 --\nhas_symlink_leading_path(): cache the last lookup\n\nThis is on top of the previous one to implement a single-entry\ncache for symlinks.  The idea is that:\n\n * The caller optionally allocates a buffer to hold the symlink\n   that caused the check to succeed and passes it in.  This is\n   an in-out parameter that the check reuses the result from the\n   previous round;\n\n * Because we call things in the index order, removed entries\n   under what used to be a directory but now is a symlink\n   cluster together.  The has_symlink_leading_path() function\n   will return true because they are under the same symlink;\n\n * When we see a \"this is updated/created\" entry, we know we are\n   no longer inside the directory that previous round of\n   last_symlink is useful, so we can clear the cached value.\n\nSo the calling sequence becomes:\n\n\tchar last_symlink[PATH_MAX];\n\n        *last_symlink = '\\0';\n        for each index entry { \n\t\tif (lose)\n                \tunlink_entry(it, last_symlink);\n\t\telse if (update) {\n\t\t\tcheckout_entry(it);\n                        *last_symlink = '\\0';\n\t}\n\n\n---\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 01acba8..8b8705a 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2022,7 +2022,7 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)\n \t\t * In such a case, path \"new_name\" does not exist as\n \t\t * far as git is concerned.\n \t\t */\n-\t\tif (has_symlink_leading_path(new_name))\n+\t\tif (has_symlink_leading_path(new_name, NULL))\n \t\t\treturn 0;\n \n \t\treturn error(\"%s: already exists in working directory\", new_name);\ndiff --git a/cache.h b/cache.h\nindex ab66263..aaeb04a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -410,7 +410,7 @@ struct checkout {\n };\n \n extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);\n-extern int has_symlink_leading_path(const char *name);\n+extern int has_symlink_leading_path(const char *name, char *last_symlink);\n \n extern struct alternate_object_database {\n \tstruct alternate_object_database *next;\ndiff --git a/symlinks.c b/symlinks.c\nindex cfecfcf..ee3f914 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -1,6 +1,6 @@\n #include \"cache.h\"\n \n-int has_symlink_leading_path(const char *name)\n+int has_symlink_leading_path(const char *name, char *last_symlink)\n {\n \tchar path[PATH_MAX];\n \tconst char *sp, *ep;\n@@ -9,6 +9,16 @@ int has_symlink_leading_path(const char *name)\n \tsp = name;\n \tdp = path;\n \n+\tif (last_symlink && *last_symlink) {\n+\t\tsize_t last_len = strlen(last_symlink);\n+\t\tsize_t len = strlen(name); \n+\t\tif (last_len < len &&\n+\t\t    !strncmp(name, last_symlink, last_len) &&\n+\t\t    name[last_len] == '/')\n+\t\t\treturn 1;\n+\t\t*last_symlink = '\\0';\n+\t}\n+\n \twhile (1) {\n \t\tsize_t len;\n \t\tstruct stat st;\n@@ -24,8 +34,11 @@ int has_symlink_leading_path(const char *name)\n \n \t\tif (lstat(path, &st))\n \t\t\treturn 0;\n-\t\tif (S_ISLNK(st.st_mode))\n+\t\tif (S_ISLNK(st.st_mode)) {\n+\t\t\tif (last_symlink)\n+\t\t\t\tstrcpy(last_symlink, path);\n \t\t\treturn 1;\n+\t\t}\n \n \t\tdp[len++] = '/';\n \t\tdp = dp + len;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex a6fa32f..906ce69 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -264,11 +264,11 @@ static int unpack_trees_rec(struct tree_entry_list **posns, int len,\n  * directories, in case this unlink is the removal of the\n  * last entry in the directory -- empty directories are removed.\n  */\n-static void unlink_entry(char *name)\n+static void unlink_entry(char *name, char *last_symlink)\n {\n \tchar *cp, *prev;\n \n-\tif (has_symlink_leading_path(name))\n+\tif (has_symlink_leading_path(name, last_symlink))\n \t\treturn;\n \tif (unlink(name))\n \t\treturn;\n@@ -293,11 +293,12 @@ static void unlink_entry(char *name)\n \n static struct checkout state;\n static void check_updates(struct cache_entry **src, int nr,\n-\t\tstruct unpack_trees_options *o)\n+\t\t\tstruct unpack_trees_options *o)\n {\n \tunsigned short mask = htons(CE_UPDATE);\n \tunsigned cnt = 0, total = 0;\n \tstruct progress progress;\n+\tchar last_symlink[PATH_MAX];\n \n \tif (o->update && o->verbose_update) {\n \t\tfor (total = cnt = 0; cnt < nr; cnt++) {\n@@ -311,6 +312,7 @@ static void check_updates(struct cache_entry **src, int nr,\n \t\tcnt = 0;\n \t}\n \n+\t*last_symlink = '\\0';\n \twhile (nr--) {\n \t\tstruct cache_entry *ce = *src++;\n \n@@ -319,13 +321,15 @@ static void check_updates(struct cache_entry **src, int nr,\n \t\t\t\tdisplay_progress(&progress, ++cnt);\n \t\tif (!ce->ce_mode) {\n \t\t\tif (o->update)\n-\t\t\t\tunlink_entry(ce->name);\n+\t\t\t\tunlink_entry(ce->name, last_symlink);\n \t\t\tcontinue;\n \t\t}\n \t\tif (ce->ce_flags & mask) {\n \t\t\tce->ce_flags &= ~mask;\n-\t\t\tif (o->update)\n+\t\t\tif (o->update) {\n \t\t\t\tcheckout_entry(ce, &state, NULL);\n+\t\t\t\t*last_symlink = '\\0';\n+\t\t\t}\n \t\t}\n \t}\n \tif (total)\n"}]}