working_copy: avoid "empty" fsmonitor saves

lib/src/local_working_copy.rs

@@ -958,6 +958,7 @@ fn file_state(metadata: &Metadata) -> Result<Option<FileState>, MtimeOutOfRange>
 struct FsmonitorMatcher {
     matcher: Option<Box<dyn Matcher>>,
     watchman_clock: Option<crate::protos::local_working_copy::WatchmanClock>,
+    should_update_watchman_clock: bool,
 }
 
 /// Settings specific to the tree state of the [`LocalWorkingCopy`] backend.
@@ -1303,11 +1304,11 @@ impl TreeState {
 
         let sparse_matcher = self.sparse_matcher();
 
-        let fsmonitor_clock_needs_save = self.fsmonitor_settings != FsmonitorSettings::None;
-        let mut is_dirty = fsmonitor_clock_needs_save;
+        let mut is_dirty = false;
         let FsmonitorMatcher {
             matcher: fsmonitor_matcher,
             watchman_clock,
+            should_update_watchman_clock,
         } = self
             .make_fsmonitor_matcher(&self.fsmonitor_settings)
             .await?;
@@ -1321,8 +1322,10 @@ impl TreeState {
             UnionMatcher::new(fsmonitor_matcher, force_tracking_matcher),
         );
         if matcher.visit(RepoPath::root()).is_nothing() {
-            // No need to load the current tree, set up channels, etc.
-            self.watchman_clock = watchman_clock;
+            if should_update_watchman_clock {
+                is_dirty |= self.watchman_clock != watchman_clock;
+                self.watchman_clock = watchman_clock;
+            }
             return Ok((is_dirty, SnapshotStats::default()));
         }
 
@@ -1412,11 +1415,15 @@ impl TreeState {
         // Since untracked paths aren't cached in the tree state, we'll need to
         // rescan the working directory changes to report or track them later.
         // TODO: store untracked paths and update watchman_clock?
-        if (stats.untracked_paths.is_empty() && stats.invalid_utf8_paths.is_empty())
-            || watchman_clock.is_none()
+        // A failed monitor query has no clock and causes a full scan. Clear the
+        // old clock after that scan even if it found untracked paths.
+        if should_update_watchman_clock
+            && ((stats.untracked_paths.is_empty() && stats.invalid_utf8_paths.is_empty())
+                || watchman_clock.is_none())
         {
+            is_dirty |= self.watchman_clock != watchman_clock;
             self.watchman_clock = watchman_clock;
-        } else {
+        } else if should_update_watchman_clock {
             tracing::info!("not updating watchman clock because there are untracked files");
         }
         Ok((is_dirty, stats))
@@ -1448,6 +1455,12 @@ impl TreeState {
                 });
             }
         };
