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

[Duy Nguyen <pclouds@gmail.com>]

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