{"thread":{"id":"13143","subject":"[RFH] cleaning up \"add across symlinks\"","startedAt":"2008-04-16T20:53:47Z","lastAt":"2008-04-17T16:31:52Z","messageCount":3,"participants":["Junio C Hamano","Tarmigan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"74596","messageId":"7vd4oppllw.fsf@gitster.siamese.dyndns.org","threadId":"13143","inReplyTo":null,"subject":"[RFH] cleaning up \"add across symlinks\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-16T20:53:47Z","receivedAt":"2008-04-16T20:53:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If you have this structure in your work tree:\n\n\tlrwxrwxrwx a -> c\n\tdrwxrwxrwx c\n\t-rw-rw-rw- c/b\n\nand let million monkeys give random paths to \"git-update-index --add\" or\n\"git add\", you should end up with the index with two entries, a symlink\n\"a\" and file \"c/b\".\n\nNot so.  If an unfortunate monkey says \"git add a/b\", we happily add it to\nthe index, because we notice lstat(\"a/b\") succeeds and assume that there\nis such a path.  There isn't, as far as git is concerned, because we track\nsymbolic links.\n\nThe attached is still rough, in that while I think the commands prevent\nsuch bogosities from entering the index, their reporting of the failure is\nsuboptimal.\n\nFor example, if you run the new test script added with this commit, the\nerror message from \"update-index\" would say \"a/b does not exist\", while it\nmight be true from the point of view of git, it is not from the end user's\npoint of view (IOW, the message may need to be reworded to read \"not\nadding a/b because leading path component a is a symlink --- add a itself,\nor not add anything at all\").  Similarly, the \"git add a/b\" should report\nthat it did not add a/b, but it doesn't.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-update-index.c            |   10 ++++++\n dir.c                             |    5 +++\n symlinks.c                        |    4 +-\n t/t2210-add-not-across-symlink.sh |   64 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 81 insertions(+), 2 deletions(-)\n create mode 100755 t/t2210-add-not-across-symlink.sh\n\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex a8795d3..24f3180 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -27,6 +27,8 @@ static int mark_valid_only;\n #define MARK_VALID 1\n #define UNMARK_VALID 2\n \n+static char last_symlink_cache[PATH_MAX];\n+\n static void report(const char *fmt, ...)\n {\n \tva_list vp;\n@@ -201,6 +203,14 @@ static int process_path(const char *path)\n \tif (lstat(path, &st) < 0)\n \t\treturn process_lstat_error(path, errno);\n \n+\t/*\n+\t * If \"a\" is a symlink and the caller gives \"a/b\", that is\n+\t * a nonsense.  As far as git is concerned, there is no such\n+\t * file in the work tree.\n+\t */\n+\tif (has_symlink_leading_path(path, last_symlink_cache))\n+\t\treturn process_lstat_error(path, ENOENT);\n+\n \tlen = strlen(path);\n \tif (S_ISDIR(st.st_mode))\n \t\treturn process_directory(path, len, &st);\ndiff --git a/dir.c b/dir.c\nindex d79762c..e91fed6 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -49,6 +49,11 @@ int common_prefix(const char **pathspec)\n \t\t\tbreak;\n \t\t}\n \t}\n+\tif (prefix) {\n+\t\tint sym = has_symlink_leading_path(path, NULL);\n+\t\tif (sym && sym < prefix)\n+\t\t\treturn sym;\n+\t}\n \treturn prefix;\n }\n \ndiff --git a/symlinks.c b/symlinks.c\nindex be9ace6..9d9116e 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -15,7 +15,7 @@ int has_symlink_leading_path(const char *name, char *last_symlink)\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\treturn last_len;\n \t\t*last_symlink = '\\0';\n \t}\n \n@@ -37,7 +37,7 @@ int has_symlink_leading_path(const char *name, char *last_symlink)\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\treturn len;\n \t\t}\n \n \t\tdp[len++] = '/';\ndiff --git a/t/t2210-add-not-across-symlink.sh b/t/t2210-add-not-across-symlink.sh\nnew file mode 100755\nindex 0000000..102d407\n--- /dev/null\n+++ b/t/t2210-add-not-across-symlink.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='index not getting confused by intermediate symlinks'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\t( echo a; echo c/b ) >expect &&\n+\tmkdir c &&\n+\t>c/b &&\n+\tln -s c a &&\n+\tgit update-index --add a c/b &&\n+\n+\tgit ls-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'update-index confusion (1)' '\n+\ttest_must_fail git update-index --add a/b &&\n+\n+\tgit ls-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'update-index confusion (2)' '\n+\ttest_must_fail git update-index --add a/b &&\n+\n+\tgit ls-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add confusion (0)' '\n+\n+\tgit add a/b c\n+\n+\tgit ls-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add confusion (1)' '\n+\n+\tgit add a/b\n+\n+\tgit ls-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add confusion (2)' '\n+\n+\tgit add a\n+\n+\tgit ls-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add confusion (3)' '\n+\n+\ttest_must_fail git add \"a/*\" &&\n+\n+\tgit ls-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.5.5.120.gea9a0\n"},{"id":"74604","messageId":"905315640804161426l442c2812k3c36c1bafe484157@mail.gmail.com","threadId":"13143","inReplyTo":"7vd4oppllw.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH] cleaning up \"add across symlinks\"","fromName":"Tarmigan","fromEmail":"tarmigan+git@gmail.com","sentAt":"2008-04-16T21:26:41Z","receivedAt":"2008-04-16T21:26:41Z","isPatch":false,"sender":{"key":"tarmigan+git@gmail.com","avatar":null},"body":"On Wed, Apr 16, 2008 at 1:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> If you have this structure in your work tree:\n>\n>         lrwxrwxrwx a -> c\n>         drwxrwxrwx c\n>         -rw-rw-rw- c/b\n>\n>  and let million monkeys give random paths to \"git-update-index --add\" or\n>  \"git add\", you should end up with the index with two entries, a symlink\n>  \"a\" and file \"c/b\".\n>\n>  Not so.  If an unfortunate monkey says \"git add a/b\", we happily add it to\n>  the index, because we notice lstat(\"a/b\") succeeds and assume that there\n>  is such a path.  There isn't, as far as git is concerned, because we track\n>  symbolic links.\n\nThanks Junio, I'll try to do some testing with it later.\n\n>  +test_expect_success 'add confusion (3)' '\n>  +\n>  +       test_must_fail git add \"a/*\" &&\n>  +\n>  +       git ls-files >actual &&\n>  +       test_cmp expect actual\n>  +'\n>  +\n>  +test_done\n\nThat's almost the case I used.  The exact test to add to these,\nwithout the '*' after a/, would be something like this (warning: cut\nand paste):\n\n+test_expect_success 'add confusion (4)' '\n+\n+       test_must_fail git add \"a/\" &&\n+\n+       git ls-files >actual &&\n+       test_cmp expect actual\n+'\n\nThanks,\nTarmigan\n"},{"id":"74649","messageId":"905315640804170931v23df8633xf51276a5858eabc7@mail.gmail.com","threadId":"13143","inReplyTo":"905315640804161426l442c2812k3c36c1bafe484157@mail.gmail.com","subject":"Re: [RFH] cleaning up \"add across symlinks\"","fromName":"Tarmigan","fromEmail":"tarmigan+git@gmail.com","sentAt":"2008-04-17T16:31:52Z","receivedAt":"2008-04-17T16:31:52Z","isPatch":false,"sender":{"key":"tarmigan+git@gmail.com","avatar":null},"body":"On Wed, Apr 16, 2008 at 2:26 PM, Tarmigan <tarmigan+git@gmail.com> wrote:\n> On Wed, Apr 16, 2008 at 1:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>  > If you have this structure in your work tree:\n>  >\n>  >         lrwxrwxrwx a -> c\n>  >         drwxrwxrwx c\n>  >         -rw-rw-rw- c/b\n>  >\n>  >  and let million monkeys give random paths to \"git-update-index --add\" or\n>  >  \"git add\", you should end up with the index with two entries, a symlink\n>  >  \"a\" and file \"c/b\".\n>  >\n>  >  Not so.  If an unfortunate monkey says \"git add a/b\", we happily add it to\n>  >  the index, because we notice lstat(\"a/b\") succeeds and assume that there\n>  >  is such a path.  There isn't, as far as git is concerned, because we track\n>  >  symbolic links.\n>\n>  Thanks Junio, I'll try to do some testing with it later.\n\nI tested with all the cases I could think of and it works for me.\n\n>  That's almost the case I used.  The exact test to add to these,\n>  without the '*' after a/, would be something like this (warning: cut\n>  and paste):\n>\n>  +test_expect_success 'add confusion (4)' '\n>  +\n>  +       test_must_fail git add \"a/\" &&\n\nCurrently, this will succeed, so if you decide to add this case, the\ntest_must_fail should go.\n\nThanks,\nTarmigan\n"}]}