From 7c902e89199870865d0b4d53628617b34a06666e Mon Sep 17 00:00:00 2001 From: dino Date: Tue, 14 Apr 2026 14:40:18 +0100 Subject: [PATCH] checkpoint: introduce trash id and keep trash in fs --- Cargo.lock | 1 + crates/fs/Cargo.toml | 1 + crates/fs/src/fs.rs | 81 ++++++++++++----------- crates/fs/tests/integration/fs.rs | 67 +++---------------- crates/project_panel/src/project_panel.rs | 2 + crates/worktree/src/worktree.rs | 75 +++------------------ 6 files changed, 66 insertions(+), 161 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5280fc72d074c22414b603a9b7092f2005f07a85..01cb046eaabae17c7eb199a455e57ba4e61ac327 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6480,6 +6480,7 @@ dependencies = [ "rope", "serde", "serde_json", + "slotmap", "smol", "tempfile", "text", diff --git a/crates/fs/Cargo.toml b/crates/fs/Cargo.toml index e7b8dcd4ebda7810ef8087e112ae43819702bdf6..ac2f5aad5a220fd2b95cee95753c72bbdaa77bf0 100644 --- a/crates/fs/Cargo.toml +++ b/crates/fs/Cargo.toml @@ -35,6 +35,7 @@ proto.workspace = true thiserror.workspace = true serde.workspace = true serde_json.workspace = true +slotmap.workspace = true smol.workspace = true tempfile.workspace = true text.workspace = true diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index e44f557646239da5dd84354e364422cf16e14233..88a7c675475a7111451061b14b6314638c417faa 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -1,6 +1,7 @@ pub mod fs_watcher; use parking_lot::Mutex; +use slotmap::SlotMap; use std::ffi::OsString; use std::sync::atomic::{AtomicU8, AtomicUsize, Ordering}; use std::time::Instant; @@ -118,7 +119,7 @@ pub trait Fs: Send + Sync { /// Moves a file or directory to the system trash. /// Returns a [`TrashedEntry`] that can be used to keep track of the /// location of the trashed item in the system's trash. - async fn trash(&self, path: &Path, options: RemoveOptions) -> Result; + async fn trash(&self, path: &Path, options: RemoveOptions) -> Result; /// Removes a file from the filesystem. /// There is no expectation that the file will be preserved in the system @@ -167,10 +168,7 @@ pub trait Fs: Send + Sync { /// Restores a given `TrashedEntry`, moving it from the system's trash back /// to the original path. - async fn restore( - &self, - trashed_entry: TrashedEntry, - ) -> std::result::Result; + async fn restore(&self, item: TrashId) -> std::result::Result; #[cfg(feature = "test-support")] fn as_fake(&self) -> Arc { @@ -184,7 +182,7 @@ pub trait Fs: Send + Sync { /// Represents a file or directory that has been moved to the system trash, /// retaining enough information to restore it to its original location. #[derive(Clone, PartialEq, Debug)] -pub struct TrashedEntry { +struct TrashedEntry { /// Platform-specific identifier for the file/directory in the trash. /// /// * Freedesktop – Path to the `.trashinfo` file. @@ -226,6 +224,11 @@ pub enum TrashRestoreError { NotFound { path: PathBuf }, #[error("File or directory ({}) already exists at the restore destination.", path.display())] Collision { path: PathBuf }, + // This should never occur, the only way to get a TrashId is to undo + // consumes the Change::Trashed. We worry about remoting duplicate messages + // we do not want to crash the app then which is why this error is there. + #[error("The item was already restored")] + AlreadyRestored, #[error("Unknown error ({description})")] Unknown { description: String }, } @@ -393,11 +396,15 @@ impl From for proto::Timestamp { } } +// TODO!(yara) for protocol get out u64 via keydata().ffi() +slotmap::new_key_type! { pub struct TrashId; } + pub struct RealFs { bundled_git_binary_path: Option, executor: BackgroundExecutor, next_job_id: Arc, job_event_subscribers: Arc>>, + trash: Arc>>, is_case_sensitive: AtomicU8, } @@ -506,6 +513,7 @@ impl RealFs { executor, next_job_id: Arc::new(AtomicUsize::new(0)), job_event_subscribers: Arc::new(Mutex::new(Vec::new())), + trash: Arc::new(Mutex::new(SlotMap::with_key())), is_case_sensitive: Default::default(), } } @@ -796,7 +804,7 @@ impl Fs for RealFs { } } - async fn trash(&self, path: &Path, _options: RemoveOptions) -> Result { + async fn trash(&self, path: &Path, _options: RemoveOptions) -> Result { // We must make the path absolute or trash will make a weird abomination // of the zed working directory (not usually the worktree) and whatever // the path variable holds. @@ -811,11 +819,14 @@ impl Fs for RealFs { .spawn(|| tx.send(trash::delete_with_info(path))) .expect("The os can spawn threads"); - Ok(rx + let entry = rx .await .context("Tx dropped or fs.restore panicked")? .context("Could not trash file or dir")? - .into()) + .into(); + + let id = self.trash.lock().insert(entry); + Ok(id) } async fn open_sync(&self, path: &Path) -> Result> { @@ -1264,10 +1275,13 @@ impl Fs for RealFs { res } - async fn restore( - &self, - trashed_entry: TrashedEntry, - ) -> std::result::Result { + async fn restore(&self, item: TrashId) -> std::result::Result { + let trashed_entry = self + .trash + .lock() + .remove(item) + .ok_or(TrashRestoreError::AlreadyRestored)?; + let restored_item_path = trashed_entry.original_parent.join(&trashed_entry.name); let (tx, rx) = futures::channel::oneshot::channel(); @@ -1316,7 +1330,7 @@ struct FakeFsState { path_write_counts: std::collections::HashMap, moves: std::collections::HashMap, job_event_subscribers: Arc>>, - trash: Vec<(TrashedEntry, FakeFsEntry)>, + trash: Mutex>, } #[cfg(feature = "test-support")] @@ -1602,7 +1616,7 @@ impl FakeFs { path_write_counts: Default::default(), moves: Default::default(), job_event_subscribers: Arc::new(Mutex::new(Vec::new())), - trash: Vec::new(), + trash: Mutex::new(SlotMap::with_key()), })), }); @@ -2429,16 +2443,6 @@ impl FakeFs { self.executor.simulate_random_delay() } - /// Returns list of all tracked trash entries. - pub fn trash_entries(&self) -> Vec { - self.state - .lock() - .trash - .iter() - .map(|(entry, _)| entry.clone()) - .collect() - } - async fn remove_dir_inner( &self, path: &Path, @@ -2814,7 +2818,7 @@ impl Fs for FakeFs { self.remove_dir_inner(path, options).await.map(|_| ()) } - async fn trash(&self, path: &Path, options: RemoveOptions) -> Result { + async fn trash(&self, path: &Path, options: RemoveOptions) -> Result { let normalized_path = normalize_path(path); let parent_path = normalized_path.parent().context("cannot remove the root")?; let base_name = normalized_path.file_name().unwrap(); @@ -2832,9 +2836,14 @@ impl Fs for FakeFs { original_parent: parent_path.to_path_buf(), }; - let mut state = self.state.lock(); - state.trash.push((trashed_entry.clone(), fake_entry)); - Ok(trashed_entry) + let trash_id = self + .state + .lock() + .trash + .lock() + .insert((trashed_entry.clone(), fake_entry)); + + Ok(trash_id) } None => anyhow::bail!("{normalized_path:?} does not exist"), } @@ -3092,18 +3101,11 @@ impl Fs for FakeFs { receiver } - async fn restore(&self, trashed_entry: TrashedEntry) -> Result { + async fn restore(&self, trash_id: TrashId) -> Result { let mut state = self.state.lock(); - let Some((trashed_entry, fake_entry)) = state - .trash - .iter() - .find(|(entry, _)| *entry == trashed_entry) - .cloned() - else { - return Err(TrashRestoreError::NotFound { - path: PathBuf::from(trashed_entry.id), - }); + let Some((trashed_entry, fake_entry)) = state.trash.lock().remove(trash_id) else { + return Err(TrashRestoreError::AlreadyRestored); }; let path = trashed_entry @@ -3122,7 +3124,6 @@ impl Fs for FakeFs { match result { Ok(_) => { - state.trash.retain(|(entry, _)| *entry != trashed_entry); state.emit_event([(path.clone(), Some(PathEventKind::Created))]); Ok(path) } diff --git a/crates/fs/tests/integration/fs.rs b/crates/fs/tests/integration/fs.rs index 97ec90bea09651bc888dfdea332ad6a4964ede2f..52683fb4a70533939a24b3e145a3c15b82128b17 100644 --- a/crates/fs/tests/integration/fs.rs +++ b/crates/fs/tests/integration/fs.rs @@ -1,6 +1,5 @@ use std::{ collections::BTreeSet, - ffi::OsString, io::Write, path::{Path, PathBuf}, time::Duration, @@ -644,15 +643,11 @@ async fn test_fake_fs_trash(executor: BackgroundExecutor) { .await; // Trashing a file. - let root_path = PathBuf::from(path!("/root")); let path = path!("/root/file_a.txt").as_ref(); - let trashed_entry = fs - .trash(path, Default::default()) + fs.trash(path, Default::default()) .await .expect("should be able to trash {path:?}"); - assert_eq!(trashed_entry.name, "file_a.txt"); - assert_eq!(trashed_entry.original_parent, root_path); assert_eq!( fs.files(), vec![ @@ -662,32 +657,19 @@ async fn test_fake_fs_trash(executor: BackgroundExecutor) { ] ); - let trash_entries = fs.trash_entries(); - assert_eq!(trash_entries.len(), 1); - assert_eq!(trash_entries[0].name, "file_a.txt"); - assert_eq!(trash_entries[0].original_parent, root_path); - // Trashing a directory. let path = path!("/root/src").as_ref(); - let trashed_entry = fs - .trash( - path, - RemoveOptions { - recursive: true, - ..Default::default() - }, - ) - .await - .expect("should be able to trash {path:?}"); + fs.trash( + path, + RemoveOptions { + recursive: true, + ..Default::default() + }, + ) + .await + .expect("should be able to trash {path:?}"); - assert_eq!(trashed_entry.name, "src"); - assert_eq!(trashed_entry.original_parent, root_path); assert_eq!(fs.files(), vec![PathBuf::from(path!("/root/file_b.txt"))]); - - let trash_entries = fs.trash_entries(); - assert_eq!(trash_entries.len(), 2); - assert_eq!(trash_entries[1].name, "src"); - assert_eq!(trash_entries[1].original_parent, root_path); } #[gpui::test] @@ -705,36 +687,14 @@ async fn test_fake_fs_restore(executor: BackgroundExecutor) { ) .await; - // Providing a non-existent `TrashedEntry` should result in an error. - let id = OsString::from("/trash/file_c.txt"); - let name = OsString::from("file_c.txt"); - let original_parent = PathBuf::from(path!("/root")); - let trashed_entry = TrashedEntry { - id, - name, - original_parent, - }; - let result = fs.restore(trashed_entry).await; - assert!(matches!(result, Err(TrashRestoreError::NotFound { .. }))); - // Attempt deleting a file, asserting that the filesystem no longer reports // it as part of its list of files, restore it and verify that the list of // files and trash has been updated accordingly. let path = path!("/root/src/file_a.txt").as_ref(); let trashed_entry = fs.trash(path, Default::default()).await.unwrap(); - assert_eq!(fs.trash_entries().len(), 1); - assert_eq!( - fs.files(), - vec![ - PathBuf::from(path!("/root/file_c.txt")), - PathBuf::from(path!("/root/src/file_b.txt")) - ] - ); - fs.restore(trashed_entry).await.unwrap(); - assert_eq!(fs.trash_entries().len(), 0); assert_eq!( fs.files(), vec![ @@ -754,7 +714,6 @@ async fn test_fake_fs_restore(executor: BackgroundExecutor) { let path = path!("/root/src/").as_ref(); let trashed_entry = fs.trash(path, options).await.unwrap(); - assert_eq!(fs.trash_entries().len(), 1); assert_eq!(fs.files(), vec![PathBuf::from(path!("/root/file_c.txt"))]); fs.restore(trashed_entry).await.unwrap(); @@ -767,14 +726,12 @@ async fn test_fake_fs_restore(executor: BackgroundExecutor) { PathBuf::from(path!("/root/src/file_b.txt")) ] ); - assert_eq!(fs.trash_entries().len(), 0); // A collision error should be returned in case a file is being restored to // a path where a file already exists. let path = path!("/root/src/file_a.txt").as_ref(); let trashed_entry = fs.trash(path, Default::default()).await.unwrap(); - assert_eq!(fs.trash_entries().len(), 1); assert_eq!( fs.files(), vec![ @@ -785,7 +742,6 @@ async fn test_fake_fs_restore(executor: BackgroundExecutor) { fs.write(path, "New File A".as_bytes()).await.unwrap(); - assert_eq!(fs.trash_entries().len(), 1); assert_eq!( fs.files(), vec![ @@ -811,19 +767,16 @@ async fn test_fake_fs_restore(executor: BackgroundExecutor) { let path = path!("/root/src/").as_ref(); let trashed_entry = fs.trash(path, options).await.unwrap(); - assert_eq!(fs.trash_entries().len(), 2); assert_eq!(fs.files(), vec![PathBuf::from(path!("/root/file_c.txt"))]); fs.create_dir(path).await.unwrap(); assert_eq!(fs.files(), vec![PathBuf::from(path!("/root/file_c.txt"))]); - assert_eq!(fs.trash_entries().len(), 2); let result = fs.restore(trashed_entry).await; assert!(result.is_err()); assert_eq!(fs.files(), vec![PathBuf::from(path!("/root/file_c.txt"))]); - assert_eq!(fs.trash_entries().len(), 2); } #[gpui::test] diff --git a/crates/project_panel/src/project_panel.rs b/crates/project_panel/src/project_panel.rs index 3690a870c4fd3b1a4827ec0b772aa3fe22f80cd6..fa0b89c3d6eab811cd0f96c7b9784fd8cb2771f5 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/crates/project_panel/src/project_panel.rs @@ -2213,6 +2213,8 @@ impl ProjectPanel { self.rename_impl(None, window, cx); } + // TODO!(dino): Ensure that the listener in remote is enabled, right now is + // disabled. fn trash(&mut self, action: &Trash, window: &mut Window, cx: &mut Context) { dbg!("ProjectPanel::trash"); self.remove(true, action.skip_prompt, window, cx); diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index 25de66e5c090edbe5025ddb8b96c614ef047b744..ae9fbde907da90a52f18cf05ed4677449ba6f76a 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -8,7 +8,7 @@ use clock::ReplicaId; use collections::{HashMap, HashSet, VecDeque}; use encoding_rs::Encoding; use fs::{ - Fs, MTime, PathEvent, PathEventKind, RemoveOptions, TrashedEntry, Watcher, copy_recursive, + Fs, MTime, PathEvent, PathEventKind, RemoveOptions, TrashId, Watcher, copy_recursive, read_dir_items, }; use futures::{ @@ -849,16 +849,10 @@ impl Worktree { entry_id: ProjectEntryId, trash: bool, cx: &mut Context, - ) -> Option>>> { + ) -> Option>>> { let task = match self { - Worktree::Local(this) => { - dbg!(("LOCAL", trash)); - this.delete_entry(entry_id, trash, cx) - } - Worktree::Remote(this) => { - dbg!(("REMOTE", trash)); - this.delete_entry(entry_id, trash, cx) - } + Worktree::Local(this) => this.delete_entry(entry_id, trash, cx), + Worktree::Remote(this) => this.delete_entry(entry_id, trash, cx), }?; let entry = match &*self { @@ -878,13 +872,13 @@ impl Worktree { } pub async fn restore_entry( - trash_entry: TrashedEntry, + trash_id: TrashId, worktree: Entity, cx: &mut AsyncApp, ) -> Result { let is_local = worktree.read_with(cx, |this, _| this.is_local()); if is_local { - LocalWorktree::restore_entry(trash_entry, worktree, cx).await + LocalWorktree::restore_entry(trash_id, worktree, cx).await } else { // TODO(dino): Add support for restoring entries in remote worktrees. Err(anyhow!("Unsupported")) @@ -899,18 +893,6 @@ impl Worktree { } } - // pub fn rename_entry( - // &mut self, - // entry_id: ProjectEntryId, - // new_path: Arc, - // cx: &Context, - // ) -> Task> { - // match self { - // Worktree::Local(this) => this.rename_entry(entry_id, new_path, cx), - // Worktree::Remote(this) => this.rename_entry(entry_id, new_path, cx), - // } - // } - pub fn copy_external_entries( &mut self, target_directory: Arc, @@ -1019,13 +1001,13 @@ impl Worktree { ), ) }); - let trashed_entry = task + let trash_id = task .ok_or_else(|| anyhow::anyhow!("invalid entry"))? .await?; Ok(proto::ProjectEntryResponse { entry: None, worktree_scan_id: scan_id as u64, - trashed_entry: trashed_entry.map(|e| proto::TrashedEntry { + trashed_entry: trash_id.map(|e| proto::TrashedEntry { trash_id: e.id.to_string_lossy().to_string(), file_name: e.name.to_string_lossy().to_string(), original_parent_path: e.original_parent.to_string_lossy().to_string(), @@ -1713,13 +1695,13 @@ impl LocalWorktree { entry_id: ProjectEntryId, trash: bool, cx: &Context, - ) -> Option>>> { + ) -> Option>>> { let entry = self.entry_for_id(entry_id)?.clone(); let abs_path = self.absolutize(&entry.path); let fs = self.fs.clone(); let delete = cx.background_spawn(async move { - let trashed_entry = match (entry.is_file(), trash) { + let trash_id = match (entry.is_file(), trash) { (true, true) => Some(fs.trash(&abs_path, Default::default()).await?), (false, true) => Some( fs.trash( @@ -1748,7 +1730,7 @@ impl LocalWorktree { } }; - anyhow::Ok((trashed_entry, entry.path)) + anyhow::Ok((trash_id, entry.path)) }); Some(cx.spawn(async move |this, cx| { @@ -2191,41 +2173,6 @@ impl RemoteWorktree { })) } - // fn rename_entry( - // &self, - // entry_id: ProjectEntryId, - // new_path: impl Into>, - // cx: &Context, - // ) -> Task> { - // let new_path: Arc = new_path.into(); - // let response = self.client.request(proto::RenameProjectEntry { - // project_id: self.project_id, - // entry_id: entry_id.to_proto(), - // new_worktree_id: new_path.worktree_id, - // new_path: new_path.as_ref().to_proto(), - // }); - // cx.spawn(async move |this, cx| { - // let response = response.await?; - // match response.entry { - // Some(entry) => this - // .update(cx, |this, cx| { - // this.as_remote_mut().unwrap().insert_entry( - // entry, - // response.worktree_scan_id as usize, - // cx, - // ) - // })? - // .await - // .map(CreatedEntry::Included), - // None => { - // let abs_path = - // this.read_with(cx, |worktree, _| worktree.absolutize(&new_path))?; - // Ok(CreatedEntry::Excluded { abs_path }) - // } - // } - // }) - // } - fn copy_external_entries( &self, target_directory: Arc,