Re: Bug in index-helper/watchman?

Re: Bug in index-helper/watchman?

[Duy Nguyen]

...and somehow I forgot to CC git@vger, grrrr....

On Thu, Jul 14, 2016 at 5:14 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> Bug reports should always be in public (so people are aware of it)
> unless it's security exploit of course.
>
> I have not read this mail yet, but I will in a couple of hours, hopefully.
>
> On Thu, Jul 14, 2016 at 12:11 AM, Ben Peart <peartben@gmail.com> wrote:
>> I’ve been chasing down an issue where it looks like the untracked cache
>> logic doesn’t work correctly in the index-helper/watchman patch series.
>> It’s also entirely possible that I’m just missing something so feel free to
>> correct my misconceptions.
>>
>>
>>
>> Ultimately, it appears that none of the untracked cache directories are
>> getting flagged as invalid from the data in the watchman extension.  I
>> believe this is happening because untracked->root doesn’t get initialized
>> until validate_untracked_cache is called from read_directory.  This causes
>> all calls to lookup_untracked to return NULL so the dir->valid flag is never
>> set to zero in mark_untracked_invalid.  See the call stacks and sequence
>> below for details:
>>
>>
>>
>>
>>
>> cmd_status at builtin/commit.c:1362
>>
>>               status_init_config (s=0x6667a0 <s>, fn=0x432790
>> <git_status_config>) at builtin/commit.c:187
>>
>>                              gitmodules_config () at submodule.c:196
>>
>>                                            read_index (istate=0x693860
>> <the_index>) at read-cache.c:1442
>>
>>                                                           read_index_from
>> (istate=0x693860 <the_index>, path=0x2ea3c58 ".git/index") at
>> read-cache.c:1849
>>
>>
>> do_read_index
>>
>>
>> read_index_extension
>>
>>
>> read_watchman_ext
>>
>>
>> mark_untracked_invalid
>>
>>
>> find_untracked_cache_dir
>>
>>
>> lookup_untracked
>>
>>
>> if (!dir)
>>
>>
>> return NULL;
>>
>>
>>
>>               wt_status_collect (s=0x6667a0 <s>) at wt-status.c:627
>>
>>                              wt_status_collect_untracked (s=0x6667a0 <s>) at
>> wt-status.c:593
>>
>>                                            fill_directory (dir=0xbafab0,
>> pathspec=0x6667b8 <s+24>) at dir.c:191
>>
>>                                                           read_directory
>> (dir=0xbafab0, path=0x61cac3 <atat+155> "", len=0, pathspec=0x6667b8 <s+24>)
>> at dir.c:2009
>>
>>
>> validate_untracked_cache at dir.c:1903
>>
>>
>> if (!dir->untracked->root)
>>
>>
>> read_directory_recursive
>>
>>
>> open_cached_dir
>>
>>
>> valid_cached_dir
>>
>>
>> read_directory_recursive
>>
>>
>> open_cached_dir
>>
>>
>> valid_cached_dir
>>
>>
>> if (dir->untracked->use_watchman)
>>
>>
>>
>>
>>
>> If I’m reading this correctly, one potential fix is to move the logic that
>> loops through the directories calling mark_untracked_invalid to between the
>> call to validate_untracked_cache and the call to read_directory_recursive.
>> I wonder if there is another simpler/better fix.
>>
>>
>>
>> Thoughts?
>>
>>
>>
>> Ben
>
>
>
> --
> Duy



-- 
Duy

Re: Bug in index-helper/watchman?

[Duy Nguyen]

On Thu, Jul 14, 2016 at 5:21 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Thu, Jul 14, 2016 at 12:11 AM, Ben Peart <peartben@gmail.com> wrote:
>>> I’ve been chasing down an issue where it looks like the untracked cache
>>> logic doesn’t work correctly in the index-helper/watchman patch series.
>>> It’s also entirely possible that I’m just missing something so feel free to
>>> correct my misconceptions.
>>>
>>>
>>>
>>> Ultimately, it appears that none of the untracked cache directories are
>>> getting flagged as invalid from the data in the watchman extension.  I
>>> believe this is happening because untracked->root doesn’t get initialized
>>> until validate_untracked_cache is called from read_directory.  This causes
>>> all calls to lookup_untracked to return NULL so the dir->valid flag is never
>>> set to zero in mark_untracked_invalid.

No, validate_untracked_cache does not do anything fancy in there to
make UC initialized. It may invalidate UC further though.

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

Re: Bug in index-helper/watchman?

[Duy Nguyen]

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