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

[Derrick Stolee <stolee@gmail.com>]

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