untracked-cache: fix subdirectory handling

Previously, some calls lookup_untracked would pass a full path. But lookup_untracked assumes that the portion of the path up to and including to the untracked_cache_dir has been removed. So lookup_untracked would be looking in the untracked_cache for 'foo' for 'foo/bar' (instead of just looking for 'bar'). This would cause untracked cache corruption.

Instead, treat_directory learns to track the base length of the parent directory, so that only the last path component is passed to lookup_untracked.

Helped-by Nguyễn Thái Ngọc Duy <[email protected]>

Signed-off-by David Turner <[email protected]>

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---

This version incorporates Duy's version of the dir.c code, and his suggested test speedup.

---
 dir.c                             | 14 ++++----
 t/t7063-status-untracked-cache.sh | 74 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 80 insertions(+), 8 deletions(-)

dir.c +8 -6

index e7b89fe..cd4ac77 100644
@@ -1297,7 +1297,7 @@ static enum exist_status directory_exists_in_index(const char *dirname, int len)
  */
 static enum path_treatment treat_directory(struct dir_struct *dir,
        struct untracked_cache_dir *untracked,
-       const char *dirname, int len, int exclude,
+       const char *dirname, int len, int baselen, int exclude,
        const struct path_simplify *simplify)
 {
        /* The "len-1" is to strip the final '/' */

@@ -1324,7 +1324,8 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
        if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
                return exclude ? path_excluded : path_untracked;
 
-       untracked = lookup_untracked(dir->untracked, untracked, dirname, len);
+       untracked = lookup_untracked(dir->untracked, untracked,
+                                    dirname + baselen, len - baselen);
        return read_directory_recursive(dir, dirname, len,
                                        untracked, 1, simplify);
 }

@@ -1444,6 +1445,7 @@ static int get_dtype(struct dirent *de, const char *path, int len)
 static enum path_treatment treat_one_path(struct dir_struct *dir,
                                          struct untracked_cache_dir *untracked,
                                          struct strbuf *path,
+                                         int baselen,
                                          const struct path_simplify *simplify,
                                          int dtype, struct dirent *de)
 {

@@ -1495,8 +1497,8 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
                return path_none;
        case DT_DIR:
                strbuf_addch(path, '/');
-               return treat_directory(dir, untracked, path->buf, path->len, exclude,
-                       simplify);
+               return treat_directory(dir, untracked, path->buf, path->len,
+                                      baselen, exclude, simplify);
        case DT_REG:
        case DT_LNK:
                return exclude ? path_excluded : path_untracked;

@@ -1557,7 +1559,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
                return path_none;
 
        dtype = DTYPE(de);
-       return treat_one_path(dir, untracked, path, simplify, dtype, de);
+       return treat_one_path(dir, untracked, path, baselen, simplify, dtype, de);
 }
 
 static void add_untracked(struct untracked_cache_dir *dir, const char *name)

