diff --git a/crates/agent_ui/src/thread_worktree_archive.rs b/crates/agent_ui/src/thread_worktree_archive.rs index b510da96b4e..4baf76f1120 100644 --- a/crates/agent_ui/src/thread_worktree_archive.rs +++ b/crates/agent_ui/src/thread_worktree_archive.rs @@ -1,13 +1,16 @@ use std::{ + future::Future, path::{Path, PathBuf}, sync::Arc, }; use anyhow::{Context as _, Result, anyhow}; -use gpui::{App, AsyncApp, Entity, Task}; +use fs::{Fs, RemoveOptions, RenameOptions}; +use git::repository::NotAWorktreeError; +use gpui::{App, AppContext as _, AsyncApp, Entity, Task}; use project::{ LocalProjectFlags, Project, WorktreeId, - git_store::{Repository, resolve_git_worktree_to_main_repo, worktrees_directory_for_repo}, + git_store::{Repository, worktrees_directory_for_repo}, project_settings::ProjectSettings, }; use remote::{RemoteConnectionOptions, same_remote_connection_identity}; @@ -517,9 +520,10 @@ pub async fn persist_worktree_state(root: &RootPlan, cx: &mut AsyncApp) -> Resul .map_err(|_| anyhow!("update_ref canceled")) .and_then(|r| r) .with_context(|| format!("failed to create ref {ref_name} on main repo"))?; - // See note in `remove_root_after_worktree_removal`: this may be a live - // or temporary project; dropping only matters in the temporary case. - drop(_temp_project); + // `_temp_project` is held until the end of this scope so that the + // temporary project (when one was created by + // `find_or_create_repository`) stays alive while the repo runs git + // commands; the leading underscore already enforces end-of-scope drop. Ok(archived_worktree_id) } @@ -536,10 +540,9 @@ pub async fn rollback_persist(archived_worktree_id: i64, root: &RootPlan, cx: &m let ref_name = archived_worktree_ref_name(archived_worktree_id); let rx = main_repo.update(cx, |repo, _cx| repo.delete_ref(ref_name)); rx.await.ok().and_then(|r| r.log_err()); - // See note in `remove_root_after_worktree_removal`: this may be a - // live or temporary project; dropping only matters in the temporary - // case. - drop(_temp_project); + // `_temp_project` lives to end of this block so a temporary + // project (if `find_or_create_repository` made one) stays alive + // through the await above. } // Delete the DB record @@ -560,167 +563,722 @@ pub async fn rollback_persist(archived_worktree_id: i64, root: &RootPlan, cx: &m /// during archival since WIP commits are detached), switches to the branch, /// then uses [`restore_archive_checkpoint`] to reconstruct the staged/ /// unstaged state from the WIP commit trees. +/// +/// **Destructive**: the final step (`restore_archive_checkpoint`) clobbers the +/// working directory unconditionally via `git read-tree --reset -u`. If the +/// path has any pre-existing content (a non-empty directory, a file, or a +/// symlink) it is moved aside into a `zed-restore-backup-` sibling +/// directory before the rest of the destructive work runs. If a later +/// step fails, the backup is moved back over `worktree_path` so the user +/// does not lose their content. On success the backup directory is +/// deleted asynchronously so a multi-GB cleanup does not block the +/// caller. pub async fn restore_worktree_via_git( row: &ArchivedGitWorktree, remote_connection: Option<&RemoteConnectionOptions>, cx: &mut AsyncApp, ) -> Result { + if remote_connection.is_some() { + anyhow::bail!("restoring archived worktrees on remote machines is not yet supported"); + } + let app_state = current_app_state(cx).context("no app state available")?; + let fs = app_state.fs.as_ref(); + let worktree_path = &row.worktree_path; + let (main_repo, _temp_project) = find_or_create_repository(&row.main_repo_path, remote_connection, cx).await?; - let worktree_path = &row.worktree_path; - let app_state = current_app_state(cx).context("no app state available")?; - let already_exists = app_state.fs.metadata(worktree_path).await?.is_some(); + // Always restore by recreating the worktree from scratch. This collapses + // every messy intermediate state into one clean flow: + // + // - Path missing or empty dir, no registration: plain add. + // - Path missing or empty dir, stale registration: scoped remove → add. + // - Path present with content, no registration + // (the original Windows file-lock bug): rename → add. + // - Path present with content, stale registration: rename → scoped remove → add. + // - Path present as a fully valid worktree: rename → scoped remove → add. + // + // Any pre-existing content at `worktree_path` is moved aside into a + // backup directory rather than deleted up-front. If any destructive + // step below fails, [`rollback_backup`] restores the backup over + // `worktree_path` so the user does not lose content they confirmed + // they wanted overwritten only on the assumption that the archived + // state would replace it. On success the backup is deleted at the end + // of this function. + // + // An empty directory at `worktree_path` is left in place: `git worktree + // add` is happy to adopt one and we don't want to round-trip through a + // backup for it. + let session = BackupSession::take(fs, worktree_path).await?; - let created_new_worktree = if already_exists { - let is_git_worktree = - resolve_git_worktree_to_main_repo(app_state.fs.as_ref(), worktree_path) + // From here on, every destructive step either (a) calls + // `session.try_step(...)` so a failure rolls the backup back, or + // (b) handles rollback inline via + // `session.rollback_with_annotation(...)` because it shares the `cx` + // borrow with its own cleanup. + + // Drop any stale registration in the main repo. Without this, the + // `git worktree add` below would fail with "already assigned but + // missing". + session + .try_step(async { + remove_worktree_registration_if_present(&main_repo, worktree_path, cx) .await - .is_some(); + .with_context(|| { + format!( + "failed to remove stale worktree registration for '{}'", + worktree_path.display() + ) + }) + }) + .await?; - if !is_git_worktree { - let rx = main_repo.update(cx, |repo, _cx| repo.repair_worktrees()); - rx.await - .map_err(|_| anyhow!("worktree repair was canceled"))? - .context("failed to repair worktrees")?; + // Recreate the worktree at the original commit. The branch still + // points here because archival used detached commits. + session + .try_step(async { + create_worktree_with_partial_cleanup( + fs, + &main_repo, + worktree_path, + &row.original_commit_hash, + cx, + ) + .await + }) + .await?; + + // Resolve the worktree's own `Repository` entity. Can't use + // `try_step` because the rollback path needs the same `cx` borrow + // the step itself uses; call into the session directly instead. + let wt_repo = match find_or_create_repository(worktree_path, remote_connection, cx).await { + Ok((repo, _temp_wt_project)) => repo, + Err(error) => { + remove_new_worktree_on_error(fs, &main_repo, worktree_path, cx).await; + return Err(session.rollback_with_annotation(error).await); } - false - } else { - // Create worktree at the original commit — the branch still points - // here because archival used detached commits. - let rx = main_repo.update(cx, |repo, _cx| { - repo.create_worktree_detached(worktree_path.clone(), row.original_commit_hash.clone()) - }); - rx.await - .map_err(|_| anyhow!("worktree creation was canceled"))? - .context("failed to create worktree")?; - true }; - let (wt_repo, _temp_wt_project) = - match find_or_create_repository(worktree_path, remote_connection, cx).await { - Ok(result) => result, - Err(error) => { - remove_new_worktree_on_error(created_new_worktree, &main_repo, worktree_path, cx) - .await; - return Err(error); - } - }; - + // Best-effort branch checkout. Non-fatal: if the branch was deleted + // or has moved since archival, we fall back to a detached checkout at + // the original commit so the worktree's filesystem state matches what + // was captured. if let Some(branch_name) = &row.branch_name { - // Attempt to check out the branch the worktree was previously on. - let checkout_result = wt_repo - .update(cx, |repo, _cx| repo.change_branch(branch_name.clone())) - .await; + checkout_branch_after_restore(&wt_repo, &main_repo, branch_name, row, cx).await; + } - match checkout_result.map_err(|e| anyhow!("{e}")).flatten() { - Ok(()) => { - // Branch checkout succeeded. Check whether the branch has moved since - // we archived the worktree, by comparing HEAD to the expected SHA. - let head_sha = wt_repo - .update(cx, |repo, _cx| repo.head_sha()) - .await - .map_err(|e| anyhow!("{e}")) - .and_then(|r| r); + // Reconstruct staged/unstaged state from the WIP commit trees. + // Inlined for the same `cx`-borrow reason as `wt_repo` above. + if let Err(error) = run_restore_checkpoint(&wt_repo, row, cx).await { + remove_new_worktree_on_error(fs, &main_repo, worktree_path, cx).await; + return Err(session.rollback_with_annotation(error).await); + } - match head_sha { - Ok(Some(sha)) if sha == row.original_commit_hash => { - // Branch still points at the original commit; we're all done! - } - Ok(Some(sha)) => { - // The branch has moved. We don't want to restore the worktree to - // a different filesystem state, so checkout the original commit - // in detached HEAD state. - log::info!( - "Branch '{branch_name}' has moved since archival (now at {sha}); \ - restoring worktree in detached HEAD at {}", + session.commit_async(app_state.fs.clone(), cx); + + Ok(worktree_path.clone()) +} + +/// Fixed leaf filename inside a backup directory where the original +/// `worktree_path` entry is renamed to. Kept as a single constant so the +/// rename target and the rollback rename source can never drift. +const BACKUP_ENTRY_NAME: &str = "worktree"; + +/// Pre-existing content at `worktree_path` that was moved aside before the +/// destructive parts of [`restore_worktree_via_git`] ran. If anything goes +/// wrong, [`rollback_backup`] uses this to put the user's content back. +struct Backup { + /// The backup directory holding the moved content, always created + /// as a sibling of `worktree_path` so the rename stays same-volume. + /// The moved entry lives at `dir/BACKUP_ENTRY_NAME` — see + /// [`Backup::target`]. + dir: PathBuf, +} + +impl Backup { + /// Path inside [`Backup::dir`] where the original `worktree_path` entry + /// was renamed to. Computed rather than stored so the two can't drift. + fn target(&self) -> PathBuf { + self.dir.join(BACKUP_ENTRY_NAME) + } +} + +/// Owns the entire "move aside any existing content, undo on failure, +/// clean up on success" lifecycle for a single call to +/// [`restore_worktree_via_git`]. Bundles the `fs` / `worktree_path` / +/// `backup` triple so individual restore steps don't have to thread them +/// through. +/// +/// Typical use: +/// +/// 1. `let session = BackupSession::take(fs, worktree_path).await?;` +/// 2. `session.try_step(some_destructive_step()).await?;` for each step +/// that doesn't need its own access to the borrow held by the step. +/// 3. For steps that share a `cx` borrow with their cleanup, call +/// [`Self::rollback_with_annotation`] directly on the error path. +/// 4. On success, `session.commit_async(fs_owned, cx)` schedules +/// background cleanup of the backup directory. Dropping the session +/// without `commit_async` will leak the backup directory — on +/// purpose, since reaching that code path means something panicked. +struct BackupSession<'a> { + fs: &'a dyn Fs, + worktree_path: &'a Path, + backup: Option, +} + +impl<'a> BackupSession<'a> { + /// Captures any pre-existing content at `worktree_path` into a + /// sibling backup directory and returns a session that can roll it + /// back. If the path was already missing or an empty directory, the + /// session holds no backup and rollback / commit become no-ops. + async fn take(fs: &'a dyn Fs, worktree_path: &'a Path) -> Result { + let backup = take_backup_if_needed(fs, worktree_path).await?; + Ok(Self { + fs, + worktree_path, + backup, + }) + } + + /// Awaits `step` and rolls back the backup on failure, returning the + /// original error annotated with the backup path if rollback strands + /// the user's content for manual recovery. + async fn try_step(&self, step: F) -> Result<()> + where + F: Future>, + { + match step.await { + Ok(()) => Ok(()), + Err(error) => Err(self.rollback_with_annotation(error).await), + } + } + + /// Rolls back the backup (if any) and returns the original `error` + /// annotated with the backup path if rollback couldn't put the + /// user's content back. Use directly when a destructive step shares + /// its `cx` borrow with the cleanup that has to follow the failure. + async fn rollback_with_annotation(&self, error: anyhow::Error) -> anyhow::Error { + match rollback_backup(self.fs, self.backup.as_ref(), self.worktree_path, &error).await { + Some(path) => annotate_with_stranded_backup(error, path), + None => error, + } + } + + /// Consumes the session and schedules cleanup of the (now-unused) + /// backup directory on a background task. Failures are logged but + /// not surfaced; the `zed-restore-backup-` naming makes any + /// orphans easy to spot manually. + fn commit_async(self, fs: Arc, cx: &mut AsyncApp) { + schedule_backup_cleanup(fs, self.backup, cx); + } +} + +/// Detects whether `worktree_path` currently holds content the caller has +/// agreed to overwrite (anything that isn't a missing path or empty +/// directory) and, if so, renames it aside into a freshly-created backup +/// directory. Returns `None` when there was nothing worth backing up. +/// +/// Used by [`BackupSession::take`]; not called directly from the restore +/// flow. +async fn take_backup_if_needed(fs: &dyn Fs, worktree_path: &Path) -> Result> { + if !worktree_path_has_content(fs, worktree_path).await? { + return Ok(None); + } + let backup_dir = create_backup_dir(fs, worktree_path).await?; + let backup = Backup { dir: backup_dir }; + let target = backup.target(); + // `rename` works for both directories and files, so we don't need to + // dispatch on the entry kind. A stray regular file or symlink at + // `worktree_path` is moved aside the same way as a directory. + fs.rename( + worktree_path, + &target, + RenameOptions { + overwrite: false, + ignore_if_exists: false, + create_parents: false, + }, + ) + .await + .with_context(|| { + format!( + "failed to move existing path '{}' to backup '{}'", + worktree_path.display(), + target.display() + ) + })?; + Ok(Some(backup)) +} + +/// Runs `git worktree add --detach` at `worktree_path` and, on failure, +/// cleans up any partial directory or stale registration so the caller's +/// rollback rename has a free target. Returns the underlying error when +/// creation fails. +async fn create_worktree_with_partial_cleanup( + fs: &dyn Fs, + main_repo: &Entity, + worktree_path: &Path, + original_commit_hash: &str, + cx: &mut AsyncApp, +) -> Result<()> { + let create_rx = main_repo.update(cx, |repo, _cx| { + repo.create_worktree_detached( + worktree_path.to_path_buf(), + original_commit_hash.to_string(), + ) + }); + let create_result = match create_rx.await { + Ok(result) => result.context("failed to create worktree"), + Err(_) => Err(anyhow!("worktree creation was canceled")), + }; + if let Err(error) = create_result { + // `create_worktree_detached` may have left a partial directory + // and/or a stale registration behind; `remove_new_worktree_on_error` + // clears both so the caller's rollback rename has somewhere to put + // the backup back. + remove_new_worktree_on_error(fs, main_repo, worktree_path, cx).await; + return Err(error); + } + Ok(()) +} + +/// Best-effort checkout of `branch_name` in the freshly restored worktree. +/// +/// * If the branch was deleted while the thread was archived, recreates it. +/// * If the branch has moved since archival, falls back to a detached +/// checkout at the original commit so the worktree's filesystem state +/// matches what was captured. +/// +/// All failures are logged; nothing here is fatal to the restore. +async fn checkout_branch_after_restore( + wt_repo: &Entity, + main_repo: &Entity, + branch_name: &str, + row: &ArchivedGitWorktree, + cx: &mut AsyncApp, +) { + let checkout_result = wt_repo + .update(cx, |repo, _cx| repo.change_branch(branch_name.to_string())) + .await; + + match checkout_result.map_err(|e| anyhow!("{e}")).flatten() { + Ok(()) => { + // Branch checkout succeeded. Check whether the branch has moved + // since archival by comparing HEAD to the expected SHA. + let head_sha = wt_repo + .update(cx, |repo, _cx| repo.head_sha()) + .await + .map_err(|e| anyhow!("{e}")) + .and_then(|r| r); + + match head_sha { + Ok(Some(sha)) if sha == row.original_commit_hash => { + // Branch still points at the original commit; done. + } + Ok(Some(sha)) => { + // Branch has moved — restore in detached HEAD at the + // original commit so the worktree state matches what + // was captured. + log::info!( + "Branch '{branch_name}' has moved since archival (now at {sha}); \ + restoring worktree in detached HEAD at {}", + row.original_commit_hash + ); + let detach_result = main_repo + .update(cx, |repo, _cx| { + repo.checkout_branch_in_worktree( + row.original_commit_hash.clone(), + row.worktree_path.clone(), + false, + ) + }) + .await; + if let Err(error) = detach_result.map_err(|e| anyhow!("{e}")).flatten() { + log::warn!( + "Failed to detach HEAD at {}: {error:#}", row.original_commit_hash ); - let detach_result = main_repo - .update(cx, |repo, _cx| { - repo.checkout_branch_in_worktree( - row.original_commit_hash.clone(), - row.worktree_path.clone(), - false, - ) - }) - .await; - - if let Err(error) = detach_result.map_err(|e| anyhow!("{e}")).flatten() { - log::warn!( - "Failed to detach HEAD at {}: {error:#}", - row.original_commit_hash - ); - } - } - Ok(None) => { - log::warn!( - "head_sha unexpectedly returned None after checking out \"{branch_name}\"; \ - proceeding in current HEAD state." - ); - } - Err(error) => { - log::warn!( - "Failed to read HEAD after checking out \"{branch_name}\": {error:#}" - ); } } - } - Err(checkout_error) => { - // We weren't able to check out the branch, most likely because it was deleted. - // This is fine; users will often delete old branches! We'll try to recreate it. - log::debug!( - "change_branch('{branch_name}') failed: {checkout_error:#}, trying create_branch" - ); - let create_result = wt_repo - .update(cx, |repo, _cx| { - repo.create_branch(branch_name.clone(), None) - }) - .await; - - if let Err(error) = create_result.map_err(|e| anyhow!("{e}")).flatten() { + Ok(None) => { log::warn!( - "Failed to create branch '{branch_name}': {error:#}; \ - restored worktree will be in detached HEAD state." + "head_sha unexpectedly returned None after checking out \"{branch_name}\"; \ + proceeding in current HEAD state." + ); + } + Err(error) => { + log::warn!( + "Failed to read HEAD after checking out \"{branch_name}\": {error:#}" ); } } } + Err(checkout_error) => { + // change_branch failed, most likely because the branch was + // deleted. Users often delete old branches; try to recreate. + log::debug!( + "change_branch('{branch_name}') failed: {checkout_error:#}, trying create_branch" + ); + let create_result = wt_repo + .update(cx, |repo, _cx| { + repo.create_branch(branch_name.to_string(), None) + }) + .await; + if let Err(error) = create_result.map_err(|e| anyhow!("{e}")).flatten() { + log::warn!( + "Failed to create branch '{branch_name}': {error:#}; \ + restored worktree will be in detached HEAD state." + ); + } + } } +} - // Restore the staged/unstaged state from the WIP commit trees. - // read-tree --reset -u applies the unstaged tree (including deletions) - // to the working directory, then a bare read-tree sets the index to - // the staged tree without touching the working directory. +/// Runs the staged/unstaged read-tree pair from the WIP commit trees to +/// reconstruct the worktree's pre-archive index + working-tree state. +/// `read-tree --reset -u` applies the unstaged tree (including deletions) +/// to the working directory, then a bare `read-tree` sets the index to +/// the staged tree without touching the working directory. +async fn run_restore_checkpoint( + wt_repo: &Entity, + row: &ArchivedGitWorktree, + cx: &mut AsyncApp, +) -> Result<()> { let restore_rx = wt_repo.update(cx, |repo, _cx| { repo.restore_archive_checkpoint( row.staged_commit_hash.clone(), row.unstaged_commit_hash.clone(), ) }); - if let Err(error) = restore_rx + restore_rx .await .map_err(|_| anyhow!("restore_archive_checkpoint canceled")) .and_then(|r| r) + .context("failed to restore archive checkpoint") +} + +/// Schedules cleanup of the (now-unused after success) backup directory +/// on a background task so the caller doesn't pay for a potentially +/// multi-GB cleanup synchronously. Failures are logged but not surfaced; +/// the `zed-restore-backup-` naming makes any orphans easy to spot +/// manually. +fn schedule_backup_cleanup(fs: Arc, backup: Option, cx: &mut AsyncApp) { + let Some(backup) = backup else { + return; + }; + cx.background_spawn(async move { + if let Err(error) = fs + .remove_dir( + &backup.dir, + RemoveOptions { + recursive: true, + ignore_if_not_exists: true, + }, + ) + .await + { + log::warn!( + "failed to clean up backup directory '{}' after successful restore: {error:#}", + backup.dir.display() + ); + } + }) + .detach(); +} + +/// If the main repo has a worktree registration pointing at +/// `worktree_path`, calls `git worktree remove --force` on it. Otherwise +/// returns `Ok(())`. +/// +/// `GitRepository::remove_worktree` already pre-checks the registry and +/// returns [`NotAWorktreeError`] when nothing is registered at the path, +/// so we just call it and treat that specific error as a no-op. (Note: the +/// RPC arm of `Repository::remove_worktree` does not preserve the error +/// type; restoring archived worktrees rejects remote connections at its +/// entry point, so we only ever reach this helper for local repos.) +async fn remove_worktree_registration_if_present( + main_repo: &Entity, + worktree_path: &Path, + cx: &mut AsyncApp, +) -> Result<()> { + let remove_rx = main_repo.update(cx, |repo, _cx| { + repo.remove_worktree(worktree_path.to_path_buf(), true) + }); + match remove_rx + .await + .map_err(|_| anyhow!("worktree remove was canceled")) + .and_then(|r| r) { - remove_new_worktree_on_error(created_new_worktree, &main_repo, worktree_path, cx).await; - return Err(error.context("failed to restore archive checkpoint")); + Ok(()) => Ok(()), + Err(error) => { + if error.downcast_ref::().is_some() { + log::debug!( + "no stale worktree registration to clean up for '{}'", + worktree_path.display() + ); + Ok(()) + } else { + Err(error) + } + } + } +} + +/// Creates the backup directory used by [`restore_worktree_via_git`]. +/// +/// Always places it as a sibling of `worktree_path` so the subsequent +/// rename stays on the same filesystem (atomic, and crucially does not +/// hit `EXDEV`/`ERROR_NOT_SAME_DEVICE`). Returns an error if a sibling +/// cannot be created — we used to fall back to `std::env::temp_dir()`, +/// but on Linux (tmpfs `/tmp`) and Windows (project on `D:` vs temp on +/// `C:`) that fallback would silently cross volumes, and `Fs::rename` +/// has no copy fallback for cross-device renames. Failing here, before +/// any destructive step in the restore runs, surfaces the real problem +/// (typically a read-only parent) instead of leaving the user with a +/// half-finished restore. +async fn create_backup_dir(fs: &dyn Fs, worktree_path: &Path) -> Result { + let parent = worktree_path.parent().with_context(|| { + format!( + "cannot create backup for worktree path '{}': path has no parent directory", + worktree_path.display() + ) + })?; + let sibling = parent.join(format!("zed-restore-backup-{}", uuid::Uuid::new_v4())); + fs.create_dir(&sibling).await.with_context(|| { + format!( + "failed to create backup directory '{}'; check that the parent directory is writable", + sibling.display() + ) + })?; + Ok(sibling) +} + +/// Restores the user's pre-existing content from a backup created by +/// [`restore_worktree_via_git`] back to `worktree_path`, then deletes the +/// now-empty backup directory. +/// +/// Returns `Some(path)` if the rollback could **not** put the user's +/// content back (e.g. unexpected new content at `worktree_path`, or the +/// rename itself failed) and the content remains stranded at that path +/// for manual recovery. Returns `None` if the rollback succeeded (or +/// there was no backup to roll back). Callers should weave the returned +/// path into the user-facing error so the toast can tell the user where +/// their files are. +/// +/// On any rollback failure we also log loudly with the original error and +/// the backup path. The original `restore_worktree_via_git` error is the +/// user-visible cause; the rollback error is intentionally not propagated. +#[must_use = "if rollback strands the backup, callers must surface the path to the user"] +async fn rollback_backup( + fs: &dyn Fs, + backup: Option<&Backup>, + worktree_path: &Path, + original_error: &anyhow::Error, +) -> Option { + let backup = backup?; + let target = backup.target(); + if let Ok(Some(metadata)) = fs.metadata(worktree_path).await { + // Treat symlinks as content even if they happen to resolve to a + // directory: the worktree path was guaranteed to be either + // missing or an empty directory when we moved the original + // aside, so a symlink here is unexpected and could point + // anywhere. Mirrors `worktree_path_has_content` so the + // preflight and rollback agree on what counts as content. + let emptiness = if metadata.is_symlink { + DirEmptiness::HasContent + } else if metadata.is_dir { + classify_dir_emptiness(fs, worktree_path).await + } else { + // Anything that's not a directory at the worktree path counts + // as unexpected content for rollback purposes (the path was + // empty / missing when we moved the original aside). + DirEmptiness::HasContent + }; + match emptiness { + DirEmptiness::Empty => { + if let Err(clear_error) = fs + .remove_dir( + worktree_path, + RemoveOptions { + recursive: false, + ignore_if_not_exists: true, + }, + ) + .await + { + log::warn!( + "failed to clear empty '{}' before rollback rename: {clear_error:#}", + worktree_path.display() + ); + } + } + DirEmptiness::HasContent => { + log::error!( + "cannot rollback: '{}' has unexpected content after restore failure; \ + original error: {original_error:#}; \ + user's pre-existing content remains at '{}' for manual recovery", + worktree_path.display(), + target.display(), + ); + return Some(target); + } + DirEmptiness::Unknown(read_error) => { + // We couldn't tell if the directory is empty (e.g. a + // transient permission error reading it). Don't blindly + // delete it — it might hold user content the partial + // restore created. Surface this as its own log line so the + // distinction from "non-empty" is clear when triaging. + log::error!( + "cannot rollback: failed to read '{}' to check whether it's empty: {read_error:#}; \ + original error: {original_error:#}; \ + user's pre-existing content remains at '{}' for manual recovery", + worktree_path.display(), + target.display(), + ); + return Some(target); + } + } + } + if let Err(rollback_error) = fs + .rename( + &target, + worktree_path, + RenameOptions { + overwrite: false, + ignore_if_exists: false, + create_parents: false, + }, + ) + .await + { + log::error!( + "failed to restore backup '{}' to '{}' after restore error: {rollback_error:#}; \ + original restore error: {original_error:#}; \ + user content remains at '{}' for manual recovery", + target.display(), + worktree_path.display(), + target.display(), + ); + return Some(target); + } + if let Err(cleanup_error) = fs + .remove_dir( + &backup.dir, + RemoveOptions { + recursive: true, + ignore_if_not_exists: true, + }, + ) + .await + { + log::warn!( + "failed to clean up empty backup directory '{}' after rollback: {cleanup_error:#}", + backup.dir.display() + ); + } + None +} + +/// Wraps the original restore error with a message pointing the user at +/// `stranded_path` for manual recovery. Used by callers of +/// [`rollback_backup`] to surface the backup path through the existing +/// `anyhow` error chain (which the sidebar toast renders verbatim). +fn annotate_with_stranded_backup(error: anyhow::Error, stranded_path: PathBuf) -> anyhow::Error { + error.context(format!( + "Your pre-existing files have been preserved at '{}' for manual recovery.", + stranded_path.display() + )) +} + +/// Result of attempting to determine whether a directory is empty for +/// rollback purposes. Distinguishes "definitely has content" from "could +/// not tell", which require different handling: the former means there's +/// new content we shouldn't blow away; the latter is a read failure where +/// the safer choice is also to leave the directory alone. +enum DirEmptiness { + Empty, + HasContent, + Unknown(anyhow::Error), +} + +async fn classify_dir_emptiness(fs: &dyn Fs, path: &Path) -> DirEmptiness { + use futures::stream::StreamExt as _; + + match fs.read_dir(path).await { + Ok(mut entries) => { + if entries.next().await.is_some() { + DirEmptiness::HasContent + } else { + DirEmptiness::Empty + } + } + Err(error) => DirEmptiness::Unknown(error), + } +} + +/// Returns whether the worktree path has any content that a restore would +/// destroy. A path that doesn't exist or that is an empty directory has no +/// content; anything else (a non-empty directory, or a file at this path) +/// counts as content. +async fn worktree_path_has_content(fs: &dyn Fs, path: &Path) -> Result { + use futures::stream::StreamExt; + + let Some(metadata) = fs.metadata(path).await? else { + return Ok(false); + }; + + if metadata.is_symlink { + return Ok(true); } - Ok(worktree_path.clone()) + if !metadata.is_dir { + return Ok(true); + } + + let mut entries = fs.read_dir(path).await?; + Ok(entries.next().await.is_some()) } async fn remove_new_worktree_on_error( - created_new_worktree: bool, + fs: &dyn Fs, main_repo: &Entity, - worktree_path: &PathBuf, + worktree_path: &Path, cx: &mut AsyncApp, ) { - if created_new_worktree { - let rx = main_repo.update(cx, |repo, _cx| { - repo.remove_worktree(worktree_path.clone(), true) - }); - rx.await.ok().and_then(|r| r.log_err()); + // Use the registration check rather than calling `remove_worktree` + // directly so a rollback at a point where no registration exists + // (e.g. when `create_worktree_detached` failed before git could + // register the path) doesn't log a misleading `NotAWorktreeError`. + if let Err(error) = remove_worktree_registration_if_present(main_repo, worktree_path, cx).await + { + log::warn!( + "failed to clean up worktree registration for '{}' during rollback: {error:#}", + worktree_path.display() + ); + } + + // `git worktree add` creates the directory before registering it, so a + // failure between mkdir and registration can leave a partial directory + // with no registry entry. The registration cleanup above is a no-op in + // that case; without this fs-level cleanup, the partial directory + // would strand the rollback's backup rename (`fs.rename` with + // `overwrite: false` fails if the target path exists), leaving the + // user's pre-existing content in the `zed-restore-backup-` + // directory and forcing manual recovery. + if let Err(error) = fs + .remove_dir( + worktree_path, + RemoveOptions { + recursive: true, + ignore_if_not_exists: true, + }, + ) + .await + { + log::warn!( + "failed to remove partial worktree directory '{}' during rollback: {error:#}", + worktree_path.display() + ); } } @@ -742,10 +1300,9 @@ pub async fn cleanup_archived_worktree_record( Ok(Err(error)) => log::warn!("Failed to delete archive ref: {error}"), Err(_) => log::warn!("Archive ref deletion was canceled"), } - // See note in `remove_root_after_worktree_removal`: this may be a - // live or temporary project; dropping only matters in the temporary - // case. - drop(_temp_project); + // `_temp_project` lives to end of this block so a temporary + // project (if `find_or_create_repository` made one) stays alive + // through the await above. } // Delete the DB records @@ -842,10 +1399,11 @@ fn current_app_state(cx: &mut AsyncApp) -> Option> { #[cfg(test)] mod tests { use super::*; - use fs::{FakeFs, Fs as _}; + use fs::FakeFs; use git::repository::Worktree as GitWorktree; use gpui::{BorrowAppContext, TestAppContext}; use project::Project; + use remote::SshConnectionOptions; use serde_json::json; use settings::SettingsStore; use workspace::MultiWorkspace; @@ -1434,4 +1992,187 @@ mod tests { "rollback should have re-added the worktree to the project" ); } + + /// Case B (the original bug): the worktree directory exists with leftover + /// content (e.g. files that Windows couldn't fully delete during archival + /// because they were locked by another process), but git has no + /// registration for it. The pre-flight check must report content so the + /// caller can prompt before the leftover files get clobbered. + #[gpui::test] + async fn test_has_content_leftover_dir_no_registration(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + + // Worktree directory has leftover content but no .git linkage and no + // entry in any main repo's .git/worktrees/. + fs.insert_tree( + "/wt-orphaned", + json!({ + "leftover.txt": "important user data", + }), + ) + .await; + + let has_content = worktree_path_has_content(fs.as_ref(), Path::new("/wt-orphaned")).await; + + assert_eq!( + has_content.unwrap(), + true, + "orphaned dir from a partial Windows archive must report content" + ); + assert!( + fs.is_file(Path::new("/wt-orphaned/leftover.txt")).await, + "the check must not touch any files" + ); + } + + /// Case D: the worktree directory exists with content but the `.git` file + /// in the worktree itself is missing. The restore would still clobber + /// any user files on disk, so we must report content here. + #[gpui::test] + async fn test_has_content_dir_with_broken_dot_git(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + + // /wt-broken has files but no `.git` file — the linkage is broken. + fs.insert_tree( + "/wt-broken", + json!({ + "src": { "lib.rs": "// existing user file" }, + }), + ) + .await; + + let has_content = worktree_path_has_content(fs.as_ref(), Path::new("/wt-broken")).await; + + assert_eq!( + has_content.unwrap(), + true, + "a directory with broken git linkage but real files must report content" + ); + } + + /// Case E: the worktree directory is fully valid — `.git` file points back + /// to the main repo. Even so, the restore will overwrite any uncommitted + /// work the user has in there via `git read-tree --reset -u`, so we must + /// still report content. + #[gpui::test] + async fn test_has_content_fully_valid_worktree(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + + fs.insert_tree( + "/wt-valid", + json!({ + ".git": "gitdir: /project/.git/worktrees/feature", + "src": { "lib.rs": "// uncommitted local work" }, + }), + ) + .await; + + let has_content = worktree_path_has_content(fs.as_ref(), Path::new("/wt-valid")).await; + + assert_eq!( + has_content.unwrap(), + true, + "a valid worktree with uncommitted work must report content \ + (read-tree --reset -u would clobber it)" + ); + } + + /// Case A: nothing exists at the worktree path. The check must report + /// no content — there's nothing to lose. + #[gpui::test] + async fn test_has_content_when_path_missing(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + + assert!( + fs.metadata(Path::new("/wt-missing")) + .await + .unwrap() + .is_none(), + "precondition: worktree path must not exist" + ); + + let has_content = worktree_path_has_content(fs.as_ref(), Path::new("/wt-missing")).await; + + assert_eq!( + has_content.unwrap(), + false, + "missing dir must not report content" + ); + } + + /// An empty directory at the worktree path has no content to lose. + #[gpui::test] + async fn test_has_content_for_empty_dir(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + + fs.create_dir(Path::new("/wt-empty")).await.unwrap(); + assert!( + fs.is_dir(Path::new("/wt-empty")).await, + "precondition: empty dir must exist" + ); + + let has_content = worktree_path_has_content(fs.as_ref(), Path::new("/wt-empty")).await; + + assert_eq!( + has_content.unwrap(), + false, + "empty dir must not report content" + ); + } + + #[gpui::test] + async fn test_restore_rejects_remote_connection(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + + fs.insert_tree( + "/project", + json!({ + ".git": { + "worktrees": {}, + }, + "src": {}, + }), + ) + .await; + cx.update(|cx| ::set_global(fs.clone(), cx)); + + let remote_connection = RemoteConnectionOptions::Ssh(SshConnectionOptions { + host: "test-host".into(), + ..Default::default() + }); + + let restore_row = ArchivedGitWorktree { + id: 1, + worktree_path: PathBuf::from("/remote/worktree"), + main_repo_path: PathBuf::from("/remote/project"), + branch_name: Some("feature".to_string()), + staged_commit_hash: "abc123".to_string(), + unstaged_commit_hash: "def456".to_string(), + original_commit_hash: "789abc".to_string(), + }; + + let restore_result = cx + .spawn(|mut cx| { + let remote_connection = remote_connection.clone(); + async move { + restore_worktree_via_git(&restore_row, Some(&remote_connection), &mut cx).await + } + }) + .await; + + assert!( + restore_result.is_err(), + "restore_worktree_via_git should reject remote connections" + ); + assert!( + format!("{:#}", restore_result.unwrap_err()).contains("remote machines"), + "error message should mention remote machines" + ); + } } diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index 13daed845c5..12c06263f1f 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -13,8 +13,8 @@ use git::{ repository::{ AskPassDelegate, Branch, CommitData, CommitDataReader, CommitDetails, CommitOptions, CreateWorktreeTarget, FetchOptions, GRAPH_CHUNK_SIZE, GitRepository, - GitRepositoryCheckpoint, InitialGraphCommitData, LogOrder, LogSource, PushOptions, RefEdit, - Remote, RepoPath, ResetMode, SearchCommitArgs, Worktree, + GitRepositoryCheckpoint, InitialGraphCommitData, LogOrder, LogSource, NotAWorktreeError, + PushOptions, RefEdit, Remote, RepoPath, ResetMode, SearchCommitArgs, Worktree, }, stash::GitStash, status::{ @@ -769,9 +769,17 @@ impl GitRepository for FakeGitRepository { .trim(); PathBuf::from(gitdir) } else { - self.find_worktree_entry_dir_by_path(&path) - .await - .with_context(|| format!("no worktree found at path: {}", path.display()))? + match self.find_worktree_entry_dir_by_path(&path).await { + Some(dir) => dir, + None => { + // Match the real backend's behavior: surface a + // structured `NotAWorktreeError` so callers can + // treat "nothing to remove" as success. + return Err(anyhow::Error::new(NotAWorktreeError { + path: path.clone(), + })); + } + } }; // Remove the worktree checkout directory if it still exists. diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index ae3172fd128..a7e060968da 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -823,6 +823,13 @@ pub trait GitRepository: Send + Sync { create: bool, ) -> BoxFuture<'_, Result<()>>; + /// Removes a git worktree. + /// + /// When the path has no entry in the main repo's worktree registry + /// (because it was never registered, or its registration was already + /// pruned), implementations return [`NotAWorktreeError`] as the error + /// payload so callers can downcast and treat the "nothing to remove" + /// case as a no-op. fn remove_worktree(&self, path: PathBuf, force: bool) -> BoxFuture<'_, Result<()>>; fn rename_worktree(&self, old_path: PathBuf, new_path: PathBuf) -> BoxFuture<'_, Result<()>>; @@ -1930,6 +1937,35 @@ impl GitRepository for RealGitRepository { self.executor .spawn(async move { + // Pre-check the worktree registry directly rather than + // matching `git worktree remove`'s stderr. Git localizes its + // error messages (`LC_MESSAGES`), and Zed makes no attempt to + // pin the locale on its git invocations, so substring-matching + // an English phrase would silently miss the "not registered" + // case on non-English machines. + let list_output = git + .build_command(&["worktree", "list", "--porcelain"]) + .output() + .await?; + if !list_output.status.success() { + let stderr = String::from_utf8_lossy(&list_output.stderr); + anyhow::bail!("git worktree list failed: {stderr}"); + } + let stdout = String::from_utf8_lossy(&list_output.stdout); + let worktrees = parse_worktrees_from_str(&stdout, None); + // Cheap literal-equality pass first so the common case + // (caller path matches the registered form byte-for-byte) + // doesn't pay for an `fs::canonicalize` per registered + // worktree. Only fall back to the symlink-resolving + // comparison when literal equality misses. + let matches_registration = worktrees.iter().any(|wt| wt.path == path) + || worktrees + .iter() + .any(|wt| util::paths::paths_resolve_to_same_location(&wt.path, &path)); + if !matches_registration { + return Err(anyhow::Error::new(NotAWorktreeError { path })); + } + let mut args: Vec = vec!["worktree".into(), "remove".into()]; if force { args.push("--force".into()); @@ -3451,6 +3487,17 @@ struct GitBinaryCommandError { status: ExitStatus, } +/// Returned by [`GitRepository::remove_worktree`] implementations when the +/// target path is not a registered worktree (e.g. its registration was +/// already pruned, or it was never `git worktree add`-ed). Callers that +/// want to treat "nothing to remove" as success can match on this with +/// `error.downcast_ref::()`. +#[derive(Error, Debug)] +#[error("'{}' is not a registered git worktree", path.display())] +pub struct NotAWorktreeError { + pub path: PathBuf, +} + async fn run_git_command( env: Arc>, ask_pass: AskPassDelegate, @@ -4450,6 +4497,133 @@ mod tests { assert!(!worktree_path.exists()); } + #[gpui::test] + async fn test_remove_worktree_returns_not_a_worktree_error_for_unregistered_path( + cx: &mut TestAppContext, + ) { + disable_git_global_config(); + cx.executor().allow_parking(); + + let temp_dir = tempfile::tempdir().unwrap(); + let repo_dir = temp_dir.path().join("repo"); + git2::Repository::init(&repo_dir).unwrap(); + + let repo = RealGitRepository::new( + &repo_dir.join(".git"), + None, + Some("git".into()), + cx.executor(), + ) + .unwrap(); + + smol::fs::write(repo_dir.join("file.txt"), "content") + .await + .unwrap(); + repo.stage_paths(vec![repo_path("file.txt")], Arc::new(HashMap::default())) + .await + .unwrap(); + repo.commit( + "Initial commit".into(), + None, + CommitOptions::default(), + AskPassDelegate::new(&mut cx.to_async(), |_, _, _| {}), + Arc::new(checkpoint_author_envs()), + ) + .await + .unwrap(); + + let bogus_path = temp_dir.path().join("never-registered"); + let error = repo + .remove_worktree(bogus_path.clone(), true) + .await + .expect_err("removing a path with no worktree registration must fail"); + + assert!( + error + .downcast_ref::() + .is_some_and(|e| e.path == bogus_path), + "error must downcast to NotAWorktreeError with the requested path, got: {error:#}" + ); + } + + /// Regression: a worktree whose directory has been deleted from disk + /// must still be removable when the caller passes a path that doesn't + /// canonicalize the same way git's registry does. Naive + /// `std::fs::canonicalize` on the caller-passed path returns `Err` + /// (path missing), so the literal/path-equality check would miss the + /// match on systems where `temp_dir()` and its canonical form differ + /// (e.g. macOS, where `/var` is a symlink to `/private/var`). We + /// instead canonicalize the longest existing ancestor and compare + /// that, which must succeed here. + #[gpui::test] + async fn test_remove_worktree_succeeds_when_directory_was_deleted(cx: &mut TestAppContext) { + disable_git_global_config(); + cx.executor().allow_parking(); + + let temp_dir = tempfile::tempdir().unwrap(); + let repo_dir = temp_dir.path().join("repo"); + let worktrees_dir = temp_dir.path().join("worktrees"); + git2::Repository::init(&repo_dir).unwrap(); + + let repo = RealGitRepository::new( + &repo_dir.join(".git"), + None, + Some("git".into()), + cx.executor(), + ) + .unwrap(); + + smol::fs::write(repo_dir.join("file.txt"), "content") + .await + .unwrap(); + repo.stage_paths(vec![repo_path("file.txt")], Arc::new(HashMap::default())) + .await + .unwrap(); + repo.commit( + "Initial commit".into(), + None, + CommitOptions::default(), + AskPassDelegate::new(&mut cx.to_async(), |_, _, _| {}), + Arc::new(checkpoint_author_envs()), + ) + .await + .unwrap(); + + let worktree_path = worktrees_dir.join("to-be-deleted"); + repo.create_worktree( + CreateWorktreeTarget::NewBranch { + branch_name: "to-be-deleted".to_string(), + base_sha: Some("HEAD".to_string()), + }, + worktree_path.clone(), + ) + .await + .unwrap(); + assert!(worktree_path.exists(), "precondition: worktree dir exists"); + + // Simulate the original Windows file-locks scenario where the + // working directory is gone but the main-repo registration still + // points at it. + std::fs::remove_dir_all(&worktree_path).unwrap(); + assert!( + !worktree_path.exists(), + "precondition: worktree dir is gone" + ); + + // Must NOT return `NotAWorktreeError` — the registration is still + // present, and git can still clean it up via + // `git worktree remove --force`. + repo.remove_worktree(worktree_path.clone(), true) + .await + .expect("remove_worktree on a missing-but-registered path must succeed"); + + let worktrees = repo.worktrees().await.unwrap(); + assert!( + worktrees.iter().all(|w| w.path != worktree_path), + "registration must be gone after remove_worktree" + ); + } + #[gpui::test] async fn test_rename_worktree(cx: &mut TestAppContext) { disable_git_global_config(); diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 9f97f829b0c..058f5022a1c 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -7140,6 +7140,21 @@ impl Repository { ) } + /// Removes a worktree by path. + /// + /// Note on error shape: the local backend translates git's "path is + /// not a working tree" failure into a structured + /// [`git::repository::NotAWorktreeError`] (see + /// [`git::repository::GitRepository::remove_worktree`]). The remote arm + /// here does **not** preserve that type — the RPC boundary collapses + /// the error into a generic `anyhow::Error` whose `Display` carries + /// the original message. Callers that need to react to the "nothing to + /// remove" case structurally must either route through the local + /// backend or pre-check with `Self::worktrees()`. As of writing the + /// only such caller (archived-thread restore in `agent_ui`) takes the + /// pre-check path and rejects remote restores at its entry point, so + /// this asymmetry has no live consumer; document it here so the next + /// caller doesn't get surprised. pub fn remove_worktree(&mut self, path: PathBuf, force: bool) -> oneshot::Receiver> { let id = self.id; let repository_anchor_path: Arc = self diff --git a/crates/sidebar/src/sidebar.rs b/crates/sidebar/src/sidebar.rs index 877b37c59e2..c080bec5462 100644 --- a/crates/sidebar/src/sidebar.rs +++ b/crates/sidebar/src/sidebar.rs @@ -24,9 +24,10 @@ use feature_flags::{ AgentThreadWorktreeLabel, AgentThreadWorktreeLabelFlag, FeatureFlag, FeatureFlagAppExt as _, }; use gpui::{ - Action as _, AnyElement, App, ClickEvent, Context, DismissEvent, Entity, EntityId, FocusHandle, - Focusable, KeyContext, ListState, Modifiers, Pixels, Render, SharedString, Task, TaskExt, - WeakEntity, Window, WindowHandle, linear_color_stop, linear_gradient, list, prelude::*, px, + Action as _, AnyElement, App, AsyncApp, ClickEvent, Context, DismissEvent, Entity, EntityId, + FocusHandle, Focusable, KeyContext, ListState, Modifiers, Pixels, Render, SharedString, Task, + TaskExt, WeakEntity, Window, WindowHandle, linear_color_stop, linear_gradient, list, + prelude::*, px, }; use itertools::Itertools; use menu::{ @@ -432,6 +433,50 @@ fn workspace_contains_worktree_path( .any(|worktree| worktree.read(cx).abs_path().as_ref() == worktree_path) } +/// Commits the DB-visible state changes for a successful restore: +/// rewrites worktree paths on the thread metadata, unarchives the +/// thread, and returns the updated metadata. +/// +/// Uses [`AsyncApp`] (not the window-bound context) so this commit +/// succeeds even if the user closed the window between the destructive +/// restore pass and now. That ordering matters for correctness: the +/// on-disk worktrees are already restored at this point, and if the DB +/// is left out-of-sync the user would re-open Zed to a thread marked +/// archived whose worktrees are physically present, and clicking +/// Restore would prompt them to overwrite their own freshly-restored +/// files. +/// +/// Returns `None` when the thread has been removed from the store mid- +/// restore (e.g. another window deleted it); the caller distinguishes +/// this case so the spinner gets cleared and the user gets a log line. +async fn commit_db_state_after_restore( + store: &Entity, + thread_id: ThreadId, + path_replacements: &[(PathBuf, PathBuf)], + cx: &mut AsyncApp, +) -> Option { + if path_replacements.is_empty() { + return None; + } + // `AsyncApp::update` panics if the App is fully dropped (process + // shutdown); for our purposes that's fine, since at that point the + // task is being torn down with the rest of the process. + cx.update(|cx| { + store.update(cx, |store, cx| { + store.update_restored_worktree_paths(thread_id, path_replacements, cx); + }); + }); + let updated_metadata = cx.update(|cx| store.read(cx).entry(thread_id).cloned()); + if updated_metadata.is_some() { + cx.update(|cx| { + store.update(cx, |store, cx| { + store.unarchive(thread_id, cx); + }); + }); + } + updated_metadata +} + #[derive(Clone)] struct WorkspaceMenuWorktreeLabel { icon: Option, @@ -2938,6 +2983,62 @@ impl Sidebar { }) } + fn finish_restore_ui( + &mut self, + thread_id: agent_ui::ThreadId, + weak_archive_view: &Option>, + cx: &mut Context, + ) { + self.restoring_tasks.remove(&thread_id); + if let Some(weak_archive_view) = weak_archive_view { + weak_archive_view + .update(cx, |view, cx| { + view.clear_restoring(&thread_id, cx); + }) + .ok(); + } + } + + /// Shows a toast in the current window's active workspace. Used for the + /// transient "restore" status messages so each callsite doesn't repeat + /// the `multi_workspace.upgrade()` ladder. Silently no-ops if the + /// multi-workspace has been torn down. + /// + /// Takes `&mut self` for consistency with [`Self::finish_restore_ui`] + /// and [`Self::fail_restore_with_toast`], even though the body only + /// reads `self`; sibling helpers are commonly called as a pair and + /// matching receivers keeps borrow patterns at the call sites uniform. + fn show_restore_toast( + &mut self, + notification_id: NotificationId, + message: impl Into, + cx: &mut Context, + ) { + let Some(multi_workspace) = self.multi_workspace.upgrade() else { + return; + }; + let workspace = multi_workspace.read(cx).workspace().clone(); + let message = message.into(); + workspace.update(cx, |workspace, cx| { + workspace.show_toast(Toast::new(notification_id, message).autohide(), cx); + }); + } + + /// Combined UI cleanup helper for the failure paths of + /// [`Self::open_thread_from_archive`]: clears the spinner / restoring + /// state and shows a toast with the given message. + fn fail_restore_with_toast( + &mut self, + thread_id: agent_ui::ThreadId, + weak_archive_view: &Option>, + notification_id: NotificationId, + message: impl Into, + cx: &mut Context, + ) { + self.finish_restore_ui(thread_id, weak_archive_view, cx); + self.show_restore_toast(notification_id, message, cx); + } + fn open_thread_from_archive( &mut self, metadata: ThreadMetadata, @@ -2945,6 +3046,15 @@ impl Sidebar { cx: &mut Context, ) { let thread_id = metadata.thread_id; + // Re-entry guard: if a restore is already in flight for this + // thread (e.g. the user double-clicked Restore while the + // previous task is still in flight), silently no-op so we don't + // spawn a second task that would race the first and leave the + // worktree in a half-restored state. + if self.restoring_tasks.contains_key(&thread_id) { + log::debug!("restore already in flight for {thread_id:?}; ignoring duplicate request"); + return; + } let weak_archive_view = match &self.view { SidebarView::Archive(view) => Some(view.downgrade()), _ => None, @@ -2990,10 +3100,29 @@ impl Sidebar { }; let path_list = metadata.folder_paths().clone(); - let restore_task = cx.spawn_in(window, async move |this, cx| { + // Mark this thread as restoring synchronously, BEFORE spawning, so the + // per-window re-entry guard at the top of this function rejects a + // double-click before the foreground executor has a chance to run the + // spawned body. The map's value type is `Task<()>` for historical + // reasons, but it's only ever read for presence — we insert a + // `Task::ready(())` placeholder here and detach the real task. + // + // The reason not to store the real task is that the spawned body's + // own error handlers call `finish_restore_ui` -> `restoring_tasks + // .remove(...)`. If the map held the real task, that remove would + // drop the task we're currently inside — which GPUI tolerates but + // is a fragile pattern. Detaching the real task makes the remove a + // no-op on a `Task::ready(())`, with no "drop the task we're inside" + // hazard. + self.restoring_tasks.insert(thread_id, Task::ready(())); + + cx.spawn_in(window, async move |this, cx| { let result: anyhow::Result<()> = async { let archived_worktrees = task.await?; + // Empty-archive activation has no destructive on-disk + // work to serialize against — skip the cross-window claim + // entirely and run the simple activation path. if archived_worktrees.is_empty() { this.update_in(cx, |this, window, cx| { this.restoring_tasks.remove(&thread_id); @@ -3035,51 +3164,37 @@ impl Sidebar { return anyhow::Ok(()); } + // Destructive pass: recreate each archived worktree. + // We do NOT delete each archived worktree's DB record in + // this loop. If we did, a later worktree's failure would + // strand the thread: the successful rows would have no + // archived metadata left to retry from, leaving on-disk + // worktrees orphaned and the thread permanently broken. + // Cleanup happens only after the DB-visible state is + // committed below. let mut path_replacements: Vec<(PathBuf, PathBuf)> = Vec::new(); for row in &archived_worktrees { match thread_worktree_archive::restore_worktree_via_git( row, metadata.remote_connection.as_ref(), - &mut *cx, + cx, ) .await { Ok(restored_path) => { - thread_worktree_archive::cleanup_archived_worktree_record( - row, - metadata.remote_connection.as_ref(), - &mut *cx, - ) - .await; path_replacements.push((row.worktree_path.clone(), restored_path)); } Err(error) => { log::error!("Failed to restore worktree: {error:#}"); this.update_in(cx, |this, _window, cx| { - this.restoring_tasks.remove(&thread_id); - if let Some(weak_archive_view) = &weak_archive_view { - weak_archive_view - .update(cx, |view, cx| { - view.clear_restoring(&thread_id, cx); - }) - .ok(); - } - - if let Some(multi_workspace) = this.multi_workspace.upgrade() { - let workspace = multi_workspace.read(cx).workspace().clone(); - workspace.update(cx, |workspace, cx| { - struct RestoreWorktreeErrorToast; - workspace.show_toast( - Toast::new( - NotificationId::unique::( - ), - format!("Failed to restore worktree: {error:#}"), - ) - .autohide(), - cx, - ); - }); - } + struct RestoreWorktreeErrorToast; + this.fail_restore_with_toast( + thread_id, + &weak_archive_view, + NotificationId::unique::(), + format!("Failed to restore worktree: {error:#}"), + cx, + ); }) .ok(); return anyhow::Ok(()); @@ -3087,29 +3202,22 @@ impl Sidebar { } } - if !path_replacements.is_empty() { - cx.update(|_window, cx| { - store.update(cx, |store, cx| { - store.update_restored_worktree_paths(thread_id, &path_replacements, cx); - }); - })?; + // Commit the DB-visible state through `AsyncApp` so it + // survives the user closing the window between the + // destructive pass and now (see `commit_db_state_after_ + // restore`'s docs). The activation `update_in` below is + // window-bound and best-effort; correctness lives in the + // commit above. + let updated_metadata = + commit_db_state_after_restore(&store, thread_id, &path_replacements, cx).await; - let updated_metadata = - cx.update(|_window, cx| store.read(cx).entry(thread_id).cloned())?; - - if let Some(updated_metadata) = updated_metadata { + match updated_metadata { + Some(updated_metadata) => { let new_paths = updated_metadata.folder_paths().clone(); let key = ProjectGroupKey::from_worktree_paths( &updated_metadata.worktree_paths, updated_metadata.remote_connection.clone(), ); - - cx.update(|_window, cx| { - store.update(cx, |store, cx| { - store.unarchive(updated_metadata.thread_id, cx); - }); - })?; - this.update_in(cx, |this, window, cx| { this.restoring_tasks.remove(&thread_id); this.open_workspace_and_activate_thread( @@ -3120,8 +3228,45 @@ impl Sidebar { cx, ); this.show_thread_list(window, cx); - })?; + }) + .ok(); } + None if !path_replacements.is_empty() => { + // The thread was removed from the store between + // the restore loop and this lookup (e.g. another + // window deleted it). The on-disk restore has + // already succeeded, but there's nothing left + // here to activate. Make sure the per-window + // "restoring..." state is cleared so the + // spinner doesn't get stuck. + log::warn!( + "Thread {thread_id:?} disappeared from the store mid-restore; \ + on-disk worktrees were restored but the thread can't be activated." + ); + this.update_in(cx, |this, _window, cx| { + this.finish_restore_ui(thread_id, &weak_archive_view, cx); + }) + .ok(); + } + None => {} + } + + // Finally, drop the archive records. This happens last so + // a failure or cancellation between the visible state + // changes above and this loop only leaves the user with + // some stale DB rows (recoverable by a later cleanup + // pass), not a thread stuck in an inconsistent + // "unarchived but archive rows missing" state. + // `cleanup_archived_worktree_record` swallows its own + // errors, so a cleanup failure leaks a DB row but doesn't + // break the thread. + for row in &archived_worktrees { + thread_worktree_archive::cleanup_archived_worktree_record( + row, + metadata.remote_connection.as_ref(), + cx, + ) + .await; } anyhow::Ok(()) @@ -3129,9 +3274,20 @@ impl Sidebar { .await; if let Err(error) = result { log::error!("{error:#}"); + this.update_in(cx, |this, _window, cx| { + struct RestoreFailedToast; + this.fail_restore_with_toast( + thread_id, + &weak_archive_view, + NotificationId::unique::(), + format!("Failed to restore thread: {error:#}"), + cx, + ); + }) + .ok(); } - }); - self.restoring_tasks.insert(thread_id, restore_task); + }) + .detach(); } fn expand_selected_entry( diff --git a/crates/sidebar/src/sidebar_tests.rs b/crates/sidebar/src/sidebar_tests.rs index 4817ab5ebc6..fd2d028f7dd 100644 --- a/crates/sidebar/src/sidebar_tests.rs +++ b/crates/sidebar/src/sidebar_tests.rs @@ -3921,6 +3921,131 @@ async fn init_test_project_with_git( (project, fs) } +/// Output of [`setup_archived_worktree_fixture`]. +/// +/// Holds the FakeFs and entity handles needed by tests that exercise the +/// archived-worktree restore flow: a `/project` main repo, a +/// `/wt-` linked worktree pointing back at it, a multi-workspace +/// containing both projects, and the staged/unstaged checkpoint hashes +/// captured from the worktree. +struct ArchivedWorktreeFixture { + fs: Arc, + worktree_path: PathBuf, + branch_name: String, + staged_hash: String, + unstaged_hash: String, +} + +impl ArchivedWorktreeFixture { + /// Builds an `ArchivedGitWorktree` row pointing at this fixture's + /// captured checkpoint. + fn archived_row(&self) -> agent_ui::thread_metadata_store::ArchivedGitWorktree { + agent_ui::thread_metadata_store::ArchivedGitWorktree { + id: 1, + worktree_path: self.worktree_path.clone(), + main_repo_path: PathBuf::from("/project"), + branch_name: Some(self.branch_name.clone()), + staged_commit_hash: self.staged_hash.clone(), + unstaged_commit_hash: self.unstaged_hash.clone(), + original_commit_hash: "original-sha".to_string(), + } + } +} + +/// Sets up the common FakeFs + multi-workspace + checkpoint state every +/// `restore_worktree_via_git` test needs. +/// +/// `extra_worktree_files` is merged into the worktree's `insert_tree` +/// payload alongside its `.git` gitfile, so tests can plant fixtures +/// without having to repeat the worktree-registration boilerplate. +/// Pass `serde_json::json!({})` if nothing extra is needed. +async fn setup_archived_worktree_fixture( + branch_name: &str, + extra_worktree_files: serde_json::Value, + cx: &mut TestAppContext, +) -> ArchivedWorktreeFixture { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + let worktree_path = PathBuf::from(format!("/wt-{branch_name}")); + + fs.insert_tree( + "/project", + serde_json::json!({ + ".git": { + "worktrees": { + branch_name: { + "commondir": "../../", + "HEAD": format!("ref: refs/heads/{branch_name}"), + }, + }, + }, + "src": {}, + }), + ) + .await; + + // Build the worktree directory tree: always includes the `.git` + // gitfile; merge in caller-provided extras (e.g. a `src/` subtree). + let mut worktree_tree = serde_json::json!({ + ".git": format!("gitdir: /project/.git/worktrees/{branch_name}"), + }); + if let (Some(base), Some(extras)) = ( + worktree_tree.as_object_mut(), + extra_worktree_files.as_object(), + ) { + for (key, value) in extras { + base.insert(key.clone(), value.clone()); + } + } + fs.insert_tree(&worktree_path, worktree_tree).await; + + fs.add_linked_worktree_for_repo( + Path::new("/project/.git"), + false, + git::repository::Worktree { + path: worktree_path.clone(), + ref_name: Some(format!("refs/heads/{branch_name}").into()), + sha: "original-sha".into(), + is_main: false, + is_bare: false, + }, + ) + .await; + cx.update(|cx| ::set_global(fs.clone(), cx)); + + let main_project = project::Project::test(fs.clone(), ["/project".as_ref()], cx).await; + let worktree_project = project::Project::test(fs.clone(), [worktree_path.as_path()], cx).await; + main_project + .update(cx, |p, cx| p.git_scans_complete(cx)) + .await; + worktree_project + .update(cx, |p, cx| p.git_scans_complete(cx)) + .await; + + let (multi_workspace, vcx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(main_project.clone(), window, cx)); + multi_workspace.update_in(vcx, |mw, window, cx| { + mw.test_add_workspace(worktree_project.clone(), window, cx) + }); + + let wt_repo = worktree_project.read_with(cx, |project, cx| { + project.repositories(cx).values().next().unwrap().clone() + }); + let (staged_hash, unstaged_hash) = cx + .update(|cx| wt_repo.update(cx, |repo, _| repo.create_archive_checkpoint())) + .await + .expect("create_archive_checkpoint task should not be canceled") + .expect("create_archive_checkpoint should succeed"); + + ArchivedWorktreeFixture { + fs, + worktree_path, + branch_name: branch_name.to_string(), + staged_hash, + unstaged_hash, + } +} + #[gpui::test] async fn test_search_matches_worktree_name(cx: &mut TestAppContext) { let (project, fs) = init_test_project_with_git("/project", cx).await; @@ -6254,6 +6379,409 @@ async fn test_restore_worktree_when_branch_does_not_exist(cx: &mut TestAppContex ); } +#[gpui::test] +async fn test_restore_worktree_cleans_up_backup_on_success(cx: &mut TestAppContext) { + // restore_worktree_via_git should move pre-existing content into a + // sibling backup directory before recreating the worktree, then delete + // that backup directory once the restore has completed successfully. + let fixture = + setup_archived_worktree_fixture("feature-success", serde_json::json!({ "src": {} }), cx) + .await; + let fs = fixture.fs.clone(); + + // Drop a sentinel file at the worktree path that is *not* part of the + // archive checkpoint, simulating user content that the user agreed to + // overwrite when they confirmed the restore prompt. + fs.write( + Path::new("/wt-feature-success/sentinel.txt"), + b"pre-existing user content", + ) + .await + .unwrap(); + + let row = fixture.archived_row(); + let result = cx + .spawn(|mut cx| async move { + agent_ui::thread_worktree_archive::restore_worktree_via_git(&row, None, &mut cx).await + }) + .await; + + assert!(result.is_ok(), "restore should succeed: {:?}", result.err()); + + // The success-path backup cleanup is scheduled via + // `cx.background_spawn(...).detach()`, so we have to drain the + // executor before asserting that the backup directory is gone. + // Without this, the assertion races the detached cleanup task. + cx.run_until_parked(); + + // No backup directory should remain in the parent of the worktree path. + let leftover_backup = fs.directories(true).into_iter().find(|path| { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with("zed-restore-backup-")) + }); + assert!( + leftover_backup.is_none(), + "backup directory should be deleted after a successful restore, found: {leftover_backup:?}" + ); + + // The restored worktree directory must exist (it was renamed away to a + // backup, then recreated by `git worktree add`). + assert!( + fs.metadata(Path::new("/wt-feature-success")) + .await + .unwrap() + .is_some(), + "worktree path should exist after a successful restore" + ); + + assert!( + fs.metadata(Path::new("/wt-feature-success/sentinel.txt")) + .await + .unwrap() + .is_none(), + "sentinel file from pre-existing content must not survive a successful restore" + ); +} + +#[gpui::test] +async fn test_restore_worktree_rolls_back_backup_on_failure(cx: &mut TestAppContext) { + // When restore_worktree_via_git fails partway through (here, because + // the archive checkpoint SHAs are bogus), it must restore the user's + // pre-existing content from the backup and not leave a backup + // directory lying around. + let fixture = + setup_archived_worktree_fixture("feature-fail", serde_json::json!({ "src": {} }), cx).await; + let fs = fixture.fs.clone(); + + // Drop a sentinel file representing pre-existing user content the + // user expected to be overwritten by the archived state on success. + fs.write( + Path::new("/wt-feature-fail/sentinel.txt"), + b"important user data", + ) + .await + .unwrap(); + + // Bogus checkpoint SHAs will cause `restore_archive_checkpoint` to + // fail, exercising the rollback path. + let bogus_sha = "0".repeat(40); + let row = agent_ui::thread_metadata_store::ArchivedGitWorktree { + staged_commit_hash: bogus_sha.clone(), + unstaged_commit_hash: bogus_sha, + ..fixture.archived_row() + }; + + let result = cx + .spawn(|mut cx| async move { + agent_ui::thread_worktree_archive::restore_worktree_via_git(&row, None, &mut cx).await + }) + .await; + + assert!( + result.is_err(), + "restore should fail when checkpoint SHAs are bogus", + ); + let error_msg = format!("{:#}", result.as_ref().unwrap_err()); + assert!( + error_msg.contains("failed to restore archive checkpoint"), + "error should indicate checkpoint failure, got: {error_msg}" + ); + + // The pre-existing sentinel must be back at the original path. + let sentinel_contents = fs + .load(Path::new("/wt-feature-fail/sentinel.txt")) + .await + .expect("sentinel file must be restored from the backup"); + assert_eq!( + sentinel_contents, "important user data", + "sentinel content must match what was on disk before the restore", + ); + + // No backup directory should remain in the parent of the worktree path. + let leftover_backup = fs.directories(true).into_iter().find(|path| { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with("zed-restore-backup-")) + }); + assert!( + leftover_backup.is_none(), + "backup directory should be cleaned up after a rollback, found: {leftover_backup:?}" + ); +} + +#[gpui::test] +async fn test_restore_worktree_round_trips_git_admin_state(cx: &mut TestAppContext) { + // End-to-end happy-path smoke test for `restore_worktree_via_git`: + // plant a checkpoint, tear the worktree directory down to simulate + // archival, then call the restore and confirm the captured tree is + // reinstated. + // + // FakeFs limitation: for a linked worktree opened via a `.git` gitfile, + // the fake's `create_archive_checkpoint` captures + // `repository_dir_path.parent()` (see `crates/fs/src/fake_git_repo.rs`), + // which resolves to `/.git/worktrees` — NOT the working + // tree at the linked worktree's path. As a result, `restore_archive_ + // checkpoint` only round-trips the contents of + // `/.git/worktrees`, not anything written to the working + // directory. We therefore plant our marker inside the captured + // location (the linked worktree's `worktrees/` registration + // directory) so we have something concrete to assert was actually + // moved through the checkpoint pipeline. We still write some user- + // facing files to the working tree before checkpointing (and assert + // the worktree path itself is recreated) to show that those don't + // crash the round-trip even though their contents aren't captured by + // the fake. + init_test(cx); + let fs = FakeFs::new(cx.executor()); + + fs.insert_tree( + "/project", + serde_json::json!({ + ".git": { + "worktrees": { + "feature-rt": { + "commondir": "../../", + "HEAD": "ref: refs/heads/feature-rt", + }, + }, + }, + "src": {}, + }), + ) + .await; + fs.insert_tree( + "/wt-feature-rt", + serde_json::json!({ + ".git": "gitdir: /project/.git/worktrees/feature-rt", + "src": {}, + }), + ) + .await; + fs.add_linked_worktree_for_repo( + Path::new("/project/.git"), + false, + git::repository::Worktree { + path: PathBuf::from("/wt-feature-rt"), + ref_name: Some("refs/heads/feature-rt".into()), + sha: "original-sha".into(), + is_main: false, + is_bare: false, + }, + ) + .await; + cx.update(|cx| ::set_global(fs.clone(), cx)); + + let main_project = project::Project::test(fs.clone(), ["/project".as_ref()], cx).await; + let worktree_project = + project::Project::test(fs.clone(), ["/wt-feature-rt".as_ref()], cx).await; + main_project + .update(cx, |p, cx| p.git_scans_complete(cx)) + .await; + worktree_project + .update(cx, |p, cx| p.git_scans_complete(cx)) + .await; + + let (multi_workspace, _cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(main_project.clone(), window, cx)); + multi_workspace.update_in(_cx, |mw, window, cx| { + mw.test_add_workspace(worktree_project.clone(), window, cx) + }); + + // Working-tree files. The fake doesn't round-trip these, but writing + // them and then asserting the restore still completes proves the + // function tolerates pre-checkpoint content in the working tree. + fs.write(Path::new("/wt-feature-rt/staged.txt"), b"staged contents") + .await + .expect("writing staged.txt should succeed"); + fs.write( + Path::new("/wt-feature-rt/src/nested.txt"), + b"nested contents", + ) + .await + .expect("writing src/nested.txt should succeed"); + + // Marker inside the captured location. This is the file we'll + // actually assert round-trips, since it sits inside what the fake's + // `create_archive_checkpoint` snapshots. + fs.write( + Path::new("/project/.git/worktrees/feature-rt/marker.txt"), + b"checkpoint marker", + ) + .await + .expect("writing checkpoint marker should succeed"); + + let wt_repo = worktree_project.read_with(cx, |project, cx| { + project.repositories(cx).values().next().unwrap().clone() + }); + let (staged_hash, unstaged_hash) = cx + .update(|cx| wt_repo.update(cx, |repo, _| repo.create_archive_checkpoint())) + .await + .expect("create_archive_checkpoint task should not be canceled") + .expect("create_archive_checkpoint should succeed"); + + // Simulate the archive having torn down the worktree directory. + fs.remove_dir( + Path::new("/wt-feature-rt"), + fs::RemoveOptions { + recursive: true, + ignore_if_not_exists: false, + }, + ) + .await + .expect("removing the worktree dir should succeed"); + + // Also clobber the captured marker so a successful restore is the + // only thing that could put it back. + fs.remove_file( + Path::new("/project/.git/worktrees/feature-rt/marker.txt"), + fs::RemoveOptions { + recursive: false, + ignore_if_not_exists: false, + }, + ) + .await + .expect("removing checkpoint marker should succeed"); + + let result = cx + .spawn(|mut cx| async move { + agent_ui::thread_worktree_archive::restore_worktree_via_git( + &agent_ui::thread_metadata_store::ArchivedGitWorktree { + id: 1, + worktree_path: PathBuf::from("/wt-feature-rt"), + main_repo_path: PathBuf::from("/project"), + branch_name: Some("feature-rt".to_string()), + staged_commit_hash: staged_hash, + unstaged_commit_hash: unstaged_hash, + original_commit_hash: "original-sha".to_string(), + }, + None, + &mut cx, + ) + .await + }) + .await; + + assert!( + result.is_ok(), + "restore should succeed for a clean round-trip: {:?}", + result.err() + ); + + // The marker we planted in the captured location must come back + // with its original contents — this is the actual round-trip + // assertion `restore_archive_checkpoint` is exercising in the fake. + let marker = fs + .load(Path::new("/project/.git/worktrees/feature-rt/marker.txt")) + .await + .expect("checkpoint marker must be restored"); + assert_eq!(marker, "checkpoint marker"); + + // The worktree directory itself must exist after restore (created + // by `create_worktree_detached`), and its `.git` gitfile must be + // present so the path is a usable linked worktree again. + assert!( + fs.metadata(Path::new("/wt-feature-rt")) + .await + .expect("metadata for worktree path must succeed") + .is_some(), + "worktree directory should exist after restore" + ); + assert!( + fs.metadata(Path::new("/wt-feature-rt/.git")) + .await + .expect("metadata for worktree .git must succeed") + .is_some(), + "worktree .git gitfile should exist after restore" + ); + + // Success-path backup cleanup runs in a detached background task; pump + // the executor so the assertion below doesn't race it. + cx.run_until_parked(); + + // No backup directory should remain on success. + let leftover_backup = fs.directories(true).into_iter().find(|path| { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with("zed-restore-backup-")) + }); + assert!( + leftover_backup.is_none(), + "backup directory should be cleaned up after a successful round-trip, found: {leftover_backup:?}" + ); +} + +#[gpui::test] +async fn test_restore_worktree_rolls_back_when_create_worktree_detached_fails( + cx: &mut TestAppContext, +) { + // Exercises the rollback path that runs when `create_worktree_detached` + // itself fails (the early failure branch in `restore_worktree_via_git`, + // before any branch / checkpoint operations have run). We force the + // failure with `FakeFs::set_create_worktree_error`, which makes the + // fake's `create_worktree` bail before producing any side effects. + let fixture = setup_archived_worktree_fixture( + "feature-create-fail", + serde_json::json!({ "src": {} }), + cx, + ) + .await; + let fs = fixture.fs.clone(); + + // Sentinel content that the rollback must put back. + fs.write( + Path::new("/wt-feature-create-fail/sentinel.txt"), + b"important user data", + ) + .await + .expect("writing sentinel.txt should succeed"); + + // Force `create_worktree_detached` to fail when the restore tries it. + fs.set_create_worktree_error( + Path::new("/project/.git"), + Some("simulated create_worktree failure".to_string()), + ); + + let row = fixture.archived_row(); + let result = cx + .spawn(|mut cx| async move { + agent_ui::thread_worktree_archive::restore_worktree_via_git(&row, None, &mut cx).await + }) + .await; + + assert!( + result.is_err(), + "restore should fail when create_worktree_detached fails", + ); + let error_msg = format!("{:#}", result.as_ref().unwrap_err()); + assert!( + error_msg.contains("failed to create worktree") + || error_msg.contains("simulated create_worktree failure"), + "error should indicate worktree creation failure, got: {error_msg}" + ); + + // The pre-existing sentinel must be back at the original path. + let sentinel_contents = fs + .load(Path::new("/wt-feature-create-fail/sentinel.txt")) + .await + .expect("sentinel file must be restored from the backup"); + assert_eq!( + sentinel_contents, "important user data", + "sentinel content must match what was on disk before the restore", + ); + + // No backup directory should remain anywhere on the fake fs. + let leftover_backup = fs.directories(true).into_iter().find(|path| { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with("zed-restore-backup-")) + }); + assert!( + leftover_backup.is_none(), + "backup directory should be cleaned up after a create_worktree rollback, found: {leftover_backup:?}" + ); +} + #[gpui::test] async fn test_restore_worktree_thread_uses_main_repo_project_group_key(cx: &mut TestAppContext) { // Activating an archived linked worktree thread whose directory has @@ -12030,3 +12558,51 @@ async fn test_cmd_click_project_header_returns_to_last_active_linked_worktree_wo linked-worktree workspace was the last-active one for the group" ); } + +#[gpui::test] +async fn test_restore_worktree_succeeds_when_path_is_missing(cx: &mut TestAppContext) { + // When the worktree path doesn't exist on disk, the destructive + // restore should proceed and recreate the worktree (there's nothing + // to back up). + let fixture = setup_archived_worktree_fixture("feature-empty", serde_json::json!({}), cx).await; + let fs = fixture.fs.clone(); + + fs.remove_dir( + Path::new("/wt-feature-empty"), + fs::RemoveOptions { + recursive: true, + ignore_if_not_exists: false, + }, + ) + .await + .unwrap(); + assert!( + fs.metadata(Path::new("/wt-feature-empty")) + .await + .unwrap() + .is_none(), + "precondition: worktree directory must not exist" + ); + + let restore_row = fixture.archived_row(); + let result = cx + .spawn(|mut cx| async move { + agent_ui::thread_worktree_archive::restore_worktree_via_git(&restore_row, None, &mut cx) + .await + }) + .await; + + assert!( + result.is_ok(), + "restore should succeed when the worktree path does not exist: {:?}", + result.err() + ); + + assert!( + fs.metadata(Path::new("/wt-feature-empty")) + .await + .unwrap() + .is_some(), + "worktree path should exist after a successful restore" + ); +} diff --git a/crates/util/src/paths.rs b/crates/util/src/paths.rs index 9f957f72f28..e709b94b72e 100644 --- a/crates/util/src/paths.rs +++ b/crates/util/src/paths.rs @@ -661,6 +661,68 @@ pub fn normalize_lexically(path: &Path) -> Result { Ok(lexical) } +/// Returns whether `a` and `b` refer to the same on-disk location. +/// +/// Tries literal equality first. Falls back to canonicalizing the longest +/// existing ancestor of each path and joining the unresolved tail, then +/// comparing. This handles two cases that a naive `std::fs::canonicalize` +/// of each side does not: +/// +/// - **The leaf path doesn't exist.** `canonicalize` requires every +/// component to exist; if the worktree directory was deleted but its +/// registration is still around, naive canonicalize returns `Err` and +/// the caller falls back to literal comparison, which then misses any +/// symlink-mediated equivalence. +/// - **The path traverses a symlinked ancestor.** Git canonicalizes +/// paths when storing them in its registries (notably resolving +/// macOS's `/var` -> `/private/var` symlink, or Windows junction +/// points), but callers passing paths into Zed APIs typically use the +/// un-resolved form. Walking up to the nearest existing ancestor and +/// canonicalizing *that* lets the comparison succeed even when the +/// leaf is gone. +/// +/// Does sync I/O via `std::fs::canonicalize`; call it from a spawned task +/// or background context, not on a foreground async path. +pub fn paths_resolve_to_same_location(a: &Path, b: &Path) -> bool { + if a == b { + return true; + } + let Some(canon_a) = canonicalize_with_existing_ancestor(a) else { + return false; + }; + let Some(canon_b) = canonicalize_with_existing_ancestor(b) else { + return false; + }; + canon_a == canon_b +} + +/// Canonicalizes the longest existing prefix of `path` and re-appends the +/// remaining (unresolved) tail. Returns `None` only if no ancestor of +/// `path` can be canonicalized, which on any working filesystem means +/// even the root is unreadable. +fn canonicalize_with_existing_ancestor(path: &Path) -> Option { + let mut tail = PathBuf::new(); + let mut current = path.to_path_buf(); + loop { + if let Ok(canon) = std::fs::canonicalize(¤t) { + return Some(if tail.as_os_str().is_empty() { + canon + } else { + canon.join(&tail) + }); + } + let Some(file_name) = current.file_name().map(|n| n.to_os_string()) else { + return None; + }; + let mut new_tail = PathBuf::from(&file_name); + new_tail.push(&tail); + tail = new_tail; + if !current.pop() { + return None; + } + } +} + /// A delimiter to use in `path_query:row_number:column_number` strings parsing. pub const FILE_ROW_COLUMN_DELIMITER: char = ':'; @@ -3559,4 +3621,64 @@ mod tests { Ok(PathBuf::from("C:\\Users\\file.txt")) ); } + + #[test] + fn test_paths_resolve_to_same_location_literal_equality() { + let p = Path::new("/some/nonexistent/path/that/will/never/exist/xyz"); + assert!(paths_resolve_to_same_location(p, p)); + } + + #[test] + fn test_paths_resolve_to_same_location_nonexistent_distinct() { + let a = Path::new("/totally/nonexistent/path/a"); + let b = Path::new("/totally/nonexistent/path/b"); + assert!(!paths_resolve_to_same_location(a, b)); + } + + #[test] + fn test_paths_resolve_to_same_location_handles_symlinked_ancestor() { + // Set up: tmp/real_dir/ exists, tmp/link_dir is a symlink to it. + // Compare a leaf under each form. The leaf itself does NOT exist, + // so naive `canonicalize` would fail on both and the function + // would have to fall back to ancestor-canonicalize. This mirrors + // the worktree-removal scenario where git registers the resolved + // path and we pass the un-resolved form for a directory that's + // already gone. + let tmp = tempfile::tempdir().unwrap(); + let real_dir = tmp.path().join("real_dir"); + std::fs::create_dir(&real_dir).unwrap(); + let link_dir = tmp.path().join("link_dir"); + #[cfg(unix)] + std::os::unix::fs::symlink(&real_dir, &link_dir).unwrap(); + #[cfg(windows)] + { + // Best-effort; skip the assertion if the test process lacks + // SeCreateSymbolicLinkPrivilege. + if std::os::windows::fs::symlink_dir(&real_dir, &link_dir).is_err() { + return; + } + } + + let via_real = real_dir.join("missing_leaf"); + let via_link = link_dir.join("missing_leaf"); + assert!( + paths_resolve_to_same_location(&via_real, &via_link), + "paths through a symlinked ancestor must compare equal even when the leaf is missing" + ); + } + + #[test] + fn test_paths_resolve_to_same_location_distinct_existing_ancestors() { + let tmp = tempfile::tempdir().unwrap(); + let a_parent = tmp.path().join("a"); + let b_parent = tmp.path().join("b"); + std::fs::create_dir(&a_parent).unwrap(); + std::fs::create_dir(&b_parent).unwrap(); + let a = a_parent.join("missing"); + let b = b_parent.join("missing"); + assert!( + !paths_resolve_to_same_location(&a, &b), + "distinct parents with the same missing leaf must not compare equal" + ); + } }