[PATCH] untracked-cache: fix subdirectory handling

[PATCH] untracked-cache: fix subdirectory handling

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

To fix this, untracked_cache_dir learns to track its depth.  Callers
to lookup_untracked which have a full path now first trim off a
sufficient number of path prefixes.  A new helper function,
lookup_untracked_recursive, helps untracked_cache_invalidate_path to
perform this operation.

Signed-off-by: David Turner <dturner@twopensource.com>
---

This patch applies on top of dt/untracked-sparse, presently in `pu` at
d2cd01bd.  I think only the test part depends on that patch.

Duy, let me know if you think this is the right approach.

---
 dir.c                             | 50 ++++++++++++++++++++-------
 dir.h                             |  1 +
 t/t7063-status-untracked-cache.sh | 72 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 110 insertions(+), 13 deletions(-)

diff --git a/dir.c b/dir.c
index e7b89fe..314080b 100644
--- a/dir.c
+++ b/dir.c
@@ -631,6 +631,7 @@ static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
 	memset(d, 0, sizeof(*d));
 	memcpy(d->name, name, len);
 	d->name[len] = '\0';
+	d->depth = dir->depth + 1;
 
 	ALLOC_GROW(dir->dirs, dir->dirs_nr + 1, dir->dirs_alloc);
 	memmove(dir->dirs + first + 1, dir->dirs + first,
@@ -1324,7 +1325,19 @@ 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);
+	if (untracked) {
+		const char *cur = dirname;
+		int i;
+
+		for (i = 0; i < untracked->depth; i++) {
+			cur = strchr(cur, '/');
+			assert(cur);
+			cur++;
+		}
+		untracked = lookup_untracked(dir->untracked, untracked,
+					     cur,
+					     len - (cur - dirname));
+	}
 	return read_directory_recursive(dir, dirname, len,
 					untracked, 1, simplify);
 }
@@ -2431,7 +2444,7 @@ static void stat_data_from_disk(struct stat_data *to, const struct stat_data *fr
 }
 
 static int read_one_dir(struct untracked_cache_dir **untracked_,
-			struct read_data *rd)
+			struct read_data *rd, int depth)
 {
 	struct untracked_cache_dir ud, *untracked;
 	const unsigned char *next, *data = rd->data, *end = rd->end;
@@ -2444,6 +2457,7 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
 	value = decode_varint(&next);
 	if (next > end)
 		return -1;
+	ud.depth = depth;
 	ud.recurse	   = 1;
 	ud.untracked_alloc = value;
 	ud.untracked_nr	   = value;
@@ -2480,7 +2494,7 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
 	rd->data = data;
 
 	for (i = 0; i < untracked->dirs_nr; i++) {
-		len = read_one_dir(untracked->dirs + i, rd);
+		len = read_one_dir(untracked->dirs + i, rd, depth + 1);
 		if (len < 0)
 			return -1;
 	}
@@ -2577,7 +2591,7 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
 	rd.index      = 0;
 	rd.ucd        = xmalloc(sizeof(*rd.ucd) * len);
 
-	if (read_one_dir(&uc->root, &rd) || rd.index != len)
+	if (read_one_dir(&uc->root, &rd, 0) || rd.index != len)
 		goto done;
 
 	next = rd.data;
@@ -2614,20 +2628,32 @@ done2:
 	return uc;
 }
 
+static struct untracked_cache_dir *lookup_untracked_recursive(
+	struct untracked_cache *uc, struct untracked_cache_dir *dir,
+	const char *path, int len)
+{
+	const char *rest = strchr(path, '/');
+
+	if (rest) {
+		int component_len = rest - path;
+		struct untracked_cache_dir *d =
+			lookup_untracked(uc, dir, path, component_len);
+		return lookup_untracked_recursive(uc, d, rest + 1,
+						  len - (component_len + 1));
+	} else {
+		return dir;
+	}
+}
+
 void untracked_cache_invalidate_path(struct index_state *istate,
 				     const char *path)
 {
-	const char *sep;
 	struct untracked_cache_dir *d;
 	if (!istate->untracked || !istate->untracked->root)
 		return;
-	sep = strrchr(path, '/');
-	if (sep)
-		d = lookup_untracked(istate->untracked,
-				     istate->untracked->root,
-				     path, sep - path);
-	else
-		d = istate->untracked->root;
+	d = lookup_untracked_recursive(istate->untracked,
+				       istate->untracked->root,
+				       path, strlen(path));
 	istate->untracked->dir_invalidated++;
 	d->valid = 0;
 	d->untracked_nr = 0;
diff --git a/dir.h b/dir.h
index 7b5855d..885ed43 100644
--- a/dir.h
+++ b/dir.h
@@ -109,6 +109,7 @@ struct sha1_stat {
  *  excludes_file_sha1[])
  */
 struct untracked_cache_dir {
+	unsigned int depth;
 	struct untracked_cache_dir **dirs;
 	char **untracked;
 	struct stat_data stat_data;
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index ff23f4e..bb0e2a9 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -408,7 +408,8 @@ test_expect_success 'set up sparse checkout' '
 	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" \
+	strace -k -o /tmp/st 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.0.4.315.gad8727a-twtrsrc