Re: Bug in index-helper/watchman?

[Duy Nguyen <pclouds@gmail.com>]

On Sat, Jul 16, 2016 at 08:10:15AM +0200, Duy Nguyen wrote:
> But you did spot a problem. What if UC extension is loaded _after_
> watchman one? Then index->untracked_cache would have nothing in there
> and invalidation is no-op when we do it (at watchmain ext loading
> time). We can't control extension loading order (technically we can,
> but other git implementations may do it differently). So, maybe we
> should do the invalidation after _all_ index extensions have been
> loaded.
> 
> Maybe we can do it in validate_untracked_cache? We already store the
> path list in the_index.untracked->invalid_untracked.
> validate_untracked_cache has all info it needs.

The squash for "index-helper: use watchman to avoid refreshing index
with lstat()" looks something like this. I did not test it at all though.

-- 8< --
diff --git a/dir.c b/dir.c
index 024b13c..04bd87c 100644
--- a/dir.c
+++ b/dir.c
@@ -1902,6 +1902,43 @@ void remove_untracked_cache(struct index_state *istate)
 	}
 }
 
+static struct untracked_cache_dir *find_untracked_cache_dir(
+	struct untracked_cache *uc, struct untracked_cache_dir *ucd,
+	const char *name)
+{
+	int component_len;
+	const char *end;
+	struct untracked_cache_dir *dir;
+
+	if (!*name)
+		return ucd;
+
+	end = strchr(name, '/');
+	if (end)
+		component_len = end - name;
+	else
+		component_len = strlen(name);
+
+	dir = lookup_untracked(uc, ucd, name, component_len);
+	if (dir)
+		return find_untracked_cache_dir(uc, dir, name + component_len + 1);
+
+	return NULL;
+}
+
+static int mark_untracked_invalid(struct string_list_item *item, void *uc)
+{
+	struct untracked_cache *untracked = uc;
+	struct untracked_cache_dir *dir;
+
+	dir = find_untracked_cache_dir(untracked, untracked->root,
+				       item->string);
+	if (dir)
+		dir->valid = 0;
+
+	return 0;
+}
+
 static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,
 						      int base_len,
 						      const struct pathspec *pathspec)
@@ -1984,6 +2021,11 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
 
 	/* Make sure this directory is not dropped out at saving phase */
 	root->recurse = 1;
+
+	if (dir->untracked->use_watchman) {
+		for_each_string_list(&dir->untracked->invalid_untracked,
+			 mark_untracked_invalid, dir->untracked);
+	}
 	return root;
 }
 
diff --git a/dir.h b/dir.h
index ed16746..896b64a 100644
--- a/dir.h
+++ b/dir.h
@@ -312,7 +312,4 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
 void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);
 void add_untracked_cache(struct index_state *istate);
 void remove_untracked_cache(struct index_state *istate);
-struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
-					     struct untracked_cache_dir *dir,
-					     const char *name, int len);
 #endif
diff --git a/read-cache.c b/read-cache.c
index ee777c1..b90187c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1388,37 +1388,12 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)
 	return 0;
 }
 
-static struct untracked_cache_dir *find_untracked_cache_dir(
-	struct untracked_cache *uc, struct untracked_cache_dir *ucd,
-	const char *name)
-{
-	int component_len;
-	const char *end;
-	struct untracked_cache_dir *dir;
-
-	if (!*name)
-		return ucd;
-
-	end = strchr(name, '/');
-	if (end)
-		component_len = end - name;
-	else
-		component_len = strlen(name);
-
-	dir = lookup_untracked(uc, ucd, name, component_len);
-	if (dir)
-		return find_untracked_cache_dir(uc, dir, name + component_len + 1);
-
-	return NULL;
-}
-
 static void mark_no_watchman(size_t pos, void *data)
 {
 	struct index_state *istate = data;
 	struct cache_entry *ce = istate->cache[pos];
 	struct strbuf sb = STRBUF_INIT;
 	char *c;
-	struct untracked_cache_dir *dir;
 
 	assert(pos < istate->cache_nr);
 	ce->ce_flags |= CE_WATCHMAN_DIRTY;
@@ -1438,27 +1413,11 @@ static void mark_no_watchman(size_t pos, void *data)
 	if (c == sb.buf)
 		strbuf_setlen(&sb, 0);
 
-	dir = find_untracked_cache_dir(istate->untracked,
-				       istate->untracked->root, sb.buf);
-	if (dir)
-		dir->valid = 0;
+	string_list_append(&istate->untracked->invalid_untracked, ce->name);
 
 	strbuf_release(&sb);
 }
 
-static int mark_untracked_invalid(struct string_list_item *item, void *uc)
-{
-	struct untracked_cache *untracked = uc;
-	struct untracked_cache_dir *dir;
-
-	dir = find_untracked_cache_dir(untracked, untracked->root,
-				       item->string);
-	if (dir)
-		dir->valid = 0;
-
-	return 0;
-}
-
 static int read_watchman_ext(struct index_state *istate, const void *data,
 			     unsigned long sz)
 {
@@ -1498,9 +1457,6 @@ static int read_watchman_ext(struct index_state *istate, const void *data,
 			untracked += len + 1;
 		}
 
-		for_each_string_list(&istate->untracked->invalid_untracked,
-			 mark_untracked_invalid, istate->untracked);
-
 		if (untracked_nr)
 			istate->cache_changed |= WATCHMAN_CHANGED;
 	}
-- 8< --
--
Duy