{"thread":{"id":"47483","subject":"[PATCH] status: add a failing test showing a core.untrackedCache bug","startedAt":"2017-12-22T14:00:48Z","lastAt":"2017-12-28T18:34:27Z","messageCount":12,"participants":["Ævar Arnfjörð Bjarmason","Duy Nguyen","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"335169","messageId":"20171222140032.21180-1-avarab@gmail.com","threadId":"47483","inReplyTo":null,"subject":"[PATCH] status: add a failing test showing a core.untrackedCache bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-22T14:00:32Z","receivedAt":"2017-12-22T14:00:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The untracked cache gets confused when a directory is swapped out for\na symlink to another directory. Whatever files are inside the target\nof the symlink will be incorrectly shown as untracked. This issue does\nnot happen if the symlink links to another file, only if it links to\nanother directory.\n\nA stand-alone testcase for copying into a terminal:\n\n (\n rm -rf /tmp/testrepo &&\n git init /tmp/testrepo &&\n cd /tmp/testrepo &&\n mkdir x y &&\n touch x/a y/b &&\n git add x/a y/b &&\n git commit -msnap &&\n git rm -rf y &&\n ln -s x y &&\n git add y &&\n git commit -msnap2 &&\n git checkout HEAD~ &&\n git status &&\n git checkout master &&\n sleep 1 &&\n git status &&\n git status\n )\n\nThis will incorrectly show y/a as an untracked file. Both the \"git\nstatus\" call right before \"git checkout master\" and the \"sleep 1\"\nafter the \"checkout master\" are needed to reproduce this, presumably\ndue to the untracked cache tracking on the basis of cached whole\nseconds from stat(2).\n\nWhen git gets into this state, a workaround to fix it is to issue a\none-off:\n\n git -c core.untrackedCache=false status\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7063-status-untracked-cache.sh | 45 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 45 insertions(+)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex e5fb892f95..7cf1e2c091 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -22,6 +22,12 @@ avoid_racy() {\n \tsleep 1\n }\n \n+status_is_clean() {\n+\t>../status.expect &&\n+\tgit status --porcelain >../status.actual &&\n+\ttest_cmp ../status.expect ../status.actual\n+}\n+\n test_lazy_prereq UNTRACKED_CACHE '\n \t{ git update-index --test-untracked-cache; ret=$?; } &&\n \ttest $ret -ne 1\n@@ -683,4 +689,43 @@ test_expect_success 'untracked cache survives a commit' '\n \ttest_cmp ../before ../after\n '\n \n+test_expect_success 'teardown worktree' '\n+ cd ..\n+'\n+\n+test_expect_success 'setup worktree for symlink test' '\n+\tgit init worktree-symlink &&\n+\tcd worktree-symlink &&\n+\tgit config core.untrackedCache true &&\n+\tmkdir one two &&\n+\ttouch one/file two/file &&\n+\tgit add one/file two/file &&\n+\tgit commit -m\"first commit\" &&\n+\tgit rm -rf one &&\n+\tln -s two one &&\n+\tgit add one &&\n+\tgit commit -m\"second commit\"\n+'\n+\n+test_expect_failure '\"status\" after symlink replacement should be clean with UC=true' '\n+\tgit checkout HEAD~ &&\n+\tstatus_is_clean &&\n+\tstatus_is_clean &&\n+\tgit checkout master &&\n+\tavoid_racy &&\n+\tstatus_is_clean &&\n+\tstatus_is_clean\n+'\n+\n+test_expect_success '\"status\" after symlink replacement should be clean with UC=false' '\n+\tgit config core.untrackedCache false &&\n+\tgit checkout HEAD~ &&\n+\tstatus_is_clean &&\n+\tstatus_is_clean &&\n+\tgit checkout master &&\n+\tavoid_racy &&\n+\tstatus_is_clean &&\n+\tstatus_is_clean\n+'\n+\n test_done\n-- \n2.15.1.424.g9478a66081\n\n"},{"id":"335283","messageId":"CACsJy8B1FNpq-AYJdcs_gVOxdPSnh-kNaeVykLSSDL1+EW9YjA@mail.gmail.com","threadId":"47483","inReplyTo":"20171222140032.21180-1-avarab@gmail.com","subject":"Re: [PATCH] status: add a failing test showing a core.untrackedCache bug","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-12-25T11:26:06Z","receivedAt":"2017-12-25T11:26:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Dec 22, 2017 at 9:00 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> The untracked cache gets confused when a directory is swapped out for\n> a symlink to another directory. Whatever files are inside the target\n> of the symlink will be incorrectly shown as untracked. This issue does\n> not happen if the symlink links to another file, only if it links to\n> another directory.\n\nSounds about right (I completely forgot about dir symlinks). Since\nI've been away for some time and have not caught up (probably cannot)\nwith the mailing list yet, is anyone working on this? It may be\neasiest to just detect symlinksand disable the cache for now.\n-- \nDuy\n"},{"id":"335299","messageId":"87wp1ar6j1.fsf@evledraar.gmail.com","threadId":"47483","inReplyTo":"CACsJy8B1FNpq-AYJdcs_gVOxdPSnh-kNaeVykLSSDL1+EW9YjA@mail.gmail.com","subject":"Re: [PATCH] status: add a failing test showing a core.untrackedCache bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-25T18:45:06Z","receivedAt":"2017-12-25T19:06:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 25 2017, Duy Nguyen jotted:\n\n> On Fri, Dec 22, 2017 at 9:00 PM, Ævar Arnfjörð Bjarmason\n> wrote:\n>> The untracked cache gets confused when a directory is swapped out for\n>> a symlink to another directory. Whatever files are inside the target\n>> of the symlink will be incorrectly shown as untracked. This issue does\n>> not happen if the symlink links to another file, only if it links to\n>> another directory.\n>\n> Sounds about right (I completely forgot about dir symlinks). Since\n> I've been away for some time and have not caught up (probably cannot)\n> with the mailing list yet, is anyone working on this? It may be\n> easiest to just detect symlinksand disable the cache for now.\n\nI haven't yet, I wanted to see what you had to say about it,\ni.e. whether it was a \"do'h here's a fix\" or if it was more involved\nthan that.\n\nBeing completely unfamiliar with this code, I hacked up [1] to add some\nad-hoc tracing to this. It has some bugs and doesn't actually work, but\nis injecting something into valid_cached_dir() and checking if the\ndirectory in question is a symlink the right approach?\n\nAlthough surely a better solution is to just see that y is a symlink to\nx, and use the data we get for x.\n\nI also see that the the untracked_cache_dir struct has a stat_data field\nwhich contains a subset of what we get from stat(), but it doesn't have\nst_mode, so you can't see from that if the thing was a symlink (but it\ncould be added).\n\nIs that the right approach? I.e. saving away whether it was a symlink\nand if it changes invalidate the cache, although it could be a symlink\nto something else, so may it needs to be keyed on st_ino (but that may\nbe chagned in-place?).\n\n1.\n\n diff --git a/dir.c b/dir.c\n index 3c54366a17..8afe068c72 100644\n --- a/dir.c\n +++ b/dir.c\n @@ -1730,10 +1730,13 @@ static int valid_cached_dir(struct dir_struct *dir,\n int check_only)\n {\n struct stat st;\n + struct stat st2;\n\n if (!untracked)\n return 0;\n\n + fprintf(stderr, \"Checking <%s>\\n\", path->buf);\n +\n /*\n * With fsmonitor, we can trust the untracked cache's valid field.\n */\n @@ -1742,6 +1745,7 @@ static int valid_cached_dir(struct dir_struct *dir,\n if (stat(path->len ? path->buf : \".\", &st)) {\n invalidate_directory(dir->untracked, untracked);\n memset(&untracked->stat_data, 0, sizeof(untracked->stat_data));\n + fprintf(stderr, \"Ret #1 = 0\\n\");\n return 0;\n }\n if (!untracked->valid ||\n @@ -1749,12 +1753,14 @@ static int valid_cached_dir(struct dir_struct *dir,\n if (untracked->valid)\n invalidate_directory(dir->untracked, untracked);\n fill_stat_data(&untracked->stat_data, &st);\n + fprintf(stderr, \"Ret #2 = 0\\n\");\n return 0;\n }\n }\n\n if (untracked->check_only != !!check_only) {\n invalidate_directory(dir->untracked, untracked);\n + fprintf(stderr, \"Ret #3 = 0\\n\");\n return 0;\n }\n\n @@ -1772,6 +1778,28 @@ static int valid_cached_dir(struct dir_struct *dir,\n } else\n prep_exclude(dir, istate, path->buf, path->len);\n\n + if (path->len && path->buf[path->len - 1] == '/') {\n + struct strbuf dirbuf = STRBUF_INIT;\n + strbuf_add(&dirbuf, path->buf, path->len - 1);\n + fprintf(stderr, \"Just dir = <%s>\\n\", dirbuf.buf);\n +\n + if (lstat(dirbuf.buf, &st2)) {\n + fprintf(stderr, \"Ret #4 = 0\\n\");\n + return 0;\n + } else if (S_ISLNK(st2.st_mode)) {\n + invalidate_directory(dir->untracked, untracked);\n + memset(&untracked->stat_data, 0, sizeof(untracked->stat_data));\n + fill_stat_data(&untracked->stat_data, &st);\n + fprintf(stderr, \"Is link = <%s>\\n\", dirbuf.buf);\n + return 0;\n + } else {\n + fprintf(stderr, \"Is not link = <%s> but <%d>\\n\", dirbuf.buf, st2.st_mode);\n + }\n + }\n +\n + fprintf(stderr, \"Falling through for <%s>\\n\", path->buf);\n +\n +\n /* hopefully prep_exclude() haven't invalidated this entry... */\n return untracked->valid;\n }\n @@ -1783,9 +1811,12 @@ static int open_cached_dir(struct cached_dir *cdir,\n struct strbuf *path,\n int check_only)\n {\n + int valid;\n memset(cdir, 0, sizeof(*cdir));\n cdir->untracked = untracked;\n - if (valid_cached_dir(dir, untracked, istate, path, check_only))\n + valid = valid_cached_dir(dir, untracked, istate, path, check_only);\n + fprintf(stderr, \"Checked <%s>, valid? <%d>\\n\", path->buf, valid);\n + if (valid)\n return 0;\n cdir->fdir = opendir(path->len ? path->buf : \".\");\n if (dir->untracked)\n"},{"id":"335322","messageId":"CACsJy8AmbKSp0mDLRaDCWn45veeNc03B-Gq8r8PQPrDt9bM_EA@mail.gmail.com","threadId":"47483","inReplyTo":"87wp1ar6j1.fsf@evledraar.gmail.com","subject":"Re: [PATCH] status: add a failing test showing a core.untrackedCache bug","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-12-26T10:47:19Z","receivedAt":"2017-12-26T10:47:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Dec 26, 2017 at 1:45 AM, Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Mon, Dec 25 2017, Duy Nguyen jotted:\n>\n>> On Fri, Dec 22, 2017 at 9:00 PM, Ævar Arnfjörð Bjarmason\n>> wrote:\n>>> The untracked cache gets confused when a directory is swapped out for\n>>> a symlink to another directory. Whatever files are inside the target\n>>> of the symlink will be incorrectly shown as untracked. This issue does\n>>> not happen if the symlink links to another file, only if it links to\n>>> another directory.\n>>\n>> Sounds about right (I completely forgot about dir symlinks). Since\n>> I've been away for some time and have not caught up (probably cannot)\n>> with the mailing list yet, is anyone working on this? It may be\n>> easiest to just detect symlinksand disable the cache for now.\n>\n> I haven't yet, I wanted to see what you had to say about it,\n> i.e. whether it was a \"do'h here's a fix\" or if it was more involved\n> than that.\n\nOK I have more time to actually read your test and play with it (last\ntime I stopped at the word \"symlink\").\n\nFirst thing out of the way first, I think the stat() call in\nvalid_cached_dir() is wrong. We don't want to automatically follow a\nsymlink, we want stat of the symlink itself.\n\nOK back to the test. If you insert test-dump-untracked-cached around\nthe \"git checkout master\" line, then we get the data that the\nnext/faulty git status uses. For me it shows this\n\n\n...\n/ 0000000000000000000000000000000000000000 recurse\n/one/ 0000000000000000000000000000000000000000 recurse valid\n/two/ 0000000000000000000000000000000000000000 recurse valid\n\nOK so we have two uptodate cached dirs for \"one\" and \"two\". Strangely,\nroot dir is not cached (no valid flag). I don't know why but that's ok\nwe'll roll with it. It means we will ignore \"/\" content and do the\nopendir() and readdir() dance instead.\n\nThis is where it gets fun, when readdir() returns \"one\", we hit this\ncode in treat_one_path() and correctly ignores it\n\n /* Always exclude indexed files */\n if (dtype != DT_DIR && has_path_in_index)\n return path_none;\n\nand we do _nothing_ towards the cached version of \"one\". In constrast,\nwhen readdir() returns \"two\" we are able to get further and invalidate\nit, deleting the cached data.\n\nAfter the readdir() dance is complete, dir.c is confident that it has\nall the good data in the world in its cache and notes down \"from now\non uses the cached data for /\". Another test-dump-... after the\nsecond-to-last git-status can show this \"valid\" flag. Unfortunately\nthe \"one\" dir is NOT invalidated.\n\nSo the last git-status sees that cached \"/\" is good, it skips\nopendir() and goes with the cached directories which are \"two\" and the\nbad \"one\". In short, we fail to invalidate bad data in some case.\n\nAn invalidate_directory() call before the \"return path_none\" above\nmight be the solution...\n-- \nDuy\n"},{"id":"335354","messageId":"20171227102551.GA26616@ash","threadId":"47483","inReplyTo":"CACsJy8AmbKSp0mDLRaDCWn45veeNc03B-Gq8r8PQPrDt9bM_EA@mail.gmail.com","subject":"Re: [PATCH] status: add a failing test showing a core.untrackedCache bug","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-12-27T10:25:51Z","receivedAt":"2017-12-27T10:26:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Dec 26, 2017 at 05:47:19PM +0700, Duy Nguyen wrote:\n> Strangely, root dir is not cached (no valid flag). I don't know why\n> but that's ok we'll roll with it.\n\nI figured this out. Which is good because now I know how/why the bug\nhappens.\n\n> An invalidate_directory() call before the \"return path_none\" above\n> might be the solution...\n\nNope. This is better. I think checking out any d/f updates would cause\nthis, not just symlinks.\n\n-- 8< --\nSubject: [PATCH] dir.c: fix missing dir invalidation in untracked code\n\nLet's start with how create a new directory cache after the last one\nbecomes invalid (e.g. because its dir mtime has changed...). In\nopen_cached_dir():\n\n1. We start out with valid_cached_dir() returning false, which should\n call invalidate_directory() to put a directory state back to\n initial state, no untracked entries (untracked_nr zero), no sub\n directory traversal (dirs[].recurse zero).\n\n2. Since the cache cannot be used, we go the slow path opendir() and\n go through items one by one via readdir(). All the directories on\n disk will be added back to the cache (if not already exist in\n dirs[]) and its flag \"recurse\" gets changed to one to note that\n it's part of the cached dir travesal next time.\n\n3. By the time we reach close_cached_dir() we should have a good\n subdir list in dirs[]. Those with \"recurse\" flag set are the ones\n present in the on-disk directory. The directory is now marked\n \"valid\".\n\nNext time read_directory() is called, since the directory is marked\nvalid, it will skip readdir(), go fast path and traverse through\ndirs[] array instead.\n\nSteps one and two need some tight cooperation. If a subdir is removed,\nreaddir() will not find it and of course we cannot examine/invalidate\nit. To make sure removed directories on disk are gone from the cache,\nstep one must make sure recurse flag of all subdirs are zero.\n\nBut that's not true. If \"valid\" flag is already false, there is a\nchance we go straight to the end of valid_cached_dir() without calling\ninvalidate_directory(). Or we fail to meet the \"if (untracked-valid)\"\ncondition and skip over the invalidate_directory().\n\nAfter step 3, we mark the cache valid. Any stale subdir with incorrect\nrecurse flagbecomes a real subdir next time we traverse the directory\nusing dirs[] array.\n\nWe could avoid this by making sure invalidate_directory() is always\ncalled (therefore dirs[].recurse cleared) at the beginning of\nopen_cached_dir(). Which is what this patch does.\n\nAs to how we get into this situation, the key in the test is this\ncommand\n\n git checkout master\n\nwhere \"one/file\" is replaced with \"one\" in the index. This index\nupdate triggers untracked_cache_invalidate_path(), which clears valid\nflag of the root directory while keeping \"recurse\" flag on the subdir\n\"one\" on. On the next git-status, we go through steps 1-3 above and\nsave an incorrect cache on disk. The second git-status blindly follows\nthe bad cache data and shows the problem.\n\nThis is arguably because of a bad design where \"recurse\" flag plays\ndouble roles: whether a directory should be saved on disk, and whether\nit is part of a directory traversal.\n\nWe need to keep recurse flag set at \"checkout master\" because of the\nfirst role: we need to keep subdir caches (dir \"two\" for example has\nnot been touched at all, no reason to throw its cache away).\n\nAs long as we make sure to ignore/reset \"recurse\" flag at the\nbeginning of a directory traversal, we're good. But maybe eventually\nwe should separate these two roles.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n dir.c | 22 ++++++++++++++--------\n t/t7063-status-untracked-cache.sh | 2 +-\n 2 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 3c54366a17..cca88247d3 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -733,7 +733,16 @@ static void invalidate_directory(struct untracked_cache *uc,\n \t\t\t\t struct untracked_cache_dir *dir)\n {\n \tint i;\n-\tuc->dir_invalidated++;\n+\n+\t/*\n+\t * Invalidation increment here is just roughly correct. If\n+\t * untracked_nr or any of dirs[].recurse is non-zero, we\n+\t * should increment dir_invalidated too. But that's more\n+\t * expensive to do.\n+\t */\n+\tif (dir->valid)\n+\t\tuc->dir_invalidated++;\n+\n \tdir->valid = 0;\n \tdir->untracked_nr = 0;\n \tfor (i = 0; i < dir->dirs_nr; i++)\n@@ -1740,23 +1749,18 @@ static int valid_cached_dir(struct dir_struct *dir,\n \trefresh_fsmonitor(istate);\n \tif (!(dir->untracked->use_fsmonitor && untracked->valid)) {\n \t\tif (stat(path->len ? path->buf : \".\", &st)) {\n-\t\t\tinvalidate_directory(dir->untracked, untracked);\n \t\t\tmemset(&untracked->stat_data, 0, sizeof(untracked->stat_data));\n \t\t\treturn 0;\n \t\t}\n \t\tif (!untracked->valid ||\n \t\t\tmatch_stat_data_racy(istate, &untracked->stat_data, &st)) {\n-\t\t\tif (untracked->valid)\n-\t\t\t\tinvalidate_directory(dir->untracked, untracked);\n \t\t\tfill_stat_data(&untracked->stat_data, &st);\n \t\t\treturn 0;\n \t\t}\n \t}\n \n-\tif (untracked->check_only != !!check_only) {\n-\t\tinvalidate_directory(dir->untracked, untracked);\n+\tif (untracked->check_only != !!check_only)\n \t\treturn 0;\n-\t}\n \n \t/*\n \t * prep_exclude will be called eventually on this directory,\n@@ -1788,8 +1792,10 @@ static int open_cached_dir(struct cached_dir *cdir,\n \tif (valid_cached_dir(dir, untracked, istate, path, check_only))\n \t\treturn 0;\n \tcdir->fdir = opendir(path->len ? path->buf : \".\");\n-\tif (dir->untracked)\n+\tif (dir->untracked) {\n+\t\tinvalidate_directory(dir->untracked, untracked);\n \t\tdir->untracked->dir_opened++;\n+\t}\n \tif (!cdir->fdir)\n \t\treturn -1;\n \treturn 0;\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex 7cf1e2c091..8f5ef32525 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -707,7 +707,7 @@ test_expect_success 'setup worktree for symlink test' '\n \tgit commit -m\"second commit\"\n '\n \n-test_expect_failure '\"status\" after symlink replacement should be clean with UC=true' '\n+test_expect_success '\"status\" after symlink replacement should be clean with UC=true' '\n \tgit checkout HEAD~ &&\n \tstatus_is_clean &&\n \tstatus_is_clean &&\n-- \n2.15.0.320.g0453912d77\n\n-- 8< --\n"},{"id":"335355","messageId":"20171227112854.1322-1-avarab@gmail.com","threadId":"47483","inReplyTo":"20171227102551.GA26616@ash","subject":"[PATCH 3/1] update-index doc: note a fixed bug in the untracked cache","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-27T11:28:54Z","receivedAt":"2017-12-27T11:29:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Document the bug tested for in my \"status: add a failing test showing\na core.untrackedCache bug\" and fixed in Duy's \"dir.c: fix missing dir\ninvalidation in untracked code\".\n\nSince this is very likely something others will encounter in the\nfuture on older versions, and it's not obvious how to fix it let's\ndocument both that it exists, and how to \"fix\" it with a one-off\ncommand.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\n> On Tue, Dec 26, 2017 at 05:47:19PM +0700, Duy Nguyen wrote:\n>> Strangely, root dir is not cached (no valid flag). I don't know why\n>> but that's ok we'll roll with it.\n>\n> I figured this out. Which is good because now I know how/why the bug\n> happens.\n\nThanks a lot. I think it's probably good to apply something like this\non top of this now 3-patch series.\n\n Documentation/git-update-index.txt | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt\nindex bdb0342593..bc6c32002f 100644\n--- a/Documentation/git-update-index.txt\n+++ b/Documentation/git-update-index.txt\n@@ -464,6 +464,17 @@ command reads the index; while when `--[no-|force-]untracked-cache`\n are used, the untracked cache is immediately added to or removed from\n the index.\n \n+Before 2.16, the untracked cache had a bug where replacing a directory\n+with a symlink to another directory could cause it to incorrectly show\n+files tracked by git as untracked. See the \"status: add a failing test\n+showing a core.untrackedCache bug\" commit to git.git. A workaround for\n+that was (and this might work for other undiscoverd bugs in the\n+future):\n+\n+----------------\n+$ git -c core.untrackedCache=false status\n+----------------\n+\n File System Monitor\n -------------------\n \n-- \n2.15.1.424.g9478a66081\n\n"},{"id":"335366","messageId":"CAPig+cSuWivA-YKnv7q6y1zFebzq72Y2F+ioPmiNEO9VQeQSNQ@mail.gmail.com","threadId":"47483","inReplyTo":"20171227102551.GA26616@ash","subject":"Re: [PATCH] status: add a failing test showing a core.untrackedCache bug","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-12-27T17:50:57Z","receivedAt":"2017-12-27T17:51:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 27, 2017 at 5:25 AM, Duy Nguyen wrote:\n> Subject: [PATCH] dir.c: fix missing dir invalidation in untracked code\n> [...]\n> After step 3, we mark the cache valid. Any stale subdir with incorrect\n> recurse flagbecomes a real subdir next time we traverse the directory\n\ns/flagbecomes/flag becomes/\n\n> using dirs[] array.\n> [...]\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n"},{"id":"335368","messageId":"xmqqpo70nixy.fsf@gitster.mtv.corp.google.com","threadId":"47483","inReplyTo":"20171227112854.1322-1-avarab@gmail.com","subject":"Re: [PATCH 3/1] update-index doc: note a fixed bug in the untracked cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-27T18:07:21Z","receivedAt":"2017-12-27T18:07:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Document the bug tested for in my \"status: add a failing test showing\n> a core.untrackedCache bug\" and fixed in Duy's \"dir.c: fix missing dir\n> invalidation in untracked code\".\n>\n> Since this is very likely something others will encounter in the\n> future on older versions, and it's not obvious how to fix it let's\n> document both that it exists, and how to \"fix\" it with a one-off\n> command.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n>\n>> On Tue, Dec 26, 2017 at 05:47:19PM +0700, Duy Nguyen wrote:\n>>> Strangely, root dir is not cached (no valid flag). I don't know why\n>>> but that's ok we'll roll with it.\n>>\n>> I figured this out. Which is good because now I know how/why the bug\n>> happens.\n>\n> Thanks a lot. I think it's probably good to apply something like this\n> on top of this now 3-patch series.\n\nThanks both for working well together. Now that the problem turns\nout not about symbolic links, can we update the test in 1/1 not to\ndepend on symbolic links?\n\n>\n> Documentation/git-update-index.txt | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n>\n> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt\n> index bdb0342593..bc6c32002f 100644\n> --- a/Documentation/git-update-index.txt\n> +++ b/Documentation/git-update-index.txt\n> @@ -464,6 +464,17 @@ command reads the index; while when `--[no-|force-]untracked-cache`\n> are used, the untracked cache is immediately added to or removed from\n> the index.\n> \n> +Before 2.16, the untracked cache had a bug where replacing a directory\n> +with a symlink to another directory could cause it to incorrectly show\n> +files tracked by git as untracked. See the \"status: add a failing test\n> +showing a core.untrackedCache bug\" commit to git.git. A workaround for\n> +that was (and this might work for other undiscoverd bugs in the\n> +future):\n> +\n> +----------------\n> +$ git -c core.untrackedCache=false status\n> +----------------\n> +\n> File System Monitor\n> -------------------\n"},{"id":"335369","messageId":"xmqqlghoniuk.fsf@gitster.mtv.corp.google.com","threadId":"47483","inReplyTo":"20171222140032.21180-1-avarab@gmail.com","subject":"Re: [PATCH] status: add a failing test showing a core.untrackedCache bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-27T18:09:23Z","receivedAt":"2017-12-27T18:09:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +status_is_clean() {\n> +\t>../status.expect &&\n> +\tgit status --porcelain >../status.actual &&\n> +\ttest_cmp ../status.expect ../status.actual\n> +}\n> +\n> test_lazy_prereq UNTRACKED_CACHE '\n> \t{ git update-index --test-untracked-cache; ret=$?; } &&\n> \ttest $ret -ne 1\n> @@ -683,4 +689,43 @@ test_expect_success 'untracked cache survives a commit' '\n> \ttest_cmp ../before ../after\n> '\n> \n> +test_expect_success 'teardown worktree' '\n> + cd ..\n> +'\n\nFunny indentation.\n\n> +test_expect_success 'setup worktree for symlink test' '\n> +\tgit init worktree-symlink &&\n> +\tcd worktree-symlink &&\n> +\tgit config core.untrackedCache true &&\n> +\tmkdir one two &&\n> +\ttouch one/file two/file &&\n> +\tgit add one/file two/file &&\n> +\tgit commit -m\"first commit\" &&\n> +\tgit rm -rf one &&\n> +\tln -s two one &&\n> +\tgit add one &&\n> +\tgit commit -m\"second commit\"\n> +'\n\nThis needs SYMLINKS prereq, which in turn means it cannot be tested\non certain platforms. I thought Duy's answer was that this does not\nneed to involve a symbolic link at all? If so, perhaps we can have\nanother test that does not need symlink?\n\nThanks.\n"},{"id":"335379","messageId":"87mv24qa2y.fsf@evledraar.gmail.com","threadId":"47483","inReplyTo":"xmqqlghoniuk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] status: add a failing test showing a core.untrackedCache bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-27T18:50:29Z","receivedAt":"2017-12-27T18:50:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 27 2017, Junio C. Hamano jotted:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> +status_is_clean() {\n>> +\t>../status.expect &&\n>> +\tgit status --porcelain >../status.actual &&\n>> +\ttest_cmp ../status.expect ../status.actual\n>> +}\n>> +\n>> test_lazy_prereq UNTRACKED_CACHE '\n>> \t{ git update-index --test-untracked-cache; ret=$?; } &&\n>> \ttest $ret -ne 1\n>> @@ -683,4 +689,43 @@ test_expect_success 'untracked cache survives a commit' '\n>> \ttest_cmp ../before ../after\n>> '\n>>\n>> +test_expect_success 'teardown worktree' '\n>> + cd ..\n>> +'\n>\n> Funny indentation.\n\nThanks. I'll submit a new series with all 3 patches with the issues you\nbrought up mentioned...\n\n>> +test_expect_success 'setup worktree for symlink test' '\n>> +\tgit init worktree-symlink &&\n>> +\tcd worktree-symlink &&\n>> +\tgit config core.untrackedCache true &&\n>> +\tmkdir one two &&\n>> +\ttouch one/file two/file &&\n>> +\tgit add one/file two/file &&\n>> +\tgit commit -m\"first commit\" &&\n>> +\tgit rm -rf one &&\n>> +\tln -s two one &&\n>> +\tgit add one &&\n>> +\tgit commit -m\"second commit\"\n>> +'\n>\n> This needs SYMLINKS prereq, which in turn means it cannot be tested\n> on certain platforms. I thought Duy's answer was that this does not\n> need to involve a symbolic link at all? If so, perhaps we can have\n> another test that does not need symlink?\n\n... as soon as I figure out how to add such a non-symlink test as well\n(seems sensible to test both), but I can't bring it to fail without\nsymlinks, I just adjusted my test script to do this instead:\n\n (\n rm -rf /tmp/testrepo &&\n git init /tmp/testrepo &&\n cd /tmp/testrepo &&\n mkdir x y &&\n touch x/a y/b &&\n git add x/a y/b &&\n git commit -msnap &&\n git rm -rf y &&\n mkdir y &&\n touch y/c &&\n git add y &&\n git commit -msnap2 &&\n git checkout HEAD~ &&\n git status &&\n git checkout master &&\n sleep 1 &&\n git status &&\n git status\n )\n\nDuy, what am I missing here?\n"},{"id":"335457","messageId":"20171228061027.GA22308@duynguyen.vn.dektech.internal","threadId":"47483","inReplyTo":"87mv24qa2y.fsf@evledraar.gmail.com","subject":"Re: [PATCH] status: add a failing test showing a core.untrackedCache bug","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-12-28T06:10:27Z","receivedAt":"2017-12-28T06:10:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 27, 2017 at 07:50:29PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > This needs SYMLINKS prereq, which in turn means it cannot be tested\n> > on certain platforms. I thought Duy's answer was that this does not\n> > need to involve a symbolic link at all? If so, perhaps we can have\n> > another test that does not need symlink?\n> \n> ... as soon as I figure out how to add such a non-symlink test as well\n> (seems sensible to test both), but I can't bring it to fail without\n> symlinks, I just adjusted my test script to do this instead:\n> \n> (\n> rm -rf /tmp/testrepo &&\n> git init /tmp/testrepo &&\n> cd /tmp/testrepo &&\n> mkdir x y &&\n> touch x/a y/b &&\n> git add x/a y/b &&\n> git commit -msnap &&\n> git rm -rf y &&\n> mkdir y &&\n> touch y/c &&\n> git add y &&\n> git commit -msnap2 &&\n> git checkout HEAD~ &&\n> git status &&\n> git checkout master &&\n> sleep 1 &&\n> git status &&\n> git status\n> )\n> \n> Duy, what am I missing here?\n\nThe problem is there, it's just easier to see or verify with\nsymlinks. Below is my test patch on top of your original one. Two\npoints:\n\n- if you look at the test-dump-untracked-cache output, you can see the\n saved cache is wrong. The line\n\n /one/ 0000000000000000000000000000000000000000 recurse valid\n\n should not be there because that implies that cached travesal of\n root includes the directory \"one\" which does not exist on disk\n anymore. With the fix, this line is gone.\n\n- We silently ignore opendir() error, the changes in dir.c shows this\n\n warning: could not open directory 'one/': Not a directory\n\n It opendir() again because it finds out the stat data of directory\n \"one\" in the cache does not match stat data of the (real) file\n \"one\".\n\n If \"one\" is a symlink, opendir() would be succesful and we go in\n anyway. If it's a file, we ignore it, accidentally make the second\n git-status output clean and pass the test.\n\nReport opendir() errors is a good and should be done regardless (i'm\njust not sure if it should be a fatal error or a warning like this, I\nguess die() is a bit too much).\n\nThe remaining question is how we write this test. Verify with\ntest-dump-untracked-cache is easiest but less intuitive, I\nguess. While using symlinks shows the problem clearly but not\nportable. Or the third option, if we error something out, you could\ncheck that git-status has clean stderr.\n\nWhich way to go?\n\n-- 8< --\ndiff --git a/dir.c b/dir.c\nindex 3c54366a17..868f544d72 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1783,15 +1783,20 @@ static int open_cached_dir(struct cached_dir *cdir,\n \t\t\t struct strbuf *path,\n \t\t\t int check_only)\n {\n+\tconst char *c_path;\n+\n \tmemset(cdir, 0, sizeof(*cdir));\n \tcdir->untracked = untracked;\n \tif (valid_cached_dir(dir, untracked, istate, path, check_only))\n \t\treturn 0;\n-\tcdir->fdir = opendir(path->len ? path->buf : \".\");\n+\tc_path = path->len ? path->buf : \".\";\n+\tcdir->fdir = opendir(c_path);\n \tif (dir->untracked)\n \t\tdir->untracked->dir_opened++;\n-\tif (!cdir->fdir)\n+\tif (!cdir->fdir) {\n+\t\twarning_errno(_(\"could not open directory '%s'\"), c_path);\n \t\treturn -1;\n+\t}\n \treturn 0;\n }\n \ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex 7cf1e2c091..ca63b80ca7 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -702,7 +702,7 @@ test_expect_success 'setup worktree for symlink test' '\n \tgit add one/file two/file &&\n \tgit commit -m\"first commit\" &&\n \tgit rm -rf one &&\n-\tln -s two one &&\n+\tcp two/file one &&\n \tgit add one &&\n \tgit commit -m\"second commit\"\n '\n@@ -714,6 +714,7 @@ test_expect_failure '\"status\" after symlink replacement should be clean with UC=\n \tgit checkout master &&\n \tavoid_racy &&\n \tstatus_is_clean &&\n+\ttest-dump-untracked-cache &&\n \tstatus_is_clean\n '\n \n-- 8< --\n"},{"id":"335472","messageId":"xmqq1sjen1lh.fsf@gitster.mtv.corp.google.com","threadId":"47483","inReplyTo":"20171228061027.GA22308@duynguyen.vn.dektech.internal","subject":"Re: [PATCH] status: add a failing test showing a core.untrackedCache bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-28T18:34:18Z","receivedAt":"2017-12-28T18:34:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> Duy, what am I missing here?\n>\n> The problem is there, it's just easier to see or verify with\n> symlinks. Below is my test patch on top of your original one. Two\n> points:\n>\n> - if you look at the test-dump-untracked-cache output, you can see the\n> saved cache is wrong. The line\n>\n> /one/ 0000000000000000000000000000000000000000 recurse valid\n>\n> should not be there because that implies that cached travesal of\n> root includes the directory \"one\" which does not exist on disk\n> anymore. With the fix, this line is gone.\n\nNice.\n\n> - We silently ignore opendir() error, the changes in dir.c shows this\n> ...\n> Report opendir() errors is a good and should be done regardless (i'm\n> just not sure if it should be a fatal error or a warning like this, I\n> guess die() is a bit too much).\n\nThanks; I agree that it definitely would be a good change to issue a\nwarning there.\n"}]}