diff --git a/crates/agent_ui/src/thread_worktree_archive.rs b/crates/agent_ui/src/thread_worktree_archive.rs index a2654ef3b43c4e24d23a8c0eff289d61d65a629d..74fac10a9f3158ca12ea5b8a423d7e265af9d150 100644 --- a/crates/agent_ui/src/thread_worktree_archive.rs +++ b/crates/agent_ui/src/thread_worktree_archive.rs @@ -189,20 +189,40 @@ async fn remove_root_after_worktree_removal( } } + // Delete the directory ourselves first, then tell git to clean up the + // metadata. This avoids a problem where `git worktree remove` can + // remove the metadata in `.git/worktrees/` but fail to delete + // the directory (git continues past directory-removal errors), leaving + // an orphaned folder on disk. By deleting the directory first, we + // guarantee it's gone, and `git worktree remove --force` with a + // missing working tree just cleans up the admin entry. + let root_path = root.root_path.clone(); + cx.background_executor() + .spawn(async move { + match std::fs::remove_dir_all(&root_path) { + Ok(()) => Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(error) => Err(error), + } + }) + .await + .with_context(|| { + format!( + "failed to delete worktree directory '{}'", + root.root_path.display() + ) + })?; + let (repo, _temp_project) = find_or_create_repository(&root.main_repo_path, cx).await?; - // force=true is required because the working directory is still dirty - // — persist_worktree_state captures state into detached commits without - // modifying the real index or working tree, so git refuses to delete - // the worktree without --force. let receiver = repo.update(cx, |repo: &mut Repository, _cx| { repo.remove_worktree(root.root_path.clone(), true) }); let result = receiver .await - .map_err(|_| anyhow!("git worktree removal was canceled"))?; + .map_err(|_| anyhow!("git worktree metadata cleanup was canceled"))?; // Keep _temp_project alive until after the await so the headless project isn't dropped mid-operation drop(_temp_project); - result.context("git worktree removal failed")?; + result.context("git worktree metadata cleanup failed")?; remove_empty_parent_dirs_up_to_worktrees_base( root.root_path.clone(), @@ -792,7 +812,7 @@ fn current_app_state(cx: &mut AsyncApp) -> Option> { #[cfg(test)] mod tests { use super::*; - use fs::FakeFs; + use fs::{FakeFs, Fs as _}; use git::repository::Worktree as GitWorktree; use gpui::TestAppContext; use project::Project; @@ -1029,4 +1049,195 @@ mod tests { ); }); } + + #[gpui::test] + async fn test_remove_root_deletes_directory_and_git_metadata(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + "/project", + json!({ + ".git": {}, + "src": { "main.rs": "fn main() {}" } + }), + ) + .await; + fs.set_branch_name(Path::new("/project/.git"), Some("main")); + fs.insert_branches(Path::new("/project/.git"), &["main", "feature"]); + + fs.add_linked_worktree_for_repo( + Path::new("/project/.git"), + true, + GitWorktree { + path: PathBuf::from("/linked-worktree"), + ref_name: Some("refs/heads/feature".into()), + sha: "abc123".into(), + is_main: false, + is_bare: false, + }, + ) + .await; + + let project = Project::test( + fs.clone(), + [Path::new("/project"), Path::new("/linked-worktree")], + cx, + ) + .await; + project + .update(cx, |project, cx| project.git_scans_complete(cx)) + .await; + + let multi_workspace = + cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = multi_workspace + .read_with(cx, |mw, _cx| mw.workspace().clone()) + .unwrap(); + + cx.run_until_parked(); + + // Build the root plan while the worktree is still loaded. + let root = workspace + .read_with(cx, |_workspace, cx| { + build_root_plan( + Path::new("/linked-worktree"), + std::slice::from_ref(&workspace), + cx, + ) + }) + .expect("should produce a root plan for the linked worktree"); + + assert!(fs.is_dir(Path::new("/linked-worktree")).await); + + // Remove the root. + let task = cx.update(|cx| cx.spawn(async move |cx| remove_root(root, cx).await)); + task.await.expect("remove_root should succeed"); + + cx.run_until_parked(); + + // The FakeFs directory should be gone (removed by the FakeGitRepository + // backend's remove_worktree implementation). + assert!( + !fs.is_dir(Path::new("/linked-worktree")).await, + "linked worktree directory should be removed from FakeFs" + ); + } + + #[gpui::test] + async fn test_remove_root_succeeds_when_directory_already_gone(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + "/project", + json!({ + ".git": {}, + "src": { "main.rs": "fn main() {}" } + }), + ) + .await; + fs.set_branch_name(Path::new("/project/.git"), Some("main")); + fs.insert_branches(Path::new("/project/.git"), &["main", "feature"]); + + fs.add_linked_worktree_for_repo( + Path::new("/project/.git"), + true, + GitWorktree { + path: PathBuf::from("/linked-worktree"), + ref_name: Some("refs/heads/feature".into()), + sha: "abc123".into(), + is_main: false, + is_bare: false, + }, + ) + .await; + + let project = Project::test( + fs.clone(), + [Path::new("/project"), Path::new("/linked-worktree")], + cx, + ) + .await; + project + .update(cx, |project, cx| project.git_scans_complete(cx)) + .await; + + let multi_workspace = + cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = multi_workspace + .read_with(cx, |mw, _cx| mw.workspace().clone()) + .unwrap(); + + cx.run_until_parked(); + + let root = workspace + .read_with(cx, |_workspace, cx| { + build_root_plan( + Path::new("/linked-worktree"), + std::slice::from_ref(&workspace), + cx, + ) + }) + .expect("should produce a root plan for the linked worktree"); + + // Manually remove the worktree directory from FakeFs before calling + // remove_root, simulating the directory being deleted externally. + fs.as_ref() + .remove_dir( + Path::new("/linked-worktree"), + fs::RemoveOptions { + recursive: true, + ignore_if_not_exists: false, + }, + ) + .await + .unwrap(); + assert!(!fs.as_ref().is_dir(Path::new("/linked-worktree")).await); + + // remove_root should still succeed — the std::fs::remove_dir_all + // handles NotFound, and git worktree remove handles a missing + // working tree directory. + let task = cx.update(|cx| cx.spawn(async move |cx| remove_root(root, cx).await)); + task.await + .expect("remove_root should succeed even when directory is already gone"); + } + + #[test] + fn test_remove_dir_all_deletes_real_directory() { + let tmp = TempDir::new().unwrap(); + let worktree_dir = tmp.path().join("linked-worktree"); + std::fs::create_dir_all(worktree_dir.join("src")).unwrap(); + std::fs::write(worktree_dir.join("src/main.rs"), "fn main() {}").unwrap(); + std::fs::write(worktree_dir.join("README.md"), "# Hello").unwrap(); + + assert!(worktree_dir.is_dir()); + + // This is the same pattern used in remove_root_after_worktree_removal. + match std::fs::remove_dir_all(&worktree_dir) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => panic!("unexpected error: {error}"), + } + + assert!( + !worktree_dir.exists(), + "worktree directory should be deleted" + ); + } + + #[test] + fn test_remove_dir_all_handles_not_found() { + let tmp = TempDir::new().unwrap(); + let nonexistent = tmp.path().join("does-not-exist"); + + assert!(!nonexistent.exists()); + + // Should not panic — NotFound is handled gracefully. + match std::fs::remove_dir_all(&nonexistent) { + Ok(()) => panic!("expected NotFound error"), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => panic!("unexpected error: {error}"), + } + } } diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index f857a53c6530b12f38559a62ed4a6ae02d39e5af..2b84de531e081f210208b09b2c3cb28ce7c3c666 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -1,3 +1,5 @@ +use std::path::Path; + use crate::{FakeFs, FakeFsEntry, Fs, RemoveOptions, RenameOptions}; use anyhow::{Context as _, Result, bail}; use collections::{HashMap, HashSet}; @@ -106,6 +108,28 @@ impl FakeGitRepository { } .boxed() } + + /// Scans `.git/worktrees/*/gitdir` to find the admin entry directory for a + /// worktree at the given checkout path. Used when the working tree directory + /// has already been deleted and we can't read its `.git` pointer file. + async fn find_worktree_entry_dir_by_path(&self, path: &Path) -> Option { + use futures::StreamExt; + + let worktrees_dir = self.common_dir_path.join("worktrees"); + let mut entries = self.fs.read_dir(&worktrees_dir).await.ok()?; + while let Some(Ok(entry_path)) = entries.next().await { + if let Ok(gitdir_content) = self.fs.load(&entry_path.join("gitdir")).await { + let worktree_path = PathBuf::from(gitdir_content.trim()) + .parent() + .map(PathBuf::from) + .unwrap_or_default(); + if worktree_path == path { + return Some(entry_path); + } + } + } + None + } } impl GitRepository for FakeGitRepository { @@ -688,24 +712,30 @@ impl GitRepository for FakeGitRepository { async move { executor.simulate_random_delay().await; - // Read the worktree's .git file to find its entry directory. + // Try to read the worktree's .git file to find its entry + // directory. If the working tree is already gone (e.g. the + // caller deleted it before asking git to clean up), fall back + // to scanning `.git/worktrees/*/gitdir` for a matching path, + // mirroring real git's behavior with `--force`. let dot_git_file = path.join(".git"); - let content = fs - .load(&dot_git_file) - .await - .with_context(|| format!("no worktree found at path: {}", path.display()))?; - let gitdir = content - .strip_prefix("gitdir:") - .context("invalid .git file in worktree")? - .trim(); - let worktree_entry_dir = PathBuf::from(gitdir); + let worktree_entry_dir = if let Ok(content) = fs.load(&dot_git_file).await { + let gitdir = content + .strip_prefix("gitdir:") + .context("invalid .git file in worktree")? + .trim(); + PathBuf::from(gitdir) + } else { + self.find_worktree_entry_dir_by_path(&path) + .await + .with_context(|| format!("no worktree found at path: {}", path.display()))? + }; - // Remove the worktree checkout directory. + // Remove the worktree checkout directory if it still exists. fs.remove_dir( &path, RemoveOptions { recursive: true, - ignore_if_not_exists: false, + ignore_if_not_exists: true, }, ) .await?;