* [PATCH] status: add a failing test showing a core.untrackedCache bug @ 2017-12-22 14:00 Ævar Arnfjörð Bjarmason 2017-12-25 11:26 ` Duy Nguyen 2017-12-27 18:09 ` Junio C Hamano 0 siblings, 2 replies; 12+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-12-22 14:00 UTC (permalink / raw) To: git Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, Jeff King, Christian Couder, Ben Peart, Ævar Arnfjörð Bjarmason 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 <avarab@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] status: add a failing test showing a core.untrackedCache bug 2017-12-22 14:00 [PATCH] status: add a failing test showing a core.untrackedCache bug Ævar Arnfjörð Bjarmason @ 2017-12-25 11:26 ` Duy Nguyen 2017-12-25 18:45 ` Ævar Arnfjörð Bjarmason 2017-12-27 18:09 ` Junio C Hamano 1 sibling, 1 reply; 12+ messages in thread From: Duy Nguyen @ 2017-12-25 11:26 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Git Mailing List, Junio C Hamano, Jeff King, Christian Couder, Ben Peart On Fri, Dec 22, 2017 at 9:00 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] status: add a failing test showing a core.untrackedCache bug 2017-12-25 11:26 ` Duy Nguyen @ 2017-12-25 18:45 ` Ævar Arnfjörð Bjarmason 2017-12-26 10:47 ` Duy Nguyen 0 siblings, 1 reply; 12+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-12-25 18:45 UTC (permalink / raw) To: Duy Nguyen Cc: Git Mailing List, Junio C Hamano, Jeff King, Christian Couder, Ben Peart On Mon, Dec 25 2017, Duy Nguyen jotted: > On Fri, Dec 22, 2017 at 9:00 PM, Ævar Arnfjörð Bjarmason > <avarab@gmail.com> 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 = untracked; - if (valid_cached_dir(dir, untracked, istate, path, check_only)) + valid = valid_cached_dir(dir, untracked, istate, path, check_only); + fprintf(stderr, "Checked <%s>, valid? <%d>\n", path->buf, valid); + if (valid) return 0; cdir->fdir = opendir(path->len ? path->buf : "."); if (dir->untracked) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] status: add a failing test showing a core.untrackedCache bug 2017-12-25 18:45 ` Ævar Arnfjörð Bjarmason @ 2017-12-26 10:47 ` Duy Nguyen 2017-12-27 10:25 ` Duy Nguyen 0 siblings, 1 reply; 12+ messages in thread From: Duy Nguyen @ 2017-12-26 10:47 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Git Mailing List, Junio C Hamano, Jeff King, Christian Couder, Ben Peart On Tue, Dec 26, 2017 at 1:45 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Mon, Dec 25 2017, Duy Nguyen jotted: > >> On Fri, Dec 22, 2017 at 9:00 PM, Ævar Arnfjörð Bjarmason >> <avarab@gmail.com> 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 ... / 0000000000000000000000000000000000000000 recurse /one/ 0000000000000000000000000000000000000000 recurse valid /two/ 0000000000000000000000000000000000000000 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] status: add a failing test showing a core.untrackedCache bug 2017-12-26 10:47 ` Duy Nguyen @ 2017-12-27 10:25 ` Duy Nguyen 2017-12-27 11:28 ` [PATCH 3/1] update-index doc: note a fixed bug in the untracked cache Ævar Arnfjörð Bjarmason 2017-12-27 17:50 ` [PATCH] status: add a failing test showing a core.untrackedCache bug Eric Sunshine 0 siblings, 2 replies; 12+ messages in thread From: Duy Nguyen @ 2017-12-27 10:25 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Git Mailing List, Junio C Hamano, Jeff King, Christian Couder, Ben Peart 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 <pclouds@gmail.com> --- 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)) { - if (untracked->valid) - invalidate_directory(dir->untracked, untracked); fill_stat_data(&untracked->stat_data, &st); return 0; } } - if (untracked->check_only != !!check_only) { - invalidate_directory(dir->untracked, untracked); + if (untracked->check_only != !!check_only) return 0; - } /* * prep_exclude will be called eventually on this directory, @@ -1788,8 +1792,10 @@ static int open_cached_dir(struct cached_dir *cdir, if (valid_cached_dir(dir, untracked, istate, path, check_only)) return 0; cdir->fdir = opendir(path->len ? path->buf : "."); - if (dir->untracked) + if (dir->untracked) { + invalidate_directory(dir->untracked, untracked); dir->untracked->dir_opened++; + } if (!cdir->fdir) return -1; return 0; diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh index 7cf1e2c091..8f5ef32525 100755 --- a/t/t7063-status-untracked-cache.sh +++ b/t/t7063-status-untracked-cache.sh @@ -707,7 +707,7 @@ test_expect_success 'setup worktree for symlink test' ' git commit -m"second commit" ' -test_expect_failure '"status" after symlink replacement should be clean with UC=true' ' +test_expect_success '"status" after symlink replacement should be clean with UC=true' ' git checkout HEAD~ && status_is_clean && status_is_clean && -- 2.15.0.320.g0453912d77 -- 8< -- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/1] update-index doc: note a fixed bug in the untracked cache 2017-12-27 10:25 ` Duy Nguyen @ 2017-12-27 11:28 ` Ævar Arnfjörð Bjarmason 2017-12-27 18:07 ` Junio C Hamano 2017-12-27 17:50 ` [PATCH] status: add a failing test showing a core.untrackedCache bug Eric Sunshine 1 sibling, 1 reply; 12+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-12-27 11:28 UTC (permalink / raw) To: git Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, Jeff King, Christian Couder, Ben Peart, Ævar Arnfjörð Bjarmason Document the bug tested for in my "status: add a failing test showing a core.untrackedCache bug" and fixed in Duy's "dir.c: fix missing dir invalidation in untracked code". Since this is very likely something others will encounter in the future on older versions, and it's not obvious how to fix it let's document both that it exists, and how to "fix" it with a one-off command. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- > 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. Thanks a lot. I think it's probably good to apply something like this on top of this now 3-patch series. Documentation/git-update-index.txt | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt index bdb0342593..bc6c32002f 100644 --- a/Documentation/git-update-index.txt +++ b/Documentation/git-update-index.txt @@ -464,6 +464,17 @@ command reads the index; while when `--[no-|force-]untracked-cache` are used, the untracked cache is immediately added to or removed from the index. +Before 2.16, the untracked cache had a bug where replacing a directory +with a symlink to another directory could cause it to incorrectly show +files tracked by git as untracked. See the "status: add a failing test +showing a core.untrackedCache bug" commit to git.git. A workaround for +that was (and this might work for other undiscoverd bugs in the +future): + +---------------- +$ git -c core.untrackedCache=false status +---------------- + File System Monitor ------------------- -- 2.15.1.424.g9478a66081 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/1] update-index doc: note a fixed bug in the untracked cache 2017-12-27 11:28 ` [PATCH 3/1] update-index doc: note a fixed bug in the untracked cache Ævar Arnfjörð Bjarmason @ 2017-12-27 18:07 ` Junio C Hamano 0 siblings, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2017-12-27 18:07 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Nguyễn Thái Ngọc Duy, Jeff King, Christian Couder, Ben Peart Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Document the bug tested for in my "status: add a failing test showing > a core.untrackedCache bug" and fixed in Duy's "dir.c: fix missing dir > invalidation in untracked code". > > Since this is very likely something others will encounter in the > future on older versions, and it's not obvious how to fix it let's > document both that it exists, and how to "fix" it with a one-off > command. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > >> 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. > > Thanks a lot. I think it's probably good to apply something like this > on top of this now 3-patch series. Thanks both for working well together. Now that the problem turns out not about symbolic links, can we update the test in 1/1 not to depend on symbolic links? > > Documentation/git-update-index.txt | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt > index bdb0342593..bc6c32002f 100644 > --- a/Documentation/git-update-index.txt > +++ b/Documentation/git-update-index.txt > @@ -464,6 +464,17 @@ command reads the index; while when `--[no-|force-]untracked-cache` > are used, the untracked cache is immediately added to or removed from > the index. > > +Before 2.16, the untracked cache had a bug where replacing a directory > +with a symlink to another directory could cause it to incorrectly show > +files tracked by git as untracked. See the "status: add a failing test > +showing a core.untrackedCache bug" commit to git.git. A workaround for > +that was (and this might work for other undiscoverd bugs in the > +future): > + > +---------------- > +$ git -c core.untrackedCache=false status > +---------------- > + > File System Monitor > ------------------- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] status: add a failing test showing a core.untrackedCache bug 2017-12-27 10:25 ` Duy Nguyen 2017-12-27 11:28 ` [PATCH 3/1] update-index doc: note a fixed bug in the untracked cache Ævar Arnfjörð Bjarmason @ 2017-12-27 17:50 ` Eric Sunshine 1 sibling, 0 replies; 12+ messages in thread From: Eric Sunshine @ 2017-12-27 17:50 UTC (permalink / raw) To: Duy Nguyen Cc: Ævar Arnfjörð Bjarmason, Git Mailing List, Junio C Hamano, Jeff King, Christian Couder, Ben Peart On Wed, Dec 27, 2017 at 5:25 AM, Duy Nguyen <pclouds@gmail.com> 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 <pclouds@gmail.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] status: add a failing test showing a core.untrackedCache bug 2017-12-22 14:00 [PATCH] status: add a failing test showing a core.untrackedCache bug Ævar Arnfjörð Bjarmason 2017-12-25 11:26 ` Duy Nguyen @ 2017-12-27 18:09 ` Junio C Hamano 2017-12-27 18:50 ` Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2017-12-27 18:09 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Nguyễn Thái Ngọc Duy, Jeff King, Christian Couder, Ben Peart Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] status: add a failing test showing a core.untrackedCache bug 2017-12-27 18:09 ` Junio C Hamano @ 2017-12-27 18:50 ` Ævar Arnfjörð Bjarmason 2017-12-28 6:10 ` Duy Nguyen 0 siblings, 1 reply; 12+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-12-27 18:50 UTC (permalink / raw) To: Junio C Hamano Cc: git, Nguyễn Thái Ngọc Duy, Jeff King, Christian Couder, Ben Peart On Wed, Dec 27 2017, Junio C. Hamano jotted: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] status: add a failing test showing a core.untrackedCache bug 2017-12-27 18:50 ` Ævar Arnfjörð Bjarmason @ 2017-12-28 6:10 ` Duy Nguyen 2017-12-28 18:34 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Duy Nguyen @ 2017-12-28 6:10 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Junio C Hamano, git, Jeff King, Christian Couder, Ben Peart 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/ 0000000000000000000000000000000000000000 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< -- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] status: add a failing test showing a core.untrackedCache bug 2017-12-28 6:10 ` Duy Nguyen @ 2017-12-28 18:34 ` Junio C Hamano 0 siblings, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2017-12-28 18:34 UTC (permalink / raw) To: Duy Nguyen Cc: Ævar Arnfjörð Bjarmason, git, Jeff King, Christian Couder, Ben Peart Duy Nguyen <pclouds@gmail.com> 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/ 0000000000000000000000000000000000000000 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-12-28 18:34 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-22 14:00 [PATCH] status: add a failing test showing a core.untrackedCache bug Ævar Arnfjörð Bjarmason 2017-12-25 11:26 ` Duy Nguyen 2017-12-25 18:45 ` Ævar Arnfjörð Bjarmason 2017-12-26 10:47 ` Duy Nguyen 2017-12-27 10:25 ` Duy Nguyen 2017-12-27 11:28 ` [PATCH 3/1] update-index doc: note a fixed bug in the untracked cache Ævar Arnfjörð Bjarmason 2017-12-27 18:07 ` Junio C Hamano 2017-12-27 17:50 ` [PATCH] status: add a failing test showing a core.untrackedCache bug Eric Sunshine 2017-12-27 18:09 ` Junio C Hamano 2017-12-27 18:50 ` Ævar Arnfjörð Bjarmason 2017-12-28 6:10 ` Duy Nguyen 2017-12-28 18:34 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).