Re: [PATCH] status: add a failing test showing a core.untrackedCache bug
Duy Nguyen writes: >> Duy, what am I missing here? > > The problem is there, it's just easier to see or verify with > symlinks. Below is my test patch on top of your original one. Two > points: > > - if you look at the test-dump-untracked-cache output, you can see the > saved cache is wrong. The line > > /one/ recurse valid > > should not be there because that implies that cached travesal of > root includes the directory "one" which does not exist on disk > anymore. With the fix, this line is gone. Nice. > - We silently ignore opendir() error, the changes in dir.c shows this > ... > Report opendir() errors is a good and should be done regardless (i'm > just not sure if it should be a fatal error or a warning like this, I > guess die() is a bit too much). Thanks; I agree that it definitely would be a good change to issue a warning there.
Re: [PATCH] status: add a failing test showing a core.untrackedCache bug
On Wed, Dec 27, 2017 at 07:50:29PM +0100, Ævar Arnfjörð Bjarmason wrote: > > This needs SYMLINKS prereq, which in turn means it cannot be tested > > on certain platforms. I thought Duy's answer was that this does not > > need to involve a symbolic link at all? If so, perhaps we can have > > another test that does not need symlink? > > ... as soon as I figure out how to add such a non-symlink test as well > (seems sensible to test both), but I can't bring it to fail without > symlinks, I just adjusted my test script to do this instead: > > ( > rm -rf /tmp/testrepo && > git init /tmp/testrepo && > cd /tmp/testrepo && > mkdir x y && > touch x/a y/b && > git add x/a y/b && > git commit -msnap && > git rm -rf y && > mkdir y && > touch y/c && > git add y && > git commit -msnap2 && > git checkout HEAD~ && > git status && > git checkout master && > sleep 1 && > git status && > git status > ) > > Duy, what am I missing here? The problem is there, it's just easier to see or verify with symlinks. Below is my test patch on top of your original one. Two points: - if you look at the test-dump-untracked-cache output, you can see the saved cache is wrong. The line /one/ recurse valid should not be there because that implies that cached travesal of root includes the directory "one" which does not exist on disk anymore. With the fix, this line is gone. - We silently ignore opendir() error, the changes in dir.c shows this warning: could not open directory 'one/': Not a directory It opendir() again because it finds out the stat data of directory "one" in the cache does not match stat data of the (real) file "one". If "one" is a symlink, opendir() would be succesful and we go in anyway. If it's a file, we ignore it, accidentally make the second git-status output clean and pass the test. Report opendir() errors is a good and should be done regardless (i'm just not sure if it should be a fatal error or a warning like this, I guess die() is a bit too much). The remaining question is how we write this test. Verify with test-dump-untracked-cache is easiest but less intuitive, I guess. While using symlinks shows the problem clearly but not portable. Or the third option, if we error something out, you could check that git-status has clean stderr. Which way to go? -- 8< -- diff --git a/dir.c b/dir.c index 3c54366a17..868f544d72 100644 --- a/dir.c +++ b/dir.c @@ -1783,15 +1783,20 @@ static int open_cached_dir(struct cached_dir *cdir, struct strbuf *path, int check_only) { + const char *c_path; + memset(cdir, 0, sizeof(*cdir)); cdir->untracked = untracked; if (valid_cached_dir(dir, untracked, istate, path, check_only)) return 0; - cdir->fdir = opendir(path->len ? path->buf : "."); + c_path = path->len ? path->buf : "."; + cdir->fdir = opendir(c_path); if (dir->untracked) dir->untracked->dir_opened++; - if (!cdir->fdir) + if (!cdir->fdir) { + warning_errno(_("could not open directory '%s'"), c_path); return -1; + } return 0; } diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh index 7cf1e2c091..ca63b80ca7 100755 --- a/t/t7063-status-untracked-cache.sh +++ b/t/t7063-status-untracked-cache.sh @@ -702,7 +702,7 @@ test_expect_success 'setup worktree for symlink test' ' git add one/file two/file && git commit -m"first commit" && git rm -rf one && - ln -s two one && + cp two/file one && git add one && git commit -m"second commit" ' @@ -714,6 +714,7 @@ test_expect_failure '"status" after symlink replacement should be clean with UC= git checkout master && avoid_racy && status_is_clean && + test-dump-untracked-cache && status_is_clean ' -- 8< --
Re: [PATCH] status: add a failing test showing a core.untrackedCache bug
On Wed, Dec 27 2017, Junio C. Hamano jotted: > Ævar Arnfjörð Bjarmason writes: > >> +status_is_clean() { >> +>../status.expect && >> +git status --porcelain >../status.actual && >> +test_cmp ../status.expect ../status.actual >> +} >> + >> test_lazy_prereq UNTRACKED_CACHE ' >> { git update-index --test-untracked-cache; ret=$?; } && >> test $ret -ne 1 >> @@ -683,4 +689,43 @@ test_expect_success 'untracked cache survives a commit' >> ' >> test_cmp ../before ../after >> ' >> >> +test_expect_success 'teardown worktree' ' >> +cd .. >> +' > > Funny indentation. Thanks. I'll submit a new series with all 3 patches with the issues you brought up mentioned... >> +test_expect_success 'setup worktree for symlink test' ' >> +git init worktree-symlink && >> +cd worktree-symlink && >> +git config core.untrackedCache true && >> +mkdir one two && >> +touch one/file two/file && >> +git add one/file two/file && >> +git commit -m"first commit" && >> +git rm -rf one && >> +ln -s two one && >> +git add one && >> +git commit -m"second commit" >> +' > > This needs SYMLINKS prereq, which in turn means it cannot be tested > on certain platforms. I thought Duy's answer was that this does not > need to involve a symbolic link at all? If so, perhaps we can have > another test that does not need symlink? ... as soon as I figure out how to add such a non-symlink test as well (seems sensible to test both), but I can't bring it to fail without symlinks, I just adjusted my test script to do this instead: ( rm -rf /tmp/testrepo && git init /tmp/testrepo && cd /tmp/testrepo && mkdir x y && touch x/a y/b && git add x/a y/b && git commit -msnap && git rm -rf y && mkdir y && touch y/c && git add y && git commit -msnap2 && git checkout HEAD~ && git status && git checkout master && sleep 1 && git status && git status ) Duy, what am I missing here?
Re: [PATCH] status: add a failing test showing a core.untrackedCache bug
Ævar Arnfjörð Bjarmason writes: > +status_is_clean() { > + >../status.expect && > + git status --porcelain >../status.actual && > + test_cmp ../status.expect ../status.actual > +} > + > test_lazy_prereq UNTRACKED_CACHE ' > { git update-index --test-untracked-cache; ret=$?; } && > test $ret -ne 1 > @@ -683,4 +689,43 @@ test_expect_success 'untracked cache survives a commit' ' > test_cmp ../before ../after > ' > > +test_expect_success 'teardown worktree' ' > +cd .. > +' Funny indentation. > +test_expect_success 'setup worktree for symlink test' ' > + git init worktree-symlink && > + cd worktree-symlink && > + git config core.untrackedCache true && > + mkdir one two && > + touch one/file two/file && > + git add one/file two/file && > + git commit -m"first commit" && > + git rm -rf one && > + ln -s two one && > + git add one && > + git commit -m"second commit" > +' This needs SYMLINKS prereq, which in turn means it cannot be tested on certain platforms. I thought Duy's answer was that this does not need to involve a symbolic link at all? If so, perhaps we can have another test that does not need symlink? Thanks.
Re: [PATCH] status: add a failing test showing a core.untrackedCache bug
On Wed, Dec 27, 2017 at 5:25 AM, Duy Nguyen wrote: > Subject: [PATCH] dir.c: fix missing dir invalidation in untracked code > [...] > After step 3, we mark the cache valid. Any stale subdir with incorrect > recurse flagbecomes a real subdir next time we traverse the directory s/flagbecomes/flag becomes/ > using dirs[] array. > [...] > Signed-off-by: Nguyễn Thái Ngọc Duy
Re: [PATCH] status: add a failing test showing a core.untrackedCache bug
On Tue, Dec 26, 2017 at 05:47:19PM +0700, Duy Nguyen wrote: > Strangely, root dir is not cached (no valid flag). I don't know why > but that's ok we'll roll with it. I figured this out. Which is good because now I know how/why the bug happens. > An invalidate_directory() call before the "return path_none" above > might be the solution... Nope. This is better. I think checking out any d/f updates would cause this, not just symlinks. -- 8< -- Subject: [PATCH] dir.c: fix missing dir invalidation in untracked code Let's start with how create a new directory cache after the last one becomes invalid (e.g. because its dir mtime has changed...). In open_cached_dir(): 1. We start out with valid_cached_dir() returning false, which should call invalidate_directory() to put a directory state back to initial state, no untracked entries (untracked_nr zero), no sub directory traversal (dirs[].recurse zero). 2. Since the cache cannot be used, we go the slow path opendir() and go through items one by one via readdir(). All the directories on disk will be added back to the cache (if not already exist in dirs[]) and its flag "recurse" gets changed to one to note that it's part of the cached dir travesal next time. 3. By the time we reach close_cached_dir() we should have a good subdir list in dirs[]. Those with "recurse" flag set are the ones present in the on-disk directory. The directory is now marked "valid". Next time read_directory() is called, since the directory is marked valid, it will skip readdir(), go fast path and traverse through dirs[] array instead. Steps one and two need some tight cooperation. If a subdir is removed, readdir() will not find it and of course we cannot examine/invalidate it. To make sure removed directories on disk are gone from the cache, step one must make sure recurse flag of all subdirs are zero. But that's not true. If "valid" flag is already false, there is a chance we go straight to the end of valid_cached_dir() without calling invalidate_directory(). Or we fail to meet the "if (untracked-valid)" condition and skip over the invalidate_directory(). After step 3, we mark the cache valid. Any stale subdir with incorrect recurse flagbecomes a real subdir next time we traverse the directory using dirs[] array. We could avoid this by making sure invalidate_directory() is always called (therefore dirs[].recurse cleared) at the beginning of open_cached_dir(). Which is what this patch does. As to how we get into this situation, the key in the test is this command git checkout master where "one/file" is replaced with "one" in the index. This index update triggers untracked_cache_invalidate_path(), which clears valid flag of the root directory while keeping "recurse" flag on the subdir "one" on. On the next git-status, we go through steps 1-3 above and save an incorrect cache on disk. The second git-status blindly follows the bad cache data and shows the problem. This is arguably because of a bad design where "recurse" flag plays double roles: whether a directory should be saved on disk, and whether it is part of a directory traversal. We need to keep recurse flag set at "checkout master" because of the first role: we need to keep subdir caches (dir "two" for example has not been touched at all, no reason to throw its cache away). As long as we make sure to ignore/reset "recurse" flag at the beginning of a directory traversal, we're good. But maybe eventually we should separate these two roles. Signed-off-by: Nguyễn Thái Ngọc Duy --- dir.c | 22 ++ t/t7063-status-untracked-cache.sh | 2 +- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/dir.c b/dir.c index 3c54366a17..cca88247d3 100644 --- a/dir.c +++ b/dir.c @@ -733,7 +733,16 @@ static void invalidate_directory(struct untracked_cache *uc, struct untracked_cache_dir *dir) { int i; - uc->dir_invalidated++; + + /* +* Invalidation increment here is just roughly correct. If +* untracked_nr or any of dirs[].recurse is non-zero, we +* should increment dir_invalidated too. But that's more +* expensive to do. +*/ + if (dir->valid) + uc->dir_invalidated++; + dir->valid = 0; dir->untracked_nr = 0; for (i = 0; i < dir->dirs_nr; i++) @@ -1740,23 +1749,18 @@ static int valid_cached_dir(struct dir_struct *dir, refresh_fsmonitor(istate); if (!(dir->untracked->use_fsmonitor && untracked->valid)) { if (stat(path->len ? path->buf : ".", &st)) { - invalidate_directory(dir->untracked, untracked); memset(&untracked->stat_data, 0, sizeof(untracked->stat_data)); return 0; } if (!untracked->valid || match_stat_data_racy(istate, &untracked->stat_data, &st)) {
Re: [PATCH] status: add a failing test showing a core.untrackedCache bug
On Tue, Dec 26, 2017 at 1:45 AM, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Dec 25 2017, Duy Nguyen jotted: > >> On Fri, Dec 22, 2017 at 9:00 PM, Ævar Arnfjörð Bjarmason >> wrote: >>> The untracked cache gets confused when a directory is swapped out for >>> a symlink to another directory. Whatever files are inside the target >>> of the symlink will be incorrectly shown as untracked. This issue does >>> not happen if the symlink links to another file, only if it links to >>> another directory. >> >> Sounds about right (I completely forgot about dir symlinks). Since >> I've been away for some time and have not caught up (probably cannot) >> with the mailing list yet, is anyone working on this? It may be >> easiest to just detect symlinksand disable the cache for now. > > I haven't yet, I wanted to see what you had to say about it, > i.e. whether it was a "do'h here's a fix" or if it was more involved > than that. OK I have more time to actually read your test and play with it (last time I stopped at the word "symlink"). First thing out of the way first, I think the stat() call in valid_cached_dir() is wrong. We don't want to automatically follow a symlink, we want stat of the symlink itself. OK back to the test. If you insert test-dump-untracked-cached around the "git checkout master" line, then we get the data that the next/faulty git status uses. For me it shows this ... / recurse /one/ recurse valid /two/ recurse valid OK so we have two uptodate cached dirs for "one" and "two". Strangely, root dir is not cached (no valid flag). I don't know why but that's ok we'll roll with it. It means we will ignore "/" content and do the opendir() and readdir() dance instead. This is where it gets fun, when readdir() returns "one", we hit this code in treat_one_path() and correctly ignores it /* Always exclude indexed files */ if (dtype != DT_DIR && has_path_in_index) return path_none; and we do _nothing_ towards the cached version of "one". In constrast, when readdir() returns "two" we are able to get further and invalidate it, deleting the cached data. After the readdir() dance is complete, dir.c is confident that it has all the good data in the world in its cache and notes down "from now on uses the cached data for /". Another test-dump-... after the second-to-last git-status can show this "valid" flag. Unfortunately the "one" dir is NOT invalidated. So the last git-status sees that cached "/" is good, it skips opendir() and goes with the cached directories which are "two" and the bad "one". In short, we fail to invalidate bad data in some case. An invalidate_directory() call before the "return path_none" above might be the solution... -- Duy
Re: [PATCH] status: add a failing test showing a core.untrackedCache bug
On Mon, Dec 25 2017, Duy Nguyen jotted: > On Fri, Dec 22, 2017 at 9:00 PM, Ævar Arnfjörð Bjarmason > wrote: >> The untracked cache gets confused when a directory is swapped out for >> a symlink to another directory. Whatever files are inside the target >> of the symlink will be incorrectly shown as untracked. This issue does >> not happen if the symlink links to another file, only if it links to >> another directory. > > Sounds about right (I completely forgot about dir symlinks). Since > I've been away for some time and have not caught up (probably cannot) > with the mailing list yet, is anyone working on this? It may be > easiest to just detect symlinksand disable the cache for now. I haven't yet, I wanted to see what you had to say about it, i.e. whether it was a "do'h here's a fix" or if it was more involved than that. Being completely unfamiliar with this code, I hacked up [1] to add some ad-hoc tracing to this. It has some bugs and doesn't actually work, but is injecting something into valid_cached_dir() and checking if the directory in question is a symlink the right approach? Although surely a better solution is to just see that y is a symlink to x, and use the data we get for x. I also see that the the untracked_cache_dir struct has a stat_data field which contains a subset of what we get from stat(), but it doesn't have st_mode, so you can't see from that if the thing was a symlink (but it could be added). Is that the right approach? I.e. saving away whether it was a symlink and if it changes invalidate the cache, although it could be a symlink to something else, so may it needs to be keyed on st_ino (but that may be chagned in-place?). 1. diff --git a/dir.c b/dir.c index 3c54366a17..8afe068c72 100644 --- a/dir.c +++ b/dir.c @@ -1730,10 +1730,13 @@ static int valid_cached_dir(struct dir_struct *dir, int check_only) { struct stat st; + struct stat st2; if (!untracked) return 0; + fprintf(stderr, "Checking <%s>\n", path->buf); + /* * With fsmonitor, we can trust the untracked cache's valid field. */ @@ -1742,6 +1745,7 @@ static int valid_cached_dir(struct dir_struct *dir, if (stat(path->len ? path->buf : ".", &st)) { invalidate_directory(dir->untracked, untracked); memset(&untracked->stat_data, 0, sizeof(untracked->stat_data)); + fprintf(stderr, "Ret #1 = 0\n"); return 0; } if (!untracked->valid || @@ -1749,12 +1753,14 @@ static int valid_cached_dir(struct dir_struct *dir, if (untracked->valid) invalidate_directory(dir->untracked, untracked); fill_stat_data(&untracked->stat_data, &st); + fprintf(stderr, "Ret #2 = 0\n"); return 0; } } if (untracked->check_only != !!check_only) { invalidate_directory(dir->untracked, untracked); + fprintf(stderr, "Ret #3 = 0\n"); return 0; } @@ -1772,6 +1778,28 @@ static int valid_cached_dir(struct dir_struct *dir, } else prep_exclude(dir, istate, path->buf, path->len); + if (path->len && path->buf[path->len - 1] == '/') { + struct strbuf dirbuf = STRBUF_INIT; + strbuf_add(&dirbuf, path->buf, path->len - 1); + fprintf(stderr, "Just dir = <%s>\n", dirbuf.buf); + + if (lstat(dirbuf.buf, &st2)) { + fprintf(stderr, "Ret #4 = 0\n"); + return 0; + } else if (S_ISLNK(st2.st_mode)) { + invalidate_directory(dir->untracked, untracked); + memset(&untracked->stat_data, 0, sizeof(untracked->stat_data)); + fill_stat_data(&untracked->stat_data, &st); + fprintf(stderr, "Is link = <%s>\n", dirbuf.buf); + return 0; + } else { + fprintf(stderr, "Is not link = <%s> but <%d>\n", dirbuf.buf, st2.st_mode); + } + } + + fprintf(stderr, "Falling through for <%s>\n", path->buf); + + /* hopefully prep_exclude() haven't invalidated this entry... */ return untracked->valid; } @@ -1783,9 +1811,12 @@ static int open_cached_dir(struct cached_dir *cdir, struct strbuf *path, int check_only) { + int valid; memset(cdir, 0, sizeof(*cdir)); cdir->untracked =
Re: [PATCH] status: add a failing test showing a core.untrackedCache bug
On Fri, Dec 22, 2017 at 9:00 PM, Ævar Arnfjörð Bjarmason wrote: > The untracked cache gets confused when a directory is swapped out for > a symlink to another directory. Whatever files are inside the target > of the symlink will be incorrectly shown as untracked. This issue does > not happen if the symlink links to another file, only if it links to > another directory. Sounds about right (I completely forgot about dir symlinks). Since I've been away for some time and have not caught up (probably cannot) with the mailing list yet, is anyone working on this? It may be easiest to just detect symlinksand disable the cache for now. -- Duy
[PATCH] status: add a failing test showing a core.untrackedCache bug
The untracked cache gets confused when a directory is swapped out for a symlink to another directory. Whatever files are inside the target of the symlink will be incorrectly shown as untracked. This issue does not happen if the symlink links to another file, only if it links to another directory. A stand-alone testcase for copying into a terminal: ( rm -rf /tmp/testrepo && git init /tmp/testrepo && cd /tmp/testrepo && mkdir x y && touch x/a y/b && git add x/a y/b && git commit -msnap && git rm -rf y && ln -s x y && git add y && git commit -msnap2 && git checkout HEAD~ && git status && git checkout master && sleep 1 && git status && git status ) This will incorrectly show y/a as an untracked file. Both the "git status" call right before "git checkout master" and the "sleep 1" after the "checkout master" are needed to reproduce this, presumably due to the untracked cache tracking on the basis of cached whole seconds from stat(2). When git gets into this state, a workaround to fix it is to issue a one-off: git -c core.untrackedCache=false status Signed-off-by: Ævar Arnfjörð Bjarmason --- t/t7063-status-untracked-cache.sh | 45 +++ 1 file changed, 45 insertions(+) diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh index e5fb892f95..7cf1e2c091 100755 --- a/t/t7063-status-untracked-cache.sh +++ b/t/t7063-status-untracked-cache.sh @@ -22,6 +22,12 @@ avoid_racy() { sleep 1 } +status_is_clean() { + >../status.expect && + git status --porcelain >../status.actual && + test_cmp ../status.expect ../status.actual +} + test_lazy_prereq UNTRACKED_CACHE ' { git update-index --test-untracked-cache; ret=$?; } && test $ret -ne 1 @@ -683,4 +689,43 @@ test_expect_success 'untracked cache survives a commit' ' test_cmp ../before ../after ' +test_expect_success 'teardown worktree' ' +cd .. +' + +test_expect_success 'setup worktree for symlink test' ' + git init worktree-symlink && + cd worktree-symlink && + git config core.untrackedCache true && + mkdir one two && + touch one/file two/file && + git add one/file two/file && + git commit -m"first commit" && + git rm -rf one && + ln -s two one && + git add one && + git commit -m"second commit" +' + +test_expect_failure '"status" after symlink replacement should be clean with UC=true' ' + git checkout HEAD~ && + status_is_clean && + status_is_clean && + git checkout master && + avoid_racy && + status_is_clean && + status_is_clean +' + +test_expect_success '"status" after symlink replacement should be clean with UC=false' ' + git config core.untrackedCache false && + git checkout HEAD~ && + status_is_clean && + status_is_clean && + git checkout master && + avoid_racy && + status_is_clean && + status_is_clean +' + test_done -- 2.15.1.424.g9478a66081