From af45db6d1e19f0a2ecc735426ce1509921c665fe Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 14 Jun 2024 16:03:34 -0700 Subject: [PATCH] Fix FS-related issues that were causing a test failure on linux (#13072) This fixes `project_tests::rescan_and_remote_updates` . That test was actually correctly failing, revealing two bugs on Linux. Release Notes: - Fixed an issue where file renames were not detected on Linux. - Fixed performance problems caused by excessive file system events on Linux. --------- Co-authored-by: Mikayla --- crates/fs/src/fs.rs | 34 +++++++++++++++++++---------- crates/project/src/project_tests.rs | 2 -- crates/worktree/src/worktree.rs | 25 ++++++++++++--------- 3 files changed, 37 insertions(+), 24 deletions(-) diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 6e4cfcaa74..7b76c4327c 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -135,8 +135,6 @@ pub struct RealFs { } pub struct RealWatcher { - #[cfg(target_os = "linux")] - root_path: PathBuf, #[cfg(target_os = "linux")] fs_watcher: parking_lot::Mutex, } @@ -452,25 +450,38 @@ impl Fs for RealFs { async fn watch( &self, path: &Path, - _latency: Duration, + latency: Duration, ) -> ( Pin>>>, Arc, ) { + use parking_lot::Mutex; + let (tx, rx) = smol::channel::unbounded(); + let pending_paths: Arc>> = Default::default(); + let root_path = path.to_path_buf(); let file_watcher = notify::recommended_watcher({ let tx = tx.clone(); + let pending_paths = pending_paths.clone(); move |event: Result| { if let Some(event) = event.log_err() { - tx.try_send(event.paths).ok(); + let mut paths = event.paths; + paths.retain(|path| path.starts_with(&root_path)); + if !paths.is_empty() { + paths.sort(); + let mut pending_paths = pending_paths.lock(); + if pending_paths.is_empty() { + tx.try_send(()).ok(); + } + util::extend_sorted(&mut *pending_paths, paths, usize::MAX, PathBuf::cmp); + } } } }) .expect("Could not start file watcher"); let watcher = Arc::new(RealWatcher { - root_path: path.to_path_buf(), fs_watcher: parking_lot::Mutex::new(file_watcher), }); @@ -484,14 +495,13 @@ impl Fs for RealFs { ( Box::pin(rx.filter_map({ let watcher = watcher.clone(); - move |mut paths| { - paths.retain(|path| path.starts_with(&watcher.root_path)); + move |_| { + let _ = watcher.clone(); + let pending_paths = pending_paths.clone(); async move { - if paths.is_empty() { - None - } else { - Some(paths) - } + smol::Timer::after(latency).await; + let paths = std::mem::take(&mut *pending_paths.lock()); + (!paths.is_empty()).then_some(paths) } } })), diff --git a/crates/project/src/project_tests.rs b/crates/project/src/project_tests.rs index 09fdd439dd..ed83d6d1ce 100644 --- a/crates/project/src/project_tests.rs +++ b/crates/project/src/project_tests.rs @@ -2977,8 +2977,6 @@ async fn test_save_as(cx: &mut gpui::TestAppContext) { assert_eq!(opened_buffer, buffer); } -// This test is currently disabled on Linux as it fails fails pretty consistently on that target. -#[cfg(not(target_os = "linux"))] #[gpui::test(retries = 5)] async fn test_rescan_and_remote_updates(cx: &mut gpui::TestAppContext) { use worktree::WorktreeModelHandle as _; diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index 1b6e2f8194..01cf4189a2 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -303,7 +303,7 @@ struct BackgroundScannerState { /// as part of the current update. These entry ids may be re-used /// if the same inode is discovered at a new path, or if the given /// path is re-created after being deleted. - removed_entry_ids: HashMap, + removed_entry_ids: HashMap<(u64, SystemTime), ProjectEntryId>, changed_paths: Vec>, prev_snapshot: Snapshot, } @@ -2638,10 +2638,13 @@ impl BackgroundScannerState { } fn reuse_entry_id(&mut self, entry: &mut Entry) { - if let Some(removed_entry_id) = self.removed_entry_ids.remove(&entry.inode) { - entry.id = removed_entry_id; - } else if let Some(existing_entry) = self.snapshot.entry_for_path(&entry.path) { - entry.id = existing_entry.id; + if let Some(mtime) = entry.mtime { + if let Some(removed_entry_id) = self.removed_entry_ids.remove(&(entry.inode, mtime)) { + eprintln!("detected that entry {entry:?} was renamed from inode and mtime reusing id {removed_entry_id:?}"); + entry.id = removed_entry_id; + } else if let Some(existing_entry) = self.snapshot.entry_for_path(&entry.path) { + entry.id = existing_entry.id; + } } } @@ -2732,11 +2735,13 @@ impl BackgroundScannerState { let mut entries_by_id_edits = Vec::new(); for entry in removed_entries.cursor::<()>() { - let removed_entry_id = self - .removed_entry_ids - .entry(entry.inode) - .or_insert(entry.id); - *removed_entry_id = cmp::max(*removed_entry_id, entry.id); + if let Some(mtime) = entry.mtime { + let removed_entry_id = self + .removed_entry_ids + .entry((entry.inode, mtime)) + .or_insert(entry.id); + *removed_entry_id = cmp::max(*removed_entry_id, entry.id); + } entries_by_id_edits.push(Edit::Remove(entry.id)); } self.snapshot.entries_by_id.edit(entries_by_id_edits, &());