diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index c31385f455..6c90de189b 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -272,16 +272,65 @@ struct BackgroundScannerState { watched_dir_abs_paths_by_entry_id: HashMap>, path_prefixes_to_scan: HashSet>, paths_to_scan: HashSet>, - /// The ids of all of the entries that were removed from the snapshot - /// 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_entries: HashMap, + removed_entries: RemovedEntries, changed_paths: Vec>, prev_snapshot: Snapshot, scanning_enabled: bool, } +/// The entries that were removed from the snapshot as part of the current +/// update. Their 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. +/// +/// Symlink aliases inside the worktree share their inode (and usually mtime) +/// with the symlink target, so an inode may correspond to several entries. +/// The path index allows an exact match to take precedence over the +/// inode-based rename heuristics in that case. +#[derive(Default)] +struct RemovedEntries { + by_inode: HashMap, + by_path: HashMap, Entry>, +} + +impl RemovedEntries { + fn insert(&mut self, entry: &Entry) { + self.by_path.insert(entry.path.clone(), entry.clone()); + match self.by_inode.entry(entry.inode) { + hash_map::Entry::Occupied(mut o) => { + if entry.id > o.get().id { + o.insert(entry.clone()); + } + } + hash_map::Entry::Vacant(v) => { + v.insert(entry.clone()); + } + } + } + + fn take_by_path(&mut self, path: &RelPath, inode: u64) -> Option { + if self.by_path.get(path)?.inode != inode { + return None; + } + let removed = self.by_path.remove(path)?; + if let hash_map::Entry::Occupied(o) = self.by_inode.entry(removed.inode) + && o.get().id == removed.id + { + o.remove(); + } + Some(removed) + } + + fn take_by_inode(&mut self, inode: u64) -> Option { + let removed = self.by_inode.remove(&inode)?; + if let hash_map::Entry::Occupied(o) = self.by_path.entry(removed.path.clone()) + && o.get().id == removed.id + { + o.remove(); + } + Some(removed) + } +} + #[derive(Clone, Debug, Eq, PartialEq)] struct EventRoot { path: Arc, @@ -1229,7 +1278,7 @@ impl LocalWorktree { scanning_enabled, path_prefixes_to_scan: Default::default(), paths_to_scan: Default::default(), - removed_entries: Default::default(), + removed_entries: RemovedEntries::default(), changed_paths: Default::default(), }), phase: BackgroundScannerPhase::InitialScan, @@ -3086,21 +3135,11 @@ impl BackgroundScannerState { } fn reuse_entry_id(&mut self, entry: &mut Entry) { - if let Some(mtime) = entry.mtime { - // If an entry with the same inode was removed from the worktree during this scan, - // then it *might* represent the same file or directory. But the OS might also have - // re-used the inode for a completely different file or directory. - // - // Conditionally reuse the old entry's id: - // * if the mtime is the same, the file was probably been renamed. - // * if the path is the same, the file may just have been updated - if let Some(removed_entry) = self.removed_entries.remove(&entry.inode) { - if removed_entry.mtime == Some(mtime) || removed_entry.path == entry.path { - entry.id = removed_entry.id; - } - } else if let Some(existing_entry) = self.snapshot.entry_for_path(&entry.path) { - entry.id = existing_entry.id; - } + let Some(mtime) = entry.mtime else { + return; + }; + if let Some(entry_id) = self.reused_entry_id(&entry.path, entry.inode, mtime) { + entry.id = entry_id; } } @@ -3110,6 +3149,20 @@ impl BackgroundScannerState { path: &RelPath, metadata: &fs::Metadata, ) -> ProjectEntryId { + self.reused_entry_id(path, metadata.inode, metadata.mtime) + .unwrap_or_else(|| ProjectEntryId::new(next_entry_id)) + } + + fn reused_entry_id( + &mut self, + path: &RelPath, + inode: u64, + mtime: MTime, + ) -> Option { + if let Some(removed_entry) = self.removed_entries.take_by_path(path, inode) { + return Some(removed_entry.id); + } + // If an entry with the same inode was removed from the worktree during this scan, // then it *might* represent the same file or directory. But the OS might also have // re-used the inode for a completely different file or directory. @@ -3117,14 +3170,12 @@ impl BackgroundScannerState { // Conditionally reuse the old entry's id: // * if the mtime is the same, the file was probably been renamed. // * if the path is the same, the file may just have been updated - if let Some(removed_entry) = self.removed_entries.remove(&metadata.inode) { - if removed_entry.mtime == Some(metadata.mtime) || *removed_entry.path == *path { - return removed_entry.id; - } - } else if let Some(existing_entry) = self.snapshot.entry_for_path(path) { - return existing_entry.id; + if let Some(removed_entry) = self.removed_entries.take_by_inode(inode) { + (removed_entry.mtime == Some(mtime) || *removed_entry.path == *path) + .then_some(removed_entry.id) + } else { + Some(self.snapshot.entry_for_path(path)?.id) } - ProjectEntryId::new(next_entry_id) } async fn insert_entry(&mut self, entry: Entry, fs: &dyn Fs, watcher: &dyn Watcher) -> Entry { @@ -3292,17 +3343,7 @@ impl BackgroundScannerState { removed_dir_abs_paths.push(watch_path); } - match self.removed_entries.entry(entry.inode) { - hash_map::Entry::Occupied(mut e) => { - let prev_removed_entry = e.get_mut(); - if entry.id > prev_removed_entry.id { - *prev_removed_entry = entry.clone(); - } - } - hash_map::Entry::Vacant(e) => { - e.insert(entry.clone()); - } - } + self.removed_entries.insert(entry); if entry.path.file_name() == Some(GITIGNORE) { let abs_parent_path = self.snapshot.absolutize(&entry.path.parent().unwrap()); @@ -4800,7 +4841,8 @@ impl BackgroundScanner { { let mut state = self.state.lock().await; state.snapshot.completed_scan_id = state.snapshot.scan_id; - for (_, entry) in mem::take(&mut state.removed_entries) { + let RemovedEntries { by_inode, by_path } = mem::take(&mut state.removed_entries); + for entry in by_inode.into_values().chain(by_path.into_values()) { state.scanned_dirs.remove(&entry.id); } } diff --git a/crates/worktree/tests/integration/worktree_tests.rs b/crates/worktree/tests/integration/worktree_tests.rs index 1fd7ed5b55..f63bf7d108 100644 --- a/crates/worktree/tests/integration/worktree_tests.rs +++ b/crates/worktree/tests/integration/worktree_tests.rs @@ -1198,6 +1198,98 @@ async fn test_real_fs_scan_symlinks_expanded(cx: &mut TestAppContext) { }); } +#[gpui::test] +async fn test_internal_symlink_updates_preserve_entry_ids(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.background_executor.clone()); + + fs.insert_tree( + "/root", + json!({ + "project": { + "real-dir": { + "existing.rs": "old", + }, + "links": {} + } + }), + ) + .await; + + fs.create_symlink( + "/root/project/links/internal".as_ref(), + "../real-dir".into(), + ) + .await + .unwrap(); + + let tree = Worktree::local( + Path::new("/root/project"), + true, + fs.clone(), + Default::default(), + true, + WorktreeId::from_proto(0), + &mut cx.to_async(), + ) + .await + .unwrap(); + + cx.read(|cx| tree.read(cx).as_local().unwrap().scan_complete()) + .await; + + let (real_entry_id, symlink_entry_id, old_mtime) = tree.read_with(cx, |tree, _| { + let real_entry = tree + .entry_for_path(rel_path("real-dir/existing.rs")) + .unwrap(); + let symlink_entry = tree + .entry_for_path(rel_path("links/internal/existing.rs")) + .unwrap(); + assert_eq!(real_entry.inode, symlink_entry.inode); + assert_eq!(real_entry.mtime, symlink_entry.mtime); + assert_ne!(real_entry.id, symlink_entry.id); + (real_entry.id, symlink_entry.id, real_entry.mtime) + }); + + fs.write(Path::new("/root/project/real-dir/existing.rs"), b"new") + .await + .unwrap(); + + wait_for_condition(cx, |cx| { + tree.read_with(cx, |tree, _| { + let real_entry = tree + .entry_for_path(rel_path("real-dir/existing.rs")) + .unwrap(); + let symlink_entry = tree + .entry_for_path(rel_path("links/internal/existing.rs")) + .unwrap(); + real_entry.mtime != old_mtime && symlink_entry.mtime != old_mtime + }) + }) + .await; + + tree.read_with(cx, |tree, _| { + let real_entry = tree + .entry_for_path(rel_path("real-dir/existing.rs")) + .unwrap(); + let symlink_entry = tree + .entry_for_path(rel_path("links/internal/existing.rs")) + .unwrap(); + + assert_eq!(real_entry.inode, symlink_entry.inode); + assert_eq!(real_entry.id, real_entry_id); + assert_eq!( + tree.entry_for_id(real_entry_id).unwrap().path.as_ref(), + rel_path("real-dir/existing.rs") + ); + assert_eq!(symlink_entry.id, symlink_entry_id); + assert_eq!( + tree.entry_for_id(symlink_entry_id).unwrap().path.as_ref(), + rel_path("links/internal/existing.rs") + ); + }); +} + #[cfg(target_os = "macos")] #[gpui::test] async fn test_renaming_case_only(cx: &mut TestAppContext) {