fs: Keep trash registry entry when restore fails (#61791)
# Objective A failed restore, for example, a collision at the original path, used to remove the `TrashedEntry` from the registry before attempting the operation, so any later attempt with the same `TrashId` would report an `AlreadyRestored` error, even though the item still sat in the system trash. ## Solution Update both `RealFs::restore` and `FakeFs::restore` to remove the entry only after the restore succeeds. There's also a small unrelated change in `ProjectPanel::drag_onto` to log, rather than silently discard, a failed undo-history record in the project panel, matching existing call sites. ## Testing Introduced a new test for these changes – `restore_can_be_retried_after_collision` . ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [ ] Unsafe blocks (if any) have justifying comments - [ ] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable --- Release Notes: - N/A
This commit is contained in:
1 parent
7ba34d8cbe
commit
b7fd7b7b4c
3 files changed
+46
-13
No files matched your search
+6
-2
@@ -1362,7 +1362,8 @@ impl Fs for RealFs {
|
||||
let trashed_entry = self
|
||||
.trash
|
||||
.lock()
|
||||
.remove(trash_id)
|
||||
.get(trash_id)
|
||||
.cloned()
|
||||
.ok_or(TrashRestoreError::AlreadyRestored)?;
|
||||
|
||||
let restored_item_path = trashed_entry.original_parent.join(&trashed_entry.name);
|
||||
@@ -1375,7 +1376,9 @@ impl Fs for RealFs {
|
||||
tx.send(res)
|
||||
})
|
||||
.expect("The OS can spawn a threads");
|
||||
|
||||
rx.await.expect("Restore all never panics")?;
|
||||
self.trash.lock().remove(trash_id);
|
||||
Ok(restored_item_path)
|
||||
}
|
||||
}
|
||||
@@ -3346,7 +3349,7 @@ impl Fs for FakeFs {
|
||||
async fn restore(&self, trash_id: TrashId) -> Result<PathBuf, TrashRestoreError> {
|
||||
let mut state = self.state.lock();
|
||||
|
||||
let Some((trashed_entry, fake_entry)) = state.trash.lock().remove(trash_id) else {
|
||||
let Some((trashed_entry, fake_entry)) = state.trash.lock().get(trash_id).cloned() else {
|
||||
return Err(TrashRestoreError::AlreadyRestored);
|
||||
};
|
||||
|
||||
@@ -3366,6 +3369,7 @@ impl Fs for FakeFs {
|
||||
|
||||
match result {
|
||||
Ok(_) => {
|
||||
state.trash.lock().remove(trash_id);
|
||||
state.emit_event([(path.clone(), Some(PathEventKind::Created))]);
|
||||
Ok(path)
|
||||
}
|
||||
|
||||
@@ -1126,3 +1126,30 @@ async fn test_realfs_watch_stress_reports_missed_paths(
|
||||
missed_paths.len()
|
||||
);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn restore_can_be_retried_after_collision(cx: &mut TestAppContext) {
|
||||
let fs = FakeFs::new(cx.background_executor.clone());
|
||||
let path = path!("/root/a.txt");
|
||||
let remove_options = RemoveOptions::default();
|
||||
fs.insert_tree(path!("/root"), json!({ "a.txt": "original"}))
|
||||
.await;
|
||||
|
||||
// We'll first trash the `a.txt` file so we can hold onto its `TrashId`,
|
||||
// allowing us to later attempt restoring it again, ensuring that it didn't
|
||||
// get removed from the trash state, even if restoring failed.
|
||||
let trash_id = fs.trash(path.as_ref(), remove_options).await.unwrap();
|
||||
|
||||
fs.insert_file(path, "conflicting".into()).await;
|
||||
let err = fs.restore(trash_id).await.unwrap_err();
|
||||
assert!(matches!(err, TrashRestoreError::Collision { .. }));
|
||||
|
||||
fs.remove_file(path.as_ref(), remove_options).await.unwrap();
|
||||
let restored_path = fs.restore(trash_id).await.unwrap();
|
||||
assert_eq!(fs.load(restored_path.as_path()).await.unwrap(), "original");
|
||||
|
||||
assert!(matches!(
|
||||
fs.restore(trash_id).await.unwrap_err(),
|
||||
TrashRestoreError::AlreadyRestored
|
||||
));
|
||||
}
|
||||
@@ -4831,18 +4831,20 @@ impl ProjectPanel {
|
||||
}
|
||||
// update selection
|
||||
if let Some(entry_id) = last_succeed {
|
||||
project_panel.update_in(cx, |project_panel, window, cx| {
|
||||
project_panel.selection = Some(SelectedEntry {
|
||||
worktree_id,
|
||||
entry_id,
|
||||
});
|
||||
// if only one entry was dragged and it was disambiguated, open the rename editor
|
||||
if item_count == 1 && disambiguation_range.is_some() {
|
||||
project_panel.rename_impl(disambiguation_range, window, cx);
|
||||
}
|
||||
project_panel
|
||||
.update_in(cx, |project_panel, window, cx| {
|
||||
project_panel.selection = Some(SelectedEntry {
|
||||
worktree_id,
|
||||
entry_id,
|
||||
});
|
||||
// if only one entry was dragged and it was disambiguated, open the rename editor
|
||||
if item_count == 1 && disambiguation_range.is_some() {
|
||||
project_panel.rename_impl(disambiguation_range, window, cx);
|
||||
}
|
||||
|
||||
project_panel.undo_manager.record(changes)
|
||||
})??;
|
||||
project_panel.undo_manager.record(changes)
|
||||
})?
|
||||
.log_err();
|
||||
}
|
||||
|
||||
std::result::Result::Ok::<(), anyhow::Error>(())
|
||||
|
||||
Reference in new issue
Block a user