@@ -1827,7 +1829,7 @@ static int treat_leading_path(struct dir_struct *dir,
                        break;
                if (simplify_away(sb.buf, sb.len, simplify))
                        break;
-               if (treat_one_path(dir, NULL, &sb, simplify,
+               if (treat_one_path(dir, NULL, &sb, baselen, simplify,
                                   DT_DIR, NULL) == path_none)
                        break; /* do not recurse into it */
                if (len <= baselen) {

t/t7063-status-untracked-cache.sh +72 -3

index ff23f4e..6716f69 100755
@@ -402,13 +402,14 @@ test_expect_success 'set up sparse checkout' '
        echo "done/[a-z]*" >.git/info/sparse-checkout &&
        test_config core.sparsecheckout true &&
        git checkout master &&
-       git update-index --untracked-cache &&
+       git update-index --untracked-cache --force-untracked-cache &&
        git status --porcelain >/dev/null && # prime the cache
        test_path_is_missing done/.gitignore &&
        test_path_is_file done/one
 '
 
-test_expect_success 'create files, some of which are gitignored' '
+test_expect_success 'create/modify files, some of which are gitignored' '
+       echo two bis >done/two &&
        echo three >done/three && # three is gitignored
        echo four >done/four && # four is gitignored at a higher level
        echo five >done/five # five is not gitignored

@@ -420,6 +421,7 @@ test_expect_success 'test sparse status with untracked cache' '
        GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace" \
        git status --porcelain >../status.actual &&
        cat >../status.expect <<EOF &&
+ M done/two
 ?? .gitignore
 ?? done/five
 ?? dtwo/

@@ -459,6 +461,7 @@ test_expect_success 'test sparse status again with untracked cache' '
        GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace" \
        git status --porcelain >../status.actual &&
        cat >../status.expect <<EOF &&
+ M done/two
 ?? .gitignore
 ?? done/five
 ?? dtwo/

@@ -473,4 +476,71 @@ EOF
        test_cmp ../trace.expect ../trace
 '
 
+test_expect_success 'set up for test of subdir and sparse checkouts' '
+       mkdir done/sub &&
+       mkdir done/sub/sub &&
+       echo "sub" > done/sub/sub/file
+'
+
+test_expect_success 'test sparse status with untracked cache and subdir' '
+       avoid_racy &&
+       : >../trace &&
+       GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace" \
+       git status --porcelain >../status.actual &&
+       cat >../status.expect <<EOF &&
+ M done/two
+?? .gitignore
+?? done/five
+?? done/sub/
+?? dtwo/
+EOF
+       test_cmp ../status.expect ../status.actual &&
+       cat >../trace.expect <<EOF &&
+node creation: 2
+gitignore invalidation: 0
+directory invalidation: 1
+opendir: 3
+EOF
+       test_cmp ../trace.expect ../trace
+'
+
+test_expect_success 'verify untracked cache dump (sparse/subdirs)' '
+       test-dump-untracked-cache >../actual &&
+       cat >../expect <<EOF &&
+info/exclude 13263c0978fb9fad16b2d580fb800b6d811c3ff0
+core.excludesfile 0000000000000000000000000000000000000000
+exclude_per_dir .gitignore
+flags 00000006
+/ e6fcc8f2ee31bae321d66afd183fcb7237afae6e recurse valid
+.gitignore
+dtwo/
+/done/ 1946f0437f90c5005533cbe1736a6451ca301714 recurse valid
+five
+sub/
+/done/sub/ 0000000000000000000000000000000000000000 recurse check_only valid
+sub/
+/done/sub/sub/ 0000000000000000000000000000000000000000 recurse check_only valid
+file
+/dthree/ 0000000000000000000000000000000000000000 recurse check_only valid
+/dtwo/ 0000000000000000000000000000000000000000 recurse check_only valid
+two
+EOF
+       test_cmp ../expect ../actual
+'
+
+test_expect_success 'test sparse status again with untracked cache and subdir' '
+       avoid_racy &&
+       : >../trace &&
+       GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace" \
+       git status --porcelain >../status.actual &&
+       test_cmp ../status.expect ../status.actual &&
+       cat >../trace.expect <<EOF &&
+node creation: 0
+gitignore invalidation: 0
+directory invalidation: 0
+opendir: 0
+EOF
+       test_cmp ../trace.expect ../trace
+'
+
 test_done
--

2.4.2.622.gac67c30-twtrsrc

Quoted 7 lines · David Turner

Previously, some calls lookup_untracked would pass a full path. But lookup_untracked assumes that the portion of the path up to and including to the untracked_cache_dir has been removed. So lookup_untracked would be looking in the untracked_cache for 'foo' for 'foo/bar' (instead of just looking for 'bar'). This would cause untracked cache corruption.

Quoted 3 lines

Instead, treat_directory learns to track the base length of the parent directory, so that only the last path component is passed to lookup_untracked.

Your v2 also fixes untracked_cache_invalidate_path(), which is not included here. Maybe it's in another patch?

First, the current code in untracked_cache_invalidate_path() is wrong because it can only handle paths "a" or "a/b", not "a/b/c" because lookup_untracked() only looks for entries directly under the given directory. In the last case, it will look for the entry "b/c" in directory "a" instead. This means if you delete or add an entry in a subdirectory, untracked cache may become out of date because it does not invalidate properly. This is noticed by David Turner.

The second problem is about invalidation inside a fully untracked/excluded directory. In this case we may have to invalidate back to root. See the comment block for detail.

Quoted 8 lines · Nguyễn Thái Ngọc Duy

First, the current code in untracked_cache_invalidate_path() is wrong because it can only handle paths "a" or "a/b", not "a/b/c" because lookup_untracked() only looks for entries directly under the given directory. In the last case, it will look for the entry "b/c" in directory "a" instead. This means if you delete or add an entry in a subdirectory, untracked cache may become out of date because it does not invalidate properly. This is noticed by David Turner.

Quoted 3 lines

The second problem is about invalidation inside a fully untracked/excluded directory. In this case we may have to invalidate back to root. See the comment block for detail.

LGTM.

The first patch is a split from David's v3 patch because it should belong to dt/untracked-sparse. The second is basically David's v3. The third patch fixes untracked_cache_invalidate_path(). David fixed it in v2, but there's another bug in this code.

David Turner (1):
untracked-cache: fix subdirectory handling

Nguyễn Thái Ngọc Duy (2):
t7063: use --force-untracked-cache to speed up a bit untracked cache: fix entry invalidation

 dir.c                             |  82 +++++++++++++++++++++++-------
 t/t7063-status-untracked-cache.sh | 102 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 163 insertions(+), 21 deletions(-)