[PATCH] fsmonitor: invalidate untracked cache after losing history

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

From: Derrick Stolee <stolee@gmail.com>

When fsmonitor fails or returns a trivial response, refresh_fsmonitor()
clears CE_FSMONITOR_VALID and disables fsmonitor use for the untracked
cache. Unfortunately, this may leave untracked cache entries marked
'valid' so future commands may miss necessary updates.

A command such as 'git update-index --refresh' can save the replacement
fsmonitor token without scanning untracked directories. If the next
query returns no changes, fsmonitor use is enabled again and 'git status'
trusts directory contents cached before the loss of event history. New
untracked files can remain invisible.

This integration bug between fsmonitor and the untracked cache has
existed since the fsmonitor feature was introduced in 883e248b8a0
(fsmonitor: teach git to optionally utilize a file system monitor to
speed up detecting new or changed files., 2017-09-22).

To fix the problem, react more completely on a failed fsmonitor token
request. In this case, invalidate every untracked cache directory and
mark the untracked extension as changed. To guarantee that the index is
updatede, set UNTRACKED_CHANGED so that the index is written even when
no tracked entry changes. Keep the directory nodes intact to preserve
pointers held by callers.

A new pair of tests validate this approach. They use the fsmonitor hook
implementation because it's easier to trigger these failure conditions,
but the fix applies to both the hook and builtin versions. The tests
confirm that the untracked cache is invalidated.

Performance tests in p7527-builtin-fsmonitor.sh demonstrate that this
change does not introduce any performance degradation, with one caveat.
My initial testing on Linux did not have its max_user_watches high
enough, so the first test run _did_ show a regression for the cases
where the untracked cache was enabled. This added 50-75% additional time
to commands, but this is because it correctly did the extra work to
validate the untracked cache when the fsmonitor failed to start.

Recall that to extend the maximum user watches, one can run

  sudo sysctl -w fs.inotify.max_user_watches=1048576

With this increased watch count, my peformance numbers equalized again,
since the fsmonitor could start successfully and did not invalidate the
untracked cache, as it would with normal operation.

Assisted-by: GPT-6.1 Sol
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
    fsmonitor: invalidate untracked cache after losing history
    
    A user of an internal monorepo reported this behavior, including an
    AI-assisted powershell script to prove how it worked. I used an agent to
    assist in porting that to a Git test script and a code fix, but then
    applied significant human editing and validation on top to get it up to
    my standards.
    
    Thanks, -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2295%2Fderrickstolee%2Ffsmonitor-untracked-v2.56-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2295/derrickstolee/fsmonitor-untracked-v2.56-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2295

 dir.c                       | 18 ++++++++++++++++++
 dir.h                       |  1 +
 fsmonitor.c                 |  4 +++-
 t/t7519-status-fsmonitor.sh | 38 +++++++++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index d896e7be4b..99a2bd9fe2 100644
--- a/dir.c
+++ b/dir.c
@@ -3976,6 +3976,24 @@ static void invalidate_one_directory(struct untracked_cache *uc,
 	ucd->untracked_nr = 0;
 }
 
+static void invalidate_directory_recursive(struct untracked_cache *uc,
+					  struct untracked_cache_dir *ucd)
+{
+	invalidate_one_directory(uc, ucd);
+	for (size_t i = 0; i < ucd->dirs_nr; i++)
+		invalidate_directory_recursive(uc, ucd->dirs[i]);
+}
+
+void untracked_cache_invalidate_all(struct index_state *istate)
+{
+	if (!istate->untracked || !istate->untracked->root)
+		return;
+
+	invalidate_directory_recursive(istate->untracked,
+				       istate->untracked->root);
+	istate->cache_changed |= UNTRACKED_CHANGED;
+}
+
 /*
  * Normally when an entry is added or removed from a directory,
  * invalidating that directory is enough. No need to touch its
diff --git a/dir.h b/dir.h
index 83e0f648a8..d2de762da8 100644
--- a/dir.h
+++ b/dir.h
@@ -596,6 +596,7 @@ static inline int dir_path_match(struct index_state *istate,
 int cmp_dir_entry(const void *p1, const void *p2);
 int check_dir_entry_contains(const struct dir_entry *out, const struct dir_entry *in);
 
+void untracked_cache_invalidate_all(struct index_state *);
 void untracked_cache_invalidate_path(struct index_state *, const char *, int safe_path);
 /*
  * Invalidate the untracked-cache for this path, but first strip
diff --git a/fsmonitor.c b/fsmonitor.c
index 107767527e..9c3079f64a 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -713,8 +713,10 @@ apply_results:
 		if (is_cache_changed)
 			istate->cache_changed |= FSMONITOR_CHANGED;
 
-		if (istate->untracked)
+		if (istate->untracked) {
+			untracked_cache_invalidate_all(istate);
 			istate->untracked->use_fsmonitor = 0;
+		}
 	}
 	trace2_region_leave("fsmonitor", "apply_results", istate->repo);
 
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 93973ed25a..7d3ff4ec6b 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -394,6 +394,44 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
 	test_cmp before after
 '
 
+for response in trivial failure
+do
+	test_expect_success UNTRACKED_CACHE \
+		"invalidate untracked cache after fsmonitor response: $response" '
+		test_create_repo "untracked-$response" &&
+		(
+			cd "untracked-$response" &&
+			git config core.untrackedCache true &&
+			git config core.fsmonitor .git/hooks/fsmonitor-test &&
+			git config core.fsmonitorHookVersion 2 &&
+			echo empty >.git/fsmonitor-response &&
+			test_hook --setup fsmonitor-test <<-\EOF &&
+			printf "token\0"
+			case "$(cat .git/fsmonitor-response)" in
+			trivial) printf "/\0" ;;
+			failure) exit 1 ;;
+			esac
+			EOF
+
+			mkdir -p inc/deep &&
+			git update-index --force-write-index &&
+			git status --porcelain >../actual &&
+			test_must_be_empty ../actual &&
+
+			>inc/deep/new &&
+			echo "$response" >.git/fsmonitor-response &&
+			git update-index --refresh &&
+			echo empty >.git/fsmonitor-response &&
+
+			test-tool dump-untracked-cache >../actual &&
+			test_grep ! " valid" ../actual &&
+			echo "?? inc/" >../expect &&
+			git status --porcelain >../actual &&
+			test_cmp ../expect ../actual
+		)
+	'
+done
+
 test_expect_success 'discard_index() also discards fsmonitor info' '
 	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
 	test_might_fail git update-index --refresh &&

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
-- 
gitgitgadget