Re: [PATCH] status: add a failing test showing a core.untrackedCache bug

[Junio C Hamano]

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

[Duy Nguyen]

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

[Ævar Arnfjörð Bjarmason]

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

[Junio C Hamano]

Æ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

[Eric Sunshine]

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

[Duy Nguyen]

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 : ".", )) {
-   invalidate_directory(dir->untracked, untracked);
memset(>stat_data, 0, 
sizeof(untracked->stat_data));
return 0;
}
if (!untracked->valid ||
match_stat_data_racy(istate, >stat_data, 
)) {
-

Re: [PATCH] status: add a failing test showing a core.untrackedCache bug

[Duy Nguyen]

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

[Ævar Arnfjörð Bjarmason]

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 : ".", )) {
invalidate_directory(dir->untracked, untracked);
memset(>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(>stat_data, );
+   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(, path->buf, path->len - 1);
+   fprintf(stderr, "Just dir = <%s>\n", dirbuf.buf);
+
+   if (lstat(dirbuf.buf, )) {
+   fprintf(stderr, "Ret #4 = 0\n");
+   return 0;
+   } else if (S_ISLNK(st2.st_mode)) {
+   invalidate_directory(dir->untracked, untracked);
+   memset(>stat_data, 0, 
sizeof(untracked->stat_data));
+   fill_stat_data(>stat_data, );
+   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 

Re: [PATCH] status: add a failing test showing a core.untrackedCache bug

[Duy Nguyen]

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

[Æ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 
---
 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