+        // Do not advance the clock when the monitor reports no changed paths.
+        // This avoids rewriting the tree state just to save the new clock.
+        // Reusing the older clock may make a later query do more work, but it
+        // cannot omit changes.
+        let should_update_watchman_clock =
+            !matches!(&changed_files, Some(files) if files.is_empty());
         let matcher: Option<Box<dyn Matcher>> = match changed_files {
             None => None,
             Some(changed_files) => {
@@ -1485,6 +1498,7 @@ impl TreeState {
         Ok(FsmonitorMatcher {
             matcher,
             watchman_clock,
+            should_update_watchman_clock,
         })
     }
 }

lib/tests/test_local_working_copy.rs

@@ -53,6 +53,7 @@ use jj_lib::merge::SameChange;
 use jj_lib::merged_tree::MergedTree;
 use jj_lib::merged_tree_builder::MergedTreeBuilder;
 use jj_lib::op_store::OperationId;
+use jj_lib::protos::local_working_copy as working_copy_proto;
 use jj_lib::ref_name::WorkspaceName;
 use jj_lib::repo::ReadonlyRepo;
 use jj_lib::repo::Repo as _;
@@ -69,6 +70,7 @@ use jj_lib::working_copy::UntrackedReason;
 use jj_lib::working_copy::WorkingCopy as _;
 use jj_lib::workspace::Workspace;
 use pollster::FutureExt as _;
+use prost::Message as _;
 use test_case::test_case;
 use testutils::CommitBuilderExt as _;
 use testutils::TestRepo;
@@ -2706,6 +2708,17 @@ fn test_fsmonitor() -> TestResult {
         state_path.clone(),
         &tree_state_settings,
     )?;
+    // Seed the tree state with a clock from an earlier monitor query.
+    let tree_state_path = state_path.join("tree_state");
+    let old_watchman_clock = working_copy_proto::WatchmanClock {
+        watchman_clock: Some(
+            working_copy_proto::watchman_clock::WatchmanClock::StringClock("c:1:1".to_string()),
+        ),
+    };
+    let mut proto =
+        working_copy_proto::TreeState::decode(std::fs::read(&tree_state_path)?.as_slice())?;
+    proto.watchman_clock = Some(old_watchman_clock.clone());
+    std::fs::write(&tree_state_path, proto.encode_to_vec())?;
 
     let foo_path = repo_path("foo");
     let bar_path = repo_path("bar");
@@ -2735,24 +2748,30 @@ fn test_fsmonitor() -> TestResult {
             &settings,
         )
         .unwrap();
-        tree_state
+        let (is_dirty, _) = tree_state
             .snapshot(&empty_snapshot_options())
             .block_on()
             .unwrap();
-        tree_state
+        (is_dirty, tree_state)
     };
 
-    let tree_state = snapshot(&[]);
+    let (is_dirty, mut tree_state) = snapshot(&[]);
+    // An empty response should not require saving the tree state.
+    assert!(!is_dirty);
     assert_tree_eq!(*tree_state.current_tree(), repo.store().empty_merged_tree());
+    // Saving explicitly verifies that the in-memory clock was retained.
+    tree_state.save()?;
+    let proto = working_copy_proto::TreeState::decode(std::fs::read(&tree_state_path)?.as_slice())?;
+    assert_eq!(proto.watchman_clock, Some(old_watchman_clock));
 
-    let tree_state = snapshot(&[foo_path]);
+    let (_, tree_state) = snapshot(&[foo_path]);
     insta::assert_snapshot!(testutils::dump_tree(tree_state.current_tree()), @r#"
     merged tree (sides: 1)
       tree 2a5341b103917cfdb48a
         file "foo" (e99c2057c15160add351): "foo\n"
     "#);
 
-    let mut tree_state = snapshot(&[foo_path, bar_path, nested_path, ignored_path]);
+    let (_, mut tree_state) = snapshot(&[foo_path, bar_path, nested_path, ignored_path]);
     insta::assert_snapshot!(testutils::dump_tree(tree_state.current_tree()), @r#"
     merged tree (sides: 1)
       tree 1c5c336421714b1df7bb
@@ -2764,7 +2783,7 @@ fn test_fsmonitor() -> TestResult {
 
     testutils::write_working_copy_file(&workspace_root, foo_path, "updated foo\n");
     testutils::write_working_copy_file(&workspace_root, bar_path, "updated bar\n");
-    let tree_state = snapshot(&[foo_path]);
+    let (_, tree_state) = snapshot(&[foo_path]);
     insta::assert_snapshot!(testutils::dump_tree(tree_state.current_tree()), @r#"
     merged tree (sides: 1)
       tree f653dfa18d0b025bdb9e
@@ -2774,7 +2793,7 @@ fn test_fsmonitor() -> TestResult {
     "#);
 
     std::fs::remove_file(foo_path.to_fs_path_unchecked(&workspace_root))?;
-    let mut tree_state = snapshot(&[foo_path]);
+    let (_, mut tree_state) = snapshot(&[foo_path]);
     insta::assert_snapshot!(testutils::dump_tree(tree_state.current_tree()), @r#"
     merged tree (sides: 1)
       tree b7416fc248a038b920c3