[PATCH] fsmonitor: check the untracked cache after a trivial response

[PATCH] fsmonitor: check the untracked cache after a trivial response

[Ilia via GitGitGadget]

From: Ilia K <ki.stfu@gmail.com>

When the monitor sends a trivial response ("/"), for example after
the daemon restarted or lost events, refresh_fsmonitor() marks every
index entry dirty, which is saved in the index. For the untracked
cache it only clears the in-memory flag use_fsmonitor. The valid bits
of the cached directories are saved unchanged, although nothing
checks them anymore: valid_cached_dir() does not lstat() a valid
directory while the monitor is trusted.

A command that does not scan for untracked files, like "git add" or
"git checkout", therefore saves the new token next to stale valid
bits, and every later "git status" trusts them. A directory removed
in the meantime keeps its untracked files listed, or, when its own
entry is invalid, makes every run print
```
warning: could not open directory 'sub/': No such file or directory
```
Check every cached directory against its stat data before the cache
is saved next to such a token, as valid_cached_dir() does without a
monitor, and invalidate the ones that changed or disappeared. Doing
this at write time skips commands that do not write the index, and
sparse index writes, which drop the fsmonitor extension anyway.
The cost is one lstat() per cached directory, once per trivial
response.

Signed-off-by: Ilia K <ki.stfu@gmail.com>
---
    fsmonitor: check the untracked cache after a trivial response

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2247%2Fk15tfu%2Funtracked-cache-trivial-response-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2247/k15tfu/untracked-cache-trivial-response-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2247

 dir.c                        | 56 ++++++++++++++++++++++++++++++++++++
 dir.h                        |  7 +++++
 read-cache.c                 | 16 +++++++++++
 t/t7519-status-fsmonitor.sh  | 55 +++++++++++++++++++++++++++++++++++
 t/t7527-builtin-fsmonitor.sh | 44 ++++++++++++++++++++++++++++
 5 files changed, 178 insertions(+)

diff --git a/dir.c b/dir.c
index d896e7be4b..fc41ca9694 100644
--- a/dir.c
+++ b/dir.c
@@ -4053,6 +4053,62 @@ void untracked_cache_invalidate_trimmed_path(struct index_state *istate,
 	}
 }
 
