DMARC-Filter: OpenDMARC Filter v1.4.2 smtp.subspace.kernel.org A2F5F606A31C
Message-Id: <pull.2247.v2.git.1794251653636.gitgitgadget@gmail.com>
In-Reply-To: <pull.2247.git.1791127155614.gitgitgadget@gmail.com>
References: <pull.2247.git.1791127155614.gitgitgadget@gmail.com>
From: "Ilia via GitGitGadget" <gitgitgadget@gmail.com>
Date: Mon, 09 Nov 2026 19:14:13 +0000
Subject: [PATCH v2] fsmonitor: check the untracked cache after a trivial
 response
Fcc: Sent
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: list
MIME-Version: 1.0
To: git@vger.kernel.org
Cc: Derrick Stolee <stolee@gmail.com>,
    Ilia <ki.stfu@gmail.com>,
    Ilia K <ki.stfu@gmail.com>

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. Within
the command itself, valid_cached_dir() already checks every directory,
because use_fsmonitor is cleared. Only what is saved needs the check.
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
    
    Changes since v1:
    
     * the whole untracked cache is dropped if !worktree
     * removed the comments as per nits
     * rebased against ToT

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

Range-diff vs v1:

 1:  84618b83fe ! 1:  d2cad8abe6 fsmonitor: check the untracked cache after a trivial response
     @@ Commit message
          ```
          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.
     +    monitor, and invalidate the ones that changed or disappeared. Within
     +    the command itself, valid_cached_dir() already checks every directory,
     +    because use_fsmonitor is cleared. Only what is saved needs the check.
     +    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.
      
     @@ dir.c: void untracked_cache_invalidate_trimmed_path(struct index_state *istate,
      +	 * run in the top-level directory of the worktree.
      +	 */
      +	worktree = repo_get_work_tree(istate->repo);
     -+	if (!worktree)
     -+		return 0;
     ++	if (!worktree) {
     ++		invalidate_directory(istate->untracked,
     ++				     istate->untracked->root);
     ++		return 1;
     ++	}
      +
      +	strbuf_addstr(&path, worktree);
      +	nr_invalidated = invalidate_stale_dirs(istate->untracked,
     @@ t/t7519-status-fsmonitor.sh: 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 &&
     @@ t/t7527-builtin-fsmonitor.sh: test_expect_success CASE_INSENSITIVE_FS 'fsmonitor
       	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" &&
      +


 dir.c                        | 59 ++++++++++++++++++++++++++++++++++++
 dir.h                        |  7 +++++
 read-cache.c                 | 16 ++++++++++
 t/t7519-status-fsmonitor.sh  | 52 +++++++++++++++++++++++++++++++
 t/t7527-builtin-fsmonitor.sh | 40 ++++++++++++++++++++++++
 5 files changed, 174 insertions(+)

diff --git a/dir.c b/dir.c
index ac246ced1d..4f748800b1 100644
--- a/dir.c
+++ b/dir.c
@@ -4066,6 +4066,65 @@ 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) {
+		invalidate_directory(istate->untracked,
+				     istate->untracked->root);
+		return 1;
+	}
+
+	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 1d2b97c429..5d95ba7680 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3084,6 +3084,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..27c3194ae0 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -336,6 +336,58 @@ do
 	done
 done
 
+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..f2770e9056 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -1389,4 +1389,44 @@ 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"
 '
 
+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: fb255012acbcbe81189cb8014da0280b09c66ecc
-- 
gitgitgadget