{"thread":{"id":"37265","subject":"Bug report about symlinks","startedAt":"2014-07-31T19:50:20Z","lastAt":"2014-08-09T17:43:29Z","messageCount":10,"participants":["Nikolay Avdeev","René Scharfe","Junio C Hamano","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"247075","messageId":"web-416867478@relay1.vsu.ru","threadId":"37265","inReplyTo":null,"subject":"Bug report about symlinks","fromName":"Nikolay Avdeev","fromEmail":"avdeev@math.vsu.ru","sentAt":"2014-07-31T19:50:20Z","receivedAt":"2014-07-31T19:50:20Z","isPatch":false,"sender":{"key":"avdeev@math.vsu.ru","avatar":null},"body":"Greetings from Russia, comrads!\n\nI've noticed something strange with git status when replacing a folder with\nsymlink to another folder.\nThere is a git repo with script with demo in the attachment.\n\nYours sincerely,\nNickKolok aka Nikolay Avdeev.\n\nДоброго времени суток, товарищи!\n\nПри замене папки на симлинк на другую папку с похожими, но не одинаковыми\nфайлами git status ведёт себя странно.\nПрилагаю архив с репозиторием. В скрипте - пример и пояснения.\n\nС уважением,\nНиколай Авдеев aka NickKolok.\n"},{"id":"247090","messageId":"53DABD69.7010004@web.de","threadId":"37265","inReplyTo":"web-416867478@relay1.vsu.ru","subject":"Re: Bug report about symlinks","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-07-31T22:04:25Z","receivedAt":"2014-07-31T22:04:25Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.07.2014 um 21:50 schrieb Nikolay Avdeev:\n> I've noticed something strange with git status when replacing a folder with\n> symlink to another folder.\n> There is a git repo with script with demo in the attachment.\n\nLet's try and make this a bit easier for folks to follow along.\n\n# Create test repo with two directories with two files each.\n$ git init\nInitialized empty Git repository in /tmp/.git/\n$ mkdir a b\n$ echo x >a/equal\n$ echo x >b/equal\n$ echo y >a/different\n$ echo z >b/different\n$ git add a b\n$ git commit -minitial\n[master (root-commit) 6d36895] initial\n 4 files changed, 4 insertions(+)\n create mode 100644 a/different\n create mode 100644 a/equal\n create mode 100644 b/different\n create mode 100644 b/equal\n\n# Replace one directory with a symlink to the other.\n$ rm -rf b\n$ ln -s a b\n\n# First git status call.\n$ git status\nOn branch master\nChanges not staged for commit:\n (use \"git add/rm ...\" to update what will be committed)\n (use \"git checkout -- ...\" to discard changes in working directory)\n\n deleted: b/different\n\nUntracked files:\n (use \"git add ...\" to include in what will be committed)\n\n b\n\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n\n# Stage the changes.\n$ git add b\n\n# Second git status call.\n$ git status\nOn branch master\nChanges to be committed:\n (use \"git reset HEAD ...\" to unstage)\n\n new file: b\n deleted: b/different\n deleted: b/equal\n\n# Commit the staged changes.\n$ git commit -msymlinked\n[master 4498f28] symlinked\n 3 files changed, 1 insertion(+), 2 deletions(-)\n create mode 120000 b\n delete mode 100644 b/different\n delete mode 100644 b/equal\n\n\nThat the first and second status call report different results is a \nfeature; staging changes using git add alters the status. A commit \nafter the first status call would be empty.\n\nIt could be debated if the first git status call should also report \nb/equal as deleted.\n\nRené\n"},{"id":"247102","messageId":"xmqqppgkw55g.fsf@gitster.dls.corp.google.com","threadId":"37265","inReplyTo":"53DABD69.7010004@web.de","subject":"Re: Bug report about symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-01T16:23:07Z","receivedAt":"2014-08-01T16:23:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> # Create test repo with two directories with two files each.\n> $ git init\n> Initialized empty Git repository in /tmp/.git/\n> $ mkdir a b\n> $ echo x >a/equal\n> $ echo x >b/equal\n> $ echo y >a/different\n> $ echo z >b/different\n> $ git add a b\n> $ git commit -minitial\n> [master (root-commit) 6d36895] initial\n> 4 files changed, 4 insertions(+)\n> create mode 100644 a/different\n> create mode 100644 a/equal\n> create mode 100644 b/different\n> create mode 100644 b/equal\n>\n> # Replace one directory with a symlink to the other.\n> $ rm -rf b\n> $ ln -s a b\n>\n> # First git status call.\n> $ git status\n> On branch master\n> Changes not staged for commit:\n> (use \"git add/rm ...\" to update what will be committed)\n> (use \"git checkout -- ...\" to discard changes in working directory)\n>\n> deleted: b/different\n>\n> Untracked files:\n> (use \"git add ...\" to include in what will be committed)\n>\n> b\n>\n> no changes added to commit (use \"git add\" and/or \"git commit -a\")\n> ...\n>\n> It could be debated if the first git status call should also report\n> b/equal as deleted.\n\nHmph.\n\nI wonder if \"could be\" is an understatement. The difference of\nreporting between b/equal and b/different may indicate that somebody\nis mistakenly comparing the index with these paths, without first\nchecking each path with has_symlink_leading_path(), or something,\nno?\n\nOr am I misreading the report and codepath?\n\nPuzzled...\n"},{"id":"247172","messageId":"53DCF14D.8040705@web.de","threadId":"37265","inReplyTo":"xmqqppgkw55g.fsf@gitster.dls.corp.google.com","subject":"Re: Bug report about symlinks","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-08-02T14:10:21Z","receivedAt":"2014-08-02T14:10:21Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.08.2014 um 18:23 schrieb Junio C Hamano:\n> René Scharfe writes:\n> \n>> # Create test repo with two directories with two files each.\n>> $ git init\n>> Initialized empty Git repository in /tmp/.git/\n>> $ mkdir a b\n>> $ echo x >a/equal\n>> $ echo x >b/equal\n>> $ echo y >a/different\n>> $ echo z >b/different\n>> $ git add a b\n>> $ git commit -minitial\n>> [master (root-commit) 6d36895] initial\n>> 4 files changed, 4 insertions(+)\n>> create mode 100644 a/different\n>> create mode 100644 a/equal\n>> create mode 100644 b/different\n>> create mode 100644 b/equal\n>>\n>> # Replace one directory with a symlink to the other.\n>> $ rm -rf b\n>> $ ln -s a b\n>>\n>> # First git status call.\n>> $ git status\n>> On branch master\n>> Changes not staged for commit:\n>> (use \"git add/rm ...\" to update what will be committed)\n>> (use \"git checkout -- ...\" to discard changes in working directory)\n>>\n>> deleted: b/different\n>>\n>> Untracked files:\n>> (use \"git add ...\" to include in what will be committed)\n>>\n>> b\n>>\n>> no changes added to commit (use \"git add\" and/or \"git commit -a\")\n>> ...\n>>\n>> It could be debated if the first git status call should also report\n>> b/equal as deleted.\n> \n> Hmph.\n> \n> I wonder if \"could be\" is an understatement. The difference of\n> reporting between b/equal and b/different may indicate that somebody\n> is mistakenly comparing the index with these paths, without first\n> checking each path with has_symlink_leading_path(), or something,\n> no?\n\nHow about the patch below? Before it checks if an index entry exists\nin the work tree, it checks if its path includes a symlink. After\nthe patch, git status reports b/equal as deleted, too. The test\nsuite still passes.\n\nAnd do we need to use the threaded_ variant of the function here?\n\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 5d3c8bd..6f0057f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1064,6 +1064,14 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \t\treturn ce;\n \t}\n \n+\tif (has_symlink_leading_path(ce->name, ce_namelen(ce))) {\n+\t\tif (ignore_missing)\n+\t\t\treturn ce;\n+\t\tif (err)\n+\t\t\t*err = ENOENT;\n+\t\treturn NULL;\n+\t}\n+\n \tif (lstat(ce->name, &st) < 0) {\n \t\tif (ignore_missing && errno == ENOENT)\n \t\t\treturn ce;\n"},{"id":"247198","messageId":"xmqqk36ptrs6.fsf@gitster.dls.corp.google.com","threadId":"37265","inReplyTo":"53DCF14D.8040705@web.de","subject":"Re: Bug report about symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-03T17:19:21Z","receivedAt":"2014-08-03T17:19:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> How about the patch below? Before it checks if an index entry exists\n> in the work tree, it checks if its path includes a symlink.\n\nHonestly, I didn't expect the fix to be in the refresh-index code\npath, but doing this there sort of makes sense.\n\nI think you, who dug to find out where to add the check, already\nknow this, and I am writing this mainly for myself and for the list\narchive, but when the knee-jerk \"has-syjmlink-leading-path missing?\"\nreaction came to me, two obvious optimizations also came to my mind:\n\n - When checking a cache entry \"a/b/c/d/e\" and we find out \"a/b/c\"\n is a symbolic link, we mark it as ~CE_VALID, but at the same\n time, we learn \"a/b/c/any/thing\" are also ~CE_VALID with that\n check, so we _could_, after the has_symlink_leading_path once\n returns true, so there may be an opportunity to fast-forward the\n scan, marking all paths under \"a/b/c\" as ~CE_VALID.\n\n - After finding out \"a/b/c\" is a symbolic link to cause anything\n underneath marked as ~CE_VALID, we also know \"a/\" and \"a/b/\"\n exists as real directories, so a later check for \"a/b/some/thing\"\n can start checking at \"a/b/some/\" without checking \"a/\" and \"a/b/\".\n\nThe latter is more important as it is a much more common case that\nthe shape of the working tree not to change.\n\nBut I think the implementation of has_symlink_leading_path() already\nhas these optimizations built inside around the (unfortunately named)\n\"struct cache_def\", so it probably would not give us much boost to\nimplement such a fast-forwarding of the scan.\n\n> And do we need to use the threaded_ variant of the function here?\n\nHmmm, this is a tangent, but you comment made me wonder if we also\nneed to adjust preload_thread() in preload-index.c somehow, but we\ndo not touch CE_UPTODATE there, so it probably is not necessary.\n\nThe caller of refresh_cache_ent() is walking an array of sorted\npathnames aka istate->cache[] in a single-threaded fashion, possibly\nwith a pathspec to limit the scan. Do you mean that we may want to\nmake istate->cache[] scanned by multiple threads? I am not sure.\n\n> diff --git a/read-cache.c b/read-cache.c\n> index 5d3c8bd..6f0057f 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1064,6 +1064,14 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n> \t\treturn ce;\n> \t}\n> \n> +\tif (has_symlink_leading_path(ce->name, ce_namelen(ce))) {\n> +\t\tif (ignore_missing)\n> +\t\t\treturn ce;\n> +\t\tif (err)\n> +\t\t\t*err = ENOENT;\n> +\t\treturn NULL;\n> +\t}\n> +\n> \tif (lstat(ce->name, &st) < 0) {\n> \t\tif (ignore_missing && errno == ENOENT)\n> \t\t\treturn ce;\n"},{"id":"247201","messageId":"53DEBEE7.6070009@web.de","threadId":"37265","inReplyTo":"xmqqk36ptrs6.fsf@gitster.dls.corp.google.com","subject":"Re: Bug report about symlinks","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-08-03T22:59:51Z","receivedAt":"2014-08-03T22:59:51Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.08.2014 um 19:19 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> How about the patch below? Before it checks if an index entry exists\n>> in the work tree, it checks if its path includes a symlink.\n>\n> Honestly, I didn't expect the fix to be in the refresh-index code\n> path, but doing this there sort of makes sense.\n\nI found it through observation, not understanding. Just looked for \nstat/lstat calls executed by git status for b/different and b/equal \nusing strace; debugging printfs told me where they came from.\n\n>> And do we need to use the threaded_ variant of the function here?\n>\n> Hmmm, this is a tangent, but you comment made me wonder if we also\n> need to adjust preload_thread() in preload-index.c somehow, but we\n> do not touch CE_UPTODATE there, so it probably is not necessary.\n\nThe function calls ce_mark_uptodate(), which does set CE_UPTODATE. It \ncalls threaded_has_symlink_leading_path() before lstat() already, \nhowever. (Since f62ce3de: Make index preloading check the whole path to \nthe file.)\n\n> The caller of refresh_cache_ent() is walking an array of sorted\n> pathnames aka istate->cache[] in a single-threaded fashion, possibly\n> with a pathspec to limit the scan.\n\nThere are two direct callers (refresh_index(), refresh_cache_entry()) \nand several indirect ones. Do we have a way to detect unsynchronized \nparallel access to the has_symlink_leading_path()-cache? Checking the \nfull callers-of-callers tree manually looks a bit scary to me.\n\n> Do you mean that we may want to\n> make istate->cache[] scanned by multiple threads? I am not sure.\n\nNo, I didn't want to suggest any performance improvements. I'm only \ninterested in correctness for now.\n\nRené\n"},{"id":"247206","messageId":"CACsJy8AKa2NBJT6+w87JqDM9eL5nw=3OjMCj-kq=yoyE6Oi_Ew@mail.gmail.com","threadId":"37265","inReplyTo":"xmqqk36ptrs6.fsf@gitster.dls.corp.google.com","subject":"Re: Bug report about symlinks","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-08-04T11:03:13Z","receivedAt":"2014-08-04T11:03:13Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Aug 4, 2014 at 12:19 AM, Junio C Hamano wrote:\n> I think you, who dug to find out where to add the check, already\n> know this, and I am writing this mainly for myself and for the list\n> archive, but when the knee-jerk \"has-syjmlink-leading-path missing?\"\n> reaction came to me, two obvious optimizations also came to my mind:\n>\n> - When checking a cache entry \"a/b/c/d/e\" and we find out \"a/b/c\"\n> is a symbolic link, we mark it as ~CE_VALID, but at the same\n> time, we learn \"a/b/c/any/thing\" are also ~CE_VALID with that\n> check, so we _could_, after the has_symlink_leading_path once\n> returns true, so there may be an opportunity to fast-forward the\n> scan, marking all paths under \"a/b/c\" as ~CE_VALID.\n>\n> - After finding out \"a/b/c\" is a symbolic link to cause anything\n> underneath marked as ~CE_VALID, we also know \"a/\" and \"a/b/\"\n> exists as real directories, so a later check for \"a/b/some/thing\"\n> can start checking at \"a/b/some/\" without checking \"a/\" and \"a/b/\".\n\nJust checking, you meant CE_UPTODATE, not CE_VALID, right? CE_VALID is\nonly used with --assume-unchanged\n\n> The latter is more important as it is a much more common case that\n> the shape of the working tree not to change.\n>\n> But I think the implementation of has_symlink_leading_path() already\n> has these optimizations built inside around the (unfortunately named)\n> \"struct cache_def\", so it probably would not give us much boost to\n> implement such a fast-forwarding of the scan.\n\nYeah my first thought is another flood of lstat(). But it looks like\nhas_symlink_leading_path() tries hard to reduce lstat() already. We\ncould disable this call in filesytems that do not support symlinks\n(e.g. vfat) but I guess that's just a minority.\n-- \nDuy\n"},{"id":"247213","messageId":"xmqqbns0tdqu.fsf@gitster.dls.corp.google.com","threadId":"37265","inReplyTo":"53DEBEE7.6070009@web.de","subject":"Re: Bug report about symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-04T16:34:49Z","receivedAt":"2014-08-04T16:34:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 03.08.2014 um 19:19 schrieb Junio C Hamano:\n>\n>>> And do we need to use the threaded_ variant of the function here?\n>>\n>> Hmmm, this is a tangent, but you comment made me wonder if we also\n>> need to adjust preload_thread() in preload-index.c somehow, but we\n>> do not touch CE_UPTODATE there, so it probably is not necessary.\n>\n> The function calls ce_mark_uptodate(), which does set CE_UPTODATE. It\n> calls threaded_has_symlink_leading_path() before lstat() already,\n> however. (Since f62ce3de: Make index preloading check the whole path\n> to the file.)\n\nYeah, by \"we do not touch\", I meant \"for paths that is beyond a\nsymlink, we do not touch\" (i.e. we have that \"continue\" before\nlstat-match-then-mark sequence).\n\n>> The caller of refresh_cache_ent() is walking an array of sorted\n>> pathnames aka istate->cache[] in a single-threaded fashion, possibly\n>> with a pathspec to limit the scan.\n>\n> There are two direct callers (refresh_index(), refresh_cache_entry())\n> and several indirect ones. Do we have a way to detect unsynchronized\n> parallel access to the has_symlink_leading_path()-cache? Checking the\n> full callers-of-callers tree manually looks a bit scary to me.\n\nThe threaded variant is not used anybody outside preload-index, so\ncurrently we should be OK, I would think.\n"},{"id":"247214","messageId":"xmqq7g2otdoq.fsf@gitster.dls.corp.google.com","threadId":"37265","inReplyTo":"CACsJy8AKa2NBJT6+w87JqDM9eL5nw=3OjMCj-kq=yoyE6Oi_Ew@mail.gmail.com","subject":"Re: Bug report about symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-04T16:36:05Z","receivedAt":"2014-08-04T16:36:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Just checking, you meant CE_UPTODATE, not CE_VALID, right? CE_VALID is\n> only used with --assume-unchanged\n\nYup. Thanks.\n"},{"id":"247533","messageId":"53E65DC1.4010502@web.de","threadId":"37265","inReplyTo":"xmqqbns0tdqu.fsf@gitster.dls.corp.google.com","subject":"[PATCH] read-cache: check for leading symlinks when refreshing index","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-08-09T17:43:29Z","receivedAt":"2014-08-09T17:43:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Don't add paths with leading symlinks to the index while refreshing; we\nonly track those symlinks themselves. We already ignore them while\npreloading (see read_index_preload.c).\n\nReported-by: Nikolay Avdeev \nSigned-off-by: Rene Scharfe \n---\n read-cache.c | 8 ++++++++\n t/t7515-status-symlinks.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 51 insertions(+)\n create mode 100755 t/t7515-status-symlinks.sh\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 5d3c8bd..6f0057f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1064,6 +1064,14 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \t\treturn ce;\n \t}\n \n+\tif (has_symlink_leading_path(ce->name, ce_namelen(ce))) {\n+\t\tif (ignore_missing)\n+\t\t\treturn ce;\n+\t\tif (err)\n+\t\t\t*err = ENOENT;\n+\t\treturn NULL;\n+\t}\n+\n \tif (lstat(ce->name, &st) < 0) {\n \t\tif (ignore_missing && errno == ENOENT)\n \t\t\treturn ce;\ndiff --git a/t/t7515-status-symlinks.sh b/t/t7515-status-symlinks.sh\nnew file mode 100755\nindex 0000000..9f989be\n--- /dev/null\n+++ b/t/t7515-status-symlinks.sh\n@@ -0,0 +1,43 @@\n+#!/bin/sh\n+\n+test_description='git status and symlinks'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo .gitignore >.gitignore &&\n+\techo actual >>.gitignore &&\n+\techo expect >>.gitignore &&\n+\tmkdir dir &&\n+\techo x >dir/file1 &&\n+\techo y >dir/file2 &&\n+\tgit add dir &&\n+\tgit commit -m initial &&\n+\tgit tag initial\n+'\n+\n+test_expect_success SYMLINKS 'symlink to a directory' '\n+\ttest_when_finished \"rm symlink\" &&\n+\tln -s dir symlink &&\n+\techo \"?? symlink\" >expect &&\n+\tgit status --porcelain >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success SYMLINKS 'symlink replacing a directory' '\n+\ttest_when_finished \"rm -rf copy && git reset --hard initial\" &&\n+\tmkdir copy &&\n+\tcp dir/file1 copy/file1 &&\n+\techo \"changed in copy\" >copy/file2 &&\n+\tgit add copy &&\n+\tgit commit -m second &&\n+\trm -rf copy &&\n+\tln -s dir copy &&\n+\techo \" D copy/file1\" >expect &&\n+\techo \" D copy/file2\" >>expect &&\n+\techo \"?? copy\" >>expect &&\n+\tgit status --porcelain >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.0.2\n"}]}