+static int invalidate_stale_dirs(struct untracked_cache *uc,
+				 struct untracked_cache_dir *ucd,
+				 struct index_state *istate,
+				 struct strbuf *path)
+{
+	struct stat st;
+	size_t len = path->len;
+	int nr_invalidated = 0;
+	unsigned int i;
+
+	if (ucd->valid &&
+	    (lstat(path->buf, &st) ||
+	     match_stat_data_racy(istate, &ucd->stat_data, &st))) {
+		invalidate_one_directory(uc, ucd);
+		nr_invalidated++;
+	}
+
+	for (i = 0; i < ucd->dirs_nr; i++) {
+		/* not written to the index, see write_one_dir() */
+		if (!ucd->dirs[i]->recurse)
+			continue;
+		strbuf_addch(path, '/');
+		strbuf_addstr(path, ucd->dirs[i]->name);
+		nr_invalidated += invalidate_stale_dirs(uc, ucd->dirs[i],
+							istate, path);
+		strbuf_setlen(path, len);
+	}
+
+	return nr_invalidated;
+}
+
+int untracked_cache_invalidate_stale_dirs(struct index_state *istate)
+{
+	struct strbuf path = STRBUF_INIT;
+	const char *worktree;
+	int nr_invalidated;
+
+	if (!istate->untracked || !istate->untracked->root)
+		return 0;
+
+	/*
+	 * The index is also read and written by commands that do not
+	 * run in the top-level directory of the worktree.
+	 */
+	worktree = repo_get_work_tree(istate->repo);
+	if (!worktree)
+		return 0;
+
+	strbuf_addstr(&path, worktree);
+	nr_invalidated = invalidate_stale_dirs(istate->untracked,
+					       istate->untracked->root,
+					       istate, &path);
+	strbuf_release(&path);
+	return nr_invalidated;
+}
+
 void untracked_cache_remove_from_index(struct index_state *istate,
 				       const char *path)
 {
diff --git a/dir.h b/dir.h
index 83e0f648a8..7af1562b44 100644
--- a/dir.h
+++ b/dir.h
@@ -604,6 +604,13 @@ void untracked_cache_invalidate_path(struct index_state *, const char *, int saf
 void untracked_cache_invalidate_trimmed_path(struct index_state *,
 					     const char *path,
 					     int safe_path);
+/*
+ * Invalidate every cached directory that no longer exists or whose
+ * stat data no longer matches the working tree. valid_cached_dir()
+ * skips this check while the file system monitor is trusted.
+ * Returns the number of invalidated directories.
+ */
+int untracked_cache_invalidate_stale_dirs(struct index_state *);
 void untracked_cache_remove_from_index(struct index_state *, const char *);
 void untracked_cache_add_to_index(struct index_state *, const char *);
 
diff --git a/read-cache.c b/read-cache.c
index c4cf08a3a3..00c9960c8e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3034,6 +3034,22 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	    istate->untracked) {
 		strbuf_reset(&sb);
 
+		/*
+		 * The monitor could not say what changed (see the trivial
+		 * response in refresh_fsmonitor()), so nothing kept the
+		 * valid bits up to date. Check them before they are saved
+		 * next to the new token, which later commands trust even
+		 * when this command did not look for untracked files.
+		 */
+		if (write_extensions & WRITE_FSMONITOR_EXTENSION &&
+		    istate->fsmonitor_last_update &&
+		    !istate->untracked->use_fsmonitor) {
+			int nr = untracked_cache_invalidate_stale_dirs(istate);
+
+			trace2_data_intmax("index", istate->repo,
+					   "extension/untr/invalidated", nr);
+		}
+
 		write_untracked_extension(&sb, istate->untracked);
 		err = write_index_ext_header(f, eoie_c, CACHE_EXT_UNTRACKED,
 					     sb.len) < 0;
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 93973ed25a..2e1d795a4b 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -336,6 +336,61 @@ do
 	done
 done
 
+# After a trivial response ("/") the monitor cannot vouch for the
+# untracked cache. Even a command that does not look for untracked
+# files must drop the stale entries, or the next "git status" trusts them.
+test_expect_success UNTRACKED_CACHE 'untracked cache is checked after a trivial response' '
+	test_when_finished "rm -rf trivial err" &&
+	git init trivial &&
+	(
+		cd trivial &&
+		mkdir -p dir/sub &&
+		echo tracked >dir/sub/tracked &&
+		git add dir &&
+		git commit -m initial &&
+		git config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-none" &&
+		# Version 1 only, or the hook prints a version complaint
+		# on stderr at every query.
+		git config core.fsmonitorHookVersion 1 &&
+		git config core.untrackedCache true &&
+		# With "normal", invalidating one path also invalidates
+		# its parents, and the stale parent below is never seen.
+		git config status.showUntrackedFiles all &&
+		echo untracked >dir/sub/untracked &&
+		echo "?? dir/sub/untracked" >../expect &&
+		git status --porcelain >../actual &&
+		test_cmp ../expect ../actual &&
+		git status --porcelain >../actual &&
+		test_cmp ../expect ../actual &&
+
+		# The monitor misses the removal of dir/sub. "git add other"
+		# gets the trivial response and does not touch the entries
+		# of dir and dir/sub by itself.
+		rm -r dir/sub &&
+		echo other >other &&
+		git -c core.fsmonitor="$TEST_DIRECTORY/t7519/fsmonitor-all" \
+			add other &&
+		cat >../expect <<-\EOF &&
+		 D dir/sub/tracked
+		A  other
+		EOF
+		git status --porcelain >../actual 2>../err &&
+		test_must_be_empty ../err &&
+		test_cmp ../expect ../actual &&
+
+		# Invalidate the entry of dir/sub. The stale entry of dir then
+		# makes "git status" open the removed directory and warn.
+		git update-index --remove dir/sub/tracked &&
+		cat >../expect <<-\EOF &&
+		D  dir/sub/tracked
+		A  other
+		EOF
+		git status --porcelain >../actual 2>../err &&
+		test_must_be_empty ../err &&
+		test_cmp ../expect ../actual
+	)
+'
+
 # test that splitting the index doesn't interfere
 test_expect_success 'splitting the index results in the same state' '
 	write_integration_script &&
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 86195770e9..46b97ef784 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -1389,4 +1389,48 @@ test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
 	test_grep -q " M dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-try3.out"
 '
 
+# After a restart the daemon sends a trivial response ("/"), because it
+# cannot know what changed while it was down. Even a command that does
+# not look for untracked files must then drop the stale untracked cache
+# entries, or the next "git status" trusts them.
+test_expect_success UNTRACKED_CACHE 'untracked cache is checked after a trivial response' '
+	test_when_finished "stop_daemon_delete_repo test_trivial" &&
+
+	git init test_trivial &&
+	mkdir -p test_trivial/dir/sub &&
+	echo tracked >test_trivial/dir/sub/tracked &&
+	git -C test_trivial add dir &&
+	git -C test_trivial commit -m initial &&
+	git -C test_trivial config core.fsmonitor true &&
+	git -C test_trivial config core.untrackedCache true &&
+	echo untracked >test_trivial/dir/sub/untracked &&
+
+	# The first status starts the daemon and builds the untracked
+	# cache, the second one trusts it.
+	echo "?? dir/sub/untracked" >expect &&
+	git -C test_trivial status --porcelain >actual &&
+	test_cmp expect actual &&
+	git -C test_trivial status --porcelain >actual &&
+	test_cmp expect actual &&
+
+	# Remove dir/sub while no daemon is running. "git add" then
+	# starts a new daemon, receives its trivial response, and does
+	# not look for untracked files.
+	git -C test_trivial fsmonitor--daemon stop &&
+	rm -r test_trivial/dir/sub &&
+	echo other >test_trivial/other &&
+	GIT_TRACE2_EVENT="$PWD/trace_trivial" \
+		git -C test_trivial add other &&
+	have_t2_data_event fsm_client query/trivial-response <trace_trivial &&
+	git -C test_trivial fsmonitor--daemon status &&
+
+	cat >expect <<-\EOF &&
+	 D dir/sub/tracked
+	A  other
+	EOF
+	git -C test_trivial status --porcelain >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp expect actual
+'
+
 test_done

base-commit: c46c1e37724f0478939de636ab8ea5a89086d532
-- 
gitgitgadget

Re: [PATCH] fsmonitor: check the untracked cache after a trivial response

[Derrick Stolee]

On 10/4/2026 11:19 AM, Ilia via GitGitGadget wrote:
> From: Ilia K <ki.stfu@gmail.com>
> 
> When the monitor sends a trivial response ("/"), for example after
> the daemon restarted or lost events, refresh_fsmonitor() marks every
> index entry dirty, which is saved in the index. For the untracked
> cache it only clears the in-memory flag use_fsmonitor. The valid bits
> of the cached directories are saved unchanged, although nothing
> checks them anymore: valid_cached_dir() does not lstat() a valid
> directory while the monitor is trusted.

Sorry that I declared "mailing list bankruptcy" and didn't see your patch
when I was preparing mine [1] for the same issue.

[1] https://lore.kernel.org/git/pull.2295.git.1793806679163.gitgitgadget@gmail.com/

...
> Check every cached directory against its stat data before the cache
> is saved next to such a token, as valid_cached_dir() does without a
> monitor, and invalidate the ones that changed or disappeared. Doing
> this at write time skips commands that do not write the index, and
> sparse index writes, which drop the fsmonitor extension anyway.
> The cost is one lstat() per cached directory, once per trivial
> response.

This is the main difference between your patch and mine. My patch does a
full clear of the untracked cache, but you do the more efficient thing to
only clear the entries that don't match the filesystem info.

This is visible when running p7527-bulitin-fsmonitor.sh on Linux with a
small setting in fs.inotify.max_user_watches, causing the builtin fsmonitor
to always fail. My "clear everything" patch gets a 50-75% penalty on the
untracked cache tests while yours has no measurable impact.

For this reason, I think your approach is better and worth the slight
increase in code complexity.

> +static int invalidate_stale_dirs(struct untracked_cache *uc,
> +				 struct untracked_cache_dir *ucd,
> +				 struct index_state *istate,
> +				 struct strbuf *path)
> +{
> +	struct stat st;
> +	size_t len = path->len;
> +	int nr_invalidated = 0;
> +	unsigned int i;
> +
> +	if (ucd->valid &&
> +	    (lstat(path->buf, &st) ||
> +	     match_stat_data_racy(istate, &ucd->stat_data, &st))) {
> +		invalidate_one_directory(uc, ucd);
> +		nr_invalidated++;
> +	}
> +
> +	for (i = 0; i < ucd->dirs_nr; i++) {
> +		/* not written to the index, see write_one_dir() */
> +		if (!ucd->dirs[i]->recurse)
> +			continue;
> +		strbuf_addch(path, '/');
> +		strbuf_addstr(path, ucd->dirs[i]->name);
> +		nr_invalidated += invalidate_stale_dirs(uc, ucd->dirs[i],
> +							istate, path);

I'm conditioned to worry about recursive calls without depth limits, but
I suppose that we are limited by the untracked cache data structure itself,
which was presumably created recursively without hitting stack limits.

We could make this iterative using a stack-based data structure, but that
would be more complicated than this is worth, probably. These are not used
in server environments and the .git/index file shouldn't be considered
"unsafe" or else we have bigger safety problems.

> +		strbuf_setlen(path, len);
> +	}
> +
> +	return nr_invalidated;
> +}
> +
> +int untracked_cache_invalidate_stale_dirs(struct index_state *istate)
> +{
> +	struct strbuf path = STRBUF_INIT;
> +	const char *worktree;
> +	int nr_invalidated;
> +
> +	if (!istate->untracked || !istate->untracked->root)
> +		return 0;
> +
> +	/*
> +	 * The index is also read and written by commands that do not
> +	 * run in the top-level directory of the worktree.
> +	 */
> +	worktree = repo_get_work_tree(istate->repo);
> +	if (!worktree)
> +		return 0;

I wonder if this is the right response if we fail to get a worktree. Maybe
we should invalidate the entire untracked cache in this case, along with a
warning that we couldn't evaluate the worktree? Or should we return -1 to
signal an error?

> +
> +	strbuf_addstr(&path, worktree);
> +	nr_invalidated = invalidate_stale_dirs(istate->untracked,
> +					       istate->untracked->root,
> +					       istate, &path);
> +	strbuf_release(&path);
> +	return nr_invalidated;
> +}
...
> +/*
> + * Invalidate every cached directory that no longer exists or whose
> + * stat data no longer matches the working tree. valid_cached_dir()
> + * skips this check while the file system monitor is trusted.
> + * Returns the number of invalidated directories.
> + */
> +int untracked_cache_invalidate_stale_dirs(struct index_state *);

We could update this doc comment if we return negative on error.

> diff --git a/read-cache.c b/read-cache.c
> index c4cf08a3a3..00c9960c8e 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -3034,6 +3034,22 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>  	    istate->untracked) {
>  		strbuf_reset(&sb);
>  
> +		/*
> +		 * The monitor could not say what changed (see the trivial
> +		 * response in refresh_fsmonitor()), so nothing kept the
> +		 * valid bits up to date. Check them before they are saved
> +		 * next to the new token, which later commands trust even
> +		 * when this command did not look for untracked files.
> +		 */
> +		if (write_extensions & WRITE_FSMONITOR_EXTENSION &&
> +		    istate->fsmonitor_last_update &&
> +		    !istate->untracked->use_fsmonitor) {
> +			int nr = untracked_cache_invalidate_stale_dirs(istate);
> +
> +			trace2_data_intmax("index", istate->repo,
> +					   "extension/untr/invalidated", nr);
> +		}

This surprised me, that you're computing this in do_write_index(). I suppose
that this avoids doing this computation unless the index is written, but is
it possible that between refresh_fsmonitor() and do_write_index() that the
untracked cache is read and used to determine what this index update should
be?

I used a coding agent to explore my concerns in this space, and it couldn't
find an example where doing this in refresh_fsmonitor() would be better than
its current placement.

> +# After a trivial response ("/") the monitor cannot vouch for the
> +# untracked cache. Even a command that does not look for untracked
> +# files must drop the stale entries, or the next "git status" trusts them.

nit: Generally, we don't need these comments because the tests should be
self-documenting.

> +test_expect_success UNTRACKED_CACHE 'untracked cache is checked after a trivial response' '
> +	test_when_finished "rm -rf trivial err" &&
> +	git init trivial &&
> +	(
> +		cd trivial &&
> +		mkdir -p dir/sub &&
> +		echo tracked >dir/sub/tracked &&
> +		git add dir &&
> +		git commit -m initial &&
> +		git config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-none" &&
> +		# Version 1 only, or the hook prints a version complaint
> +		# on stderr at every query.
> +		git config core.fsmonitorHookVersion 1 &&
> +		git config core.untrackedCache true &&
> +		# With "normal", invalidating one path also invalidates
> +		# its parents, and the stale parent below is never seen.
> +		git config status.showUntrackedFiles all &&
> +		echo untracked >dir/sub/untracked &&
> +		echo "?? dir/sub/untracked" >../expect &&
> +		git status --porcelain >../actual &&
> +		test_cmp ../expect ../actual &&
> +		git status --porcelain >../actual &&
> +		test_cmp ../expect ../actual &&
> +
> +		# The monitor misses the removal of dir/sub. "git add other"
> +		# gets the trivial response and does not touch the entries
> +		# of dir and dir/sub by itself.
> +		rm -r dir/sub &&
> +		echo other >other &&
> +		git -c core.fsmonitor="$TEST_DIRECTORY/t7519/fsmonitor-all" \
> +			add other &&
> +		cat >../expect <<-\EOF &&
> +		 D dir/sub/tracked
> +		A  other

I initially thought that this difference in left-padding was a typo, but
it is functional. We need a space before D and a space after A to signal
the different states.

> +		EOF
> +		git status --porcelain >../actual 2>../err &&
> +		test_must_be_empty ../err &&
> +		test_cmp ../expect ../actual &&
> +
> +		# Invalidate the entry of dir/sub. The stale entry of dir then
> +		# makes "git status" open the removed directory and warn.
> +		git update-index --remove dir/sub/tracked &&
> +		cat >../expect <<-\EOF &&
> +		D  dir/sub/tracked
> +		A  other

And of course now the deletion is staged. Good.

> +		EOF
> +		git status --porcelain >../actual 2>../err &&
> +		test_must_be_empty ../err &&
> +		test_cmp ../expect ../actual
> +	)
> +'

> +# After a restart the daemon sends a trivial response ("/"), because it
> +# cannot know what changed while it was down. Even a command that does
> +# not look for untracked files must then drop the stale untracked cache
> +# entries, or the next "git status" trusts them.

same nit: comments shouldn't be necessary here.

> +test_expect_success UNTRACKED_CACHE 'untracked cache is checked after a trivial response' '
> +	test_when_finished "stop_daemon_delete_repo test_trivial" &&
> +
> +	git init test_trivial &&
> +	mkdir -p test_trivial/dir/sub &&
> +	echo tracked >test_trivial/dir/sub/tracked &&
> +	git -C test_trivial add dir &&
> +	git -C test_trivial commit -m initial &&
> +	git -C test_trivial config core.fsmonitor true &&
> +	git -C test_trivial config core.untrackedCache true &&
> +	echo untracked >test_trivial/dir/sub/untracked &&
> +
> +	# The first status starts the daemon and builds the untracked
> +	# cache, the second one trusts it.
> +	echo "?? dir/sub/untracked" >expect &&
> +	git -C test_trivial status --porcelain >actual &&
> +	test_cmp expect actual &&
> +	git -C test_trivial status --porcelain >actual &&
> +	test_cmp expect actual &&
> +
> +	# Remove dir/sub while no daemon is running. "git add" then
> +	# starts a new daemon, receives its trivial response, and does
> +	# not look for untracked files.
> +	git -C test_trivial fsmonitor--daemon stop &&
> +	rm -r test_trivial/dir/sub &&
> +	echo other >test_trivial/other &&
> +	GIT_TRACE2_EVENT="$PWD/trace_trivial" \
> +		git -C test_trivial add other &&
> +	have_t2_data_event fsm_client query/trivial-response <trace_trivial &&
> +	git -C test_trivial fsmonitor--daemon status &&
> +
> +	cat >expect <<-\EOF &&
> +	 D dir/sub/tracked
> +	A  other
> +	EOF
> +	git -C test_trivial status --porcelain >actual 2>err &&
> +	test_must_be_empty err &&
> +	test_cmp expect actual
> +'

Overall, this patch appears to solve the same problem that pointed me in
this direction. I don't see any issues worth re-rolling this patch, so it
LGTM.

Thanks,
-Stolee

Re: [PATCH] fsmonitor: check the untracked cache after a trivial response

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

>> +	for (i = 0; i < ucd->dirs_nr; i++) {
>> +		/* not written to the index, see write_one_dir() */
>> +		if (!ucd->dirs[i]->recurse)
>> +			continue;
>> +		strbuf_addch(path, '/');
>> +		strbuf_addstr(path, ucd->dirs[i]->name);
>> +		nr_invalidated += invalidate_stale_dirs(uc, ucd->dirs[i],
>> +							istate, path);
>
> I'm conditioned to worry about recursive calls without depth limits, but
> I suppose that we are limited by the untracked cache data structure itself,
> which was presumably created recursively without hitting stack limits.
>
> We could make this iterative using a stack-based data structure, but that
> would be more complicated than this is worth, probably. These are not used
> in server environments and the .git/index file shouldn't be considered
> "unsafe" or else we have bigger safety problems.

Good reasoning.  Thanks for spelling it out.

>> +	/*
>> +	 * The index is also read and written by commands that do not
>> +	 * run in the top-level directory of the worktree.
>> +	 */
>> +	worktree = repo_get_work_tree(istate->repo);
>> +	if (!worktree)
>> +		return 0;
>
> I wonder if this is the right response if we fail to get a worktree. Maybe
> we should invalidate the entire untracked cache in this case, along with a
> warning that we couldn't evaluate the worktree? Or should we return -1 to
> signal an error?

It probably is safer to invalidate and continue.  If there is truly
not a worktree, untracked cache entries do not matter ;-)

Thanks for a review.