{"thread":{"id":"47630","subject":"[PATCH] dir.c: print correct errno when opendir() fails","startedAt":"2018-01-18T09:51:02Z","lastAt":"2018-02-01T23:00:24Z","messageCount":12,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"336762","messageId":"20180118095036.29422-1-pclouds@gmail.com","threadId":"47630","inReplyTo":null,"subject":"[PATCH] dir.c: print correct errno when opendir() fails","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-18T09:50:36Z","receivedAt":"2018-01-18T09:51:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The call invalidate_directory() between opendir() and warning_errno() in\ntheory could make some system calls and change errno. Prevent that by\nwarning immediately after opendir().\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n This is on top of nd/fix-untracked-cache-invalidation which is now on\n 'next'. Sorry I waited too long to send the replacement and it's now\n too late.\n\n dir.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex dd1e50c328..55736d3e2a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1795,14 +1795,14 @@ static int open_cached_dir(struct cached_dir *cdir,\n \t\treturn 0;\n \tc_path = path->len ? path->buf : \".\";\n \tcdir->fdir = opendir(c_path);\n+\tif (!cdir->fdir)\n+\t\twarning_errno(_(\"could not open directory '%s'\"), c_path);\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\twarning_errno(_(\"could not open directory '%s'\"), c_path);\n+\tif (!cdir->fdir)\n \t\treturn -1;\n-\t}\n \treturn 0;\n }\n \n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336935","messageId":"xmqqa7x9pnda.fsf@gitster.mtv.corp.google.com","threadId":"47630","inReplyTo":"20180118095036.29422-1-pclouds@gmail.com","subject":"Re: [PATCH] dir.c: print correct errno when opendir() fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-19T21:09:05Z","receivedAt":"2018-01-19T21:09:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> The call invalidate_directory() between opendir() and warning_errno() in\n> theory could make some system calls and change errno. Prevent that by\n> warning immediately after opendir().\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> This is on top of nd/fix-untracked-cache-invalidation which is now on\n> 'next'. Sorry I waited too long to send the replacement and it's now\n> too late.\n\nWell, we'll see a rewind of 'next' soonish anyway, so you can just\ntell me to tentatively kick it back to 'pu' to be replaced with a\nreroll if you prefer.\n\n\n\n\n> dir.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index dd1e50c328..55736d3e2a 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1795,14 +1795,14 @@ static int open_cached_dir(struct cached_dir *cdir,\n> \t\treturn 0;\n> \tc_path = path->len ? path->buf : \".\";\n> \tcdir->fdir = opendir(c_path);\n> +\tif (!cdir->fdir)\n> +\t\twarning_errno(_(\"could not open directory '%s'\"), c_path);\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\twarning_errno(_(\"could not open directory '%s'\"), c_path);\n> +\tif (!cdir->fdir)\n> \t\treturn -1;\n> -\t}\n> \treturn 0;\n> }\n"},{"id":"337025","messageId":"CACsJy8AecDh6Bxo6xCBG+4AY5GCdtZgb2Cs2QXo5MYZvD7XKcQ@mail.gmail.com","threadId":"47630","inReplyTo":"xmqqa7x9pnda.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] dir.c: print correct errno when opendir() fails","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-21T11:52:08Z","receivedAt":"2018-01-21T11:52:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 20, 2018 at 4:09 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> The call invalidate_directory() between opendir() and warning_errno() in\n>> theory could make some system calls and change errno. Prevent that by\n>> warning immediately after opendir().\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> This is on top of nd/fix-untracked-cache-invalidation which is now on\n>> 'next'. Sorry I waited too long to send the replacement and it's now\n>> too late.\n>\n> Well, we'll see a rewind of 'next' soonish anyway, so you can just\n> tell me to tentatively kick it back to 'pu' to be replaced with a\n> reroll if you prefer.\n\nPlease kick it back to 'pu'.\n-- \nDuy\n"},{"id":"337207","messageId":"20180124093023.9071-1-pclouds@gmail.com","threadId":"47630","inReplyTo":"20180118095036.29422-1-pclouds@gmail.com","subject":"[PATCH 0/5] nd/fix-untracked-cache-invalidation updates","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:30:18Z","receivedAt":"2018-01-24T09:30:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is a small update where the last two patches on 'pu' are merged\nin one. The final content is exactly the same as on 'pu'.\n\nNguyễn Thái Ngọc Duy (3):\n dir.c: avoid stat() in valid_cached_dir()\n dir.c: fix missing dir invalidation in untracked code\n dir.c: stop ignoring opendir() error in open_cached_dir()\n\nÆvar Arnfjörð Bjarmason (2):\n status: add a failing test showing a core.untrackedCache bug\n update-index doc: note a fixed bug in the untracked cache\n\n Documentation/git-update-index.txt | 16 +++++++\n dir.c | 31 +++++++++-----\n t/t7063-status-untracked-cache.sh | 87 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 124 insertions(+), 10 deletions(-)\n\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337208","messageId":"20180124093023.9071-2-pclouds@gmail.com","threadId":"47630","inReplyTo":"20180124093023.9071-1-pclouds@gmail.com","subject":"[PATCH 1/5] status: add a failing test showing a core.untrackedCache bug","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:30:19Z","receivedAt":"2018-01-24T09:30:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Ævar Arnfjörð Bjarmason \n\nThe untracked cache gets confused when a directory is swapped out for\na file. It is easiest to reproduce this by swapping out a directory\nwith a symlink to another directory, and as the tests show the symlink\ncase is the only case we've found where \"git status\" will subsequently\nreport incorrect information, even though it's possible to otherwise\nget the untracked cache into a state where its internal data\nstructures don't reflect reality.\n\nIn the symlink case, whatever files are inside the target of the\nsymlink will be incorrectly shown as untracked. This issue does not\nhappen 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\nFor the non-symlink case, the bug is that the output of\ntest-dump-untracked-cache should not include:\n\n /one/ 0000000000000000000000000000000000000000 recurse valid\n\nIt being in the output implies that cached traversal of root includes\nthe directory \"one\" which does not exist on disk anymore.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n t/t7063-status-untracked-cache.sh | 87 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 87 insertions(+)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex e5fb892f95..dba7f50bbb 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,85 @@ test_expect_success 'untracked cache survives a commit' '\n \ttest_cmp ../before ../after\n '\n \n+test_expect_success 'teardown worktree' '\n+\tcd ..\n+'\n+\n+test_expect_success SYMLINKS '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 SYMLINKS '\"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 SYMLINKS '\"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_expect_success 'setup worktree for non-symlink test' '\n+\tgit init worktree-non-symlink &&\n+\tcd worktree-non-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+\tcp two/file one &&\n+\tgit add one &&\n+\tgit commit -m\"second commit\"\n+'\n+\n+test_expect_failure '\"status\" after file 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+\ttest-dump-untracked-cache >../actual &&\n+\tgrep -F \"recurse valid\" ../actual >../actual.grep &&\n+\tcat >../expect.grep <buf is a\nsymlink, we most likely find that its target's stat data does not\nmatch our cached one, which means we ignore the cache and fall back to\nslow path.\n\nThis is performance issue, not correctness (though we could still\ncatch it by verifying test-dump-untracked-cache. The less unlikely\ncase is, link target stat data matches the cached version and we\nincorrectly go fast path, ignoring real data on disk. A test for this\nmay involve manipulating stat data, which may be not portable.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n dir.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex 3c54366a17..ee1605f004 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1739,7 +1739,7 @@ static int valid_cached_dir(struct dir_struct *dir,\n \t */\n \trefresh_fsmonitor(istate);\n \tif (!(dir->untracked->use_fsmonitor && untracked->valid)) {\n-\t\tif (stat(path->len ? path->buf : \".\", &st)) {\n+\t\tif (lstat(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-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337210","messageId":"20180124093023.9071-4-pclouds@gmail.com","threadId":"47630","inReplyTo":"20180124093023.9071-1-pclouds@gmail.com","subject":"[PATCH 3/5] dir.c: fix missing dir invalidation in untracked code","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:30:21Z","receivedAt":"2018-01-24T09:30:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Let'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 flag becomes 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 \nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n dir.c | 22 ++++++++++++++--------\n t/t7063-status-untracked-cache.sh | 4 ++--\n 2 files changed, 16 insertions(+), 10 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex ee1605f004..44b4dd2ec8 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 (lstat(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 dba7f50bbb..46b947824f 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 SYMLINKS 'setup worktree for symlink test' '\n \tgit commit -m\"second commit\"\n '\n \n-test_expect_failure SYMLINKS '\"status\" after symlink replacement should be clean with UC=true' '\n+test_expect_success SYMLINKS '\"status\" after symlink replacement should be clean with UC=true' '\n \tgit checkout HEAD~ &&\n \tstatus_is_clean &&\n \tstatus_is_clean &&\n@@ -742,7 +742,7 @@ test_expect_success 'setup worktree for non-symlink test' '\n \tgit commit -m\"second commit\"\n '\n \n-test_expect_failure '\"status\" after file replacement should be clean with UC=true' '\n+test_expect_success '\"status\" after file replacement should be clean with UC=true' '\n \tgit checkout HEAD~ &&\n \tstatus_is_clean &&\n \tstatus_is_clean &&\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337211","messageId":"20180124093023.9071-5-pclouds@gmail.com","threadId":"47630","inReplyTo":"20180124093023.9071-1-pclouds@gmail.com","subject":"[PATCH 4/5] update-index doc: note a fixed bug in the untracked cache","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:30:22Z","receivedAt":"2018-01-24T09:30:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Ævar Arnfjörð Bjarmason \n\nDocument 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\nAs noted in that commit, even though this bug gets the untracked cache\ninto a bad state, we have not yet found a case where this is user\nvisible, and thus it makes sense for these docs to focus on the\nsymlink case only.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-update-index.txt | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt\nindex bdb0342593..128e0c671f 100644\n--- a/Documentation/git-update-index.txt\n+++ b/Documentation/git-update-index.txt\n@@ -464,6 +464,22 @@ 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+This bug has also been shown to affect non-symlink cases of replacing\n+a directory with a file when it comes to the internal structures of\n+the untracked cache, but no case has been found where this resulted in\n+wrong \"git status\" output.\n+\n File System Monitor\n -------------------\n \n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337212","messageId":"20180124093023.9071-6-pclouds@gmail.com","threadId":"47630","inReplyTo":"20180124093023.9071-1-pclouds@gmail.com","subject":"[PATCH 5/5] dir.c: stop ignoring opendir() error in open_cached_dir()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:30:23Z","receivedAt":"2018-01-24T09:31:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A follow-up to the recently fixed bugs in the untracked\ninvalidation. If opendir() fails it should show a warning, perhaps\nthis should die, but if this ever happens the error is probably\nrecoverable for the user, and dying would just make things worse.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n dir.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex 44b4dd2ec8..55736d3e2a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1787,11 +1787,16 @@ 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 (!cdir->fdir)\n+\t\twarning_errno(_(\"could not open directory '%s'\"), c_path);\n \tif (dir->untracked) {\n \t\tinvalidate_directory(dir->untracked, untracked);\n \t\tdir->untracked->dir_opened++;\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337250","messageId":"87d11zh40i.fsf@evledraar.gmail.com","threadId":"47630","inReplyTo":"20180124093023.9071-1-pclouds@gmail.com","subject":"Re: [PATCH 0/5] nd/fix-untracked-cache-invalidation updates","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-24T11:47:57Z","receivedAt":"2018-01-24T11:48:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 24 2018, Nguyễn Thái Ngọc Duy jotted:\n\n> This is a small update where the last two patches on 'pu' are merged\n> in one. The final content is exactly the same as on 'pu'.\n>\n> Nguyễn Thái Ngọc Duy (3):\n> dir.c: avoid stat() in valid_cached_dir()\n> dir.c: fix missing dir invalidation in untracked code\n> dir.c: stop ignoring opendir() error in open_cached_dir()\n>\n> Ævar Arnfjörð Bjarmason (2):\n> status: add a failing test showing a core.untrackedCache bug\n> update-index doc: note a fixed bug in the untracked cache\n\nLooks good to me.\n"},{"id":"337299","messageId":"xmqqwp07c7md.fsf@gitster.mtv.corp.google.com","threadId":"47630","inReplyTo":"87d11zh40i.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/5] nd/fix-untracked-cache-invalidation updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-24T20:41:14Z","receivedAt":"2018-01-24T20:41:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Wed, Jan 24 2018, Nguyễn Thái Ngọc Duy jotted:\n>\n>> This is a small update where the last two patches on 'pu' are merged\n>> in one. The final content is exactly the same as on 'pu'.\n>>\n>> Nguyễn Thái Ngọc Duy (3):\n>> dir.c: avoid stat() in valid_cached_dir()\n>> dir.c: fix missing dir invalidation in untracked code\n>> dir.c: stop ignoring opendir() error in open_cached_dir()\n>>\n>> Ævar Arnfjörð Bjarmason (2):\n>> status: add a failing test showing a core.untrackedCache bug\n>> update-index doc: note a fixed bug in the untracked cache\n>\n> Looks good to me.\n\nYup, looks good. Thanks, both.\n"},{"id":"338052","messageId":"87d11omi2o.fsf@evledraar.gmail.com","threadId":"47630","inReplyTo":"20180124093023.9071-6-pclouds@gmail.com","subject":"Re: [PATCH 5/5] dir.c: stop ignoring opendir() error in open_cached_dir()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-02-01T23:00:15Z","receivedAt":"2018-02-01T23:00:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 24 2018, Nguyễn Thái Ngọc Duy jotted:\n\n> A follow-up to the recently fixed bugs in the untracked\n> invalidation. If opendir() fails it should show a warning, perhaps\n> this should die, but if this ever happens the error is probably\n> recoverable for the user, and dying would just make things worse.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> dir.c | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 44b4dd2ec8..55736d3e2a 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1787,11 +1787,16 @@ 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 (!cdir->fdir)\n> +\t\twarning_errno(_(\"could not open directory '%s'\"), c_path);\n> \tif (dir->untracked) {\n> \t\tinvalidate_directory(dir->untracked, untracked);\n> \t\tdir->untracked->dir_opened++;\n\nI haven't found the root cause yet, but we should not release a 2.17\nwith this patch.\n\nI tried deploying a 2.16.1 + various patches (including this) internally\ntoday, and on a test set of 236 machines with existing checkouts\n(already using untracked cache) 79 would spew \"warning: could not open\ndirectory\" warnings, warning about a lot of directories that did not\nexist at the currently checked-out commit.\n\nThe warnings go away on a one-off:\n\n git -c core.untrackedCache=false status\n\nSo there's some issue where an existing index with stale UC will be used\nby a newer version of git, and will start very verbosely warning about\nevery directory referenced that can't be found anymore.\n"}]}