From 0457a61a61cd652a62a89dc94c963d565c356cdc Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Mon, 24 Aug 2026 10:38:32 +0100 Subject: [PATCH] refactor(maestro): sidestep worktree path collisions with a numeric suffix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Anything occupying /worktrees/ — one of ours, one the operator made, an unrelated directory — is now bypassed by appending -2, -3, ... to the directory; the branch stays as requested. The same suffix applies under the [daemon].worktree_root override. The daemon no longer asks whether an occupant is "ours": delete registered_for(), the adoption branch in run_add and the SessionManager::create live-session check, plus WorktreeError::PathExists. A branch already checked out elsewhere is git's refusal to surface, already marked by the branch picker's is_head/worktree flags. --- crates/maestro-daemon/src/session.rs | 22 +------ crates/maestro-daemon/src/worktree.rs | 85 +++++++------------------ crates/maestro-daemon/tests/worktree.rs | 77 ++++++++++++---------- 3 files changed, 69 insertions(+), 115 deletions(-) diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index 2472afe..72ae8f9 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -226,25 +226,9 @@ impl SessionManager { let WorktreeSpec { base_repo, branch, .. } = spec; - // `run_add` adopts a leftover tree for this branch — the - // dirty-work guard refuses to delete unsaved work and the path - // is deterministic, so erroring would burn the branch forever. - // A tree a *live* session is using is a different matter: - // two sessions must not share one checkout, so reject it here - // where the session records are visible (#67 review). - let planned = self.inner.worktree_mgr.planned_path(base_repo, branch); - if self - .inner - .sessions - .lock() - .await - .values() - .any(|r| r.worktree_path.as_deref() == Some(planned.as_path())) - { - return Err(SessionError::Worktree( - crate::worktree::WorktreeError::PathExists { path: planned }, - )); - } + // A path collision is sidestepped inside `run_add` (#77), and + // a branch already checked out elsewhere is git's refusal, + // not ours. let path = if spec.create_branch { self.inner.worktree_mgr.add_new(base_repo, branch) } else { diff --git a/crates/maestro-daemon/src/worktree.rs b/crates/maestro-daemon/src/worktree.rs index 1421260..d698606 100644 --- a/crates/maestro-daemon/src/worktree.rs +++ b/crates/maestro-daemon/src/worktree.rs @@ -10,10 +10,6 @@ pub enum WorktreeError { Spawn(#[from] std::io::Error), #[error("git exited with code {code}: {stderr}")] NonZero { code: i32, stderr: String }, - /// A worktree already occupies the target path. The caller decides what - /// to do; the daemon never relocates to a different directory (#56). - #[error("worktree path already exists: {}", .path.display())] - PathExists { path: PathBuf }, /// The worktree holds unsaved work; removal was refused (#56). #[error("left {} on disk: {}", .path.display(), .reason)] Guarded { path: PathBuf, reason: String }, @@ -39,9 +35,9 @@ impl WorktreeManager { } /// Where a worktree for `branch` will be created: the override root, or - /// `
/worktrees/` by default. The path is - /// predictable (no UUID suffix), so an existing directory at the target - /// is an error rather than a silently different location. + /// `
/worktrees/` by default. If the path + /// is occupied, [`Self::add_new`] / [`Self::add_existing`] sidestep to + /// `-2`, `-3`, ... instead. /// /// `base_repo` is whatever the client picked, which the project list makes /// easy to point at a *linked worktree* — placing the new tree inside it @@ -51,14 +47,7 @@ impl WorktreeManager { /// uncommitted work (#54 review). Always place against the main checkout. /// /// The override root gets the repo's basename too: without it two repos - /// spawning the same branch name target one directory and the second - /// fails with `PathExists`. - /// Where [`Self::add_new`] / [`Self::add_existing`] would place this - /// branch's worktree. Callers use it to check ownership before creating. - pub fn planned_path(&self, base_repo: &Path, branch: &str) -> PathBuf { - self.target_path(base_repo, branch) - } - + /// spawning the same branch name target one directory. fn target_path(&self, base_repo: &Path, branch: &str) -> PathBuf { // An unusable branch name falls back to a digest rather than escaping // the root; `run_add` surfaces git's own error for it anyway. @@ -80,6 +69,14 @@ impl WorktreeManager { } } + /// [`Self::target_path`] with `-n` appended to the final component, the + /// collision suffix (#77). + fn target_path_suffixed(&self, base_repo: &Path, branch: &str, n: u32) -> PathBuf { + let mut name = self.target_path(base_repo, branch).into_os_string(); + name.push(format!("-{n}")); + PathBuf::from(name) + } + /// `git -C worktree add -b ` — creates the /// branch. Returns the path of the new worktree on success. pub fn add_new(&self, base_repo: &Path, branch: &str) -> Result { @@ -98,22 +95,18 @@ impl WorktreeManager { branch: &str, create_branch: bool, ) -> Result { - let path = self.target_path(base_repo, branch); - // Clear registrations whose directory is gone before testing the - // path, so a previously-leaked entry doesn't block this branch. + // Clear registrations whose directory is gone first, so a + // previously-leaked entry doesn't push this branch to a suffix. prune(base_repo)?; - if path.symlink_metadata().is_ok() { - // A tree we left behind for this very branch — the dirty-work - // guard refuses to delete unsaved work, and the path is - // deterministic, so erroring here would burn the branch - // permanently: no spawn could ever produce a worktree for it - // again and the product offers no way to clear it. Adopt it - // instead; landing in the operator's own unsaved work on the - // branch they asked for is what they wanted (#67 review). - if registered_for(base_repo, &path, branch) { - return Ok(path); - } - return Err(WorktreeError::PathExists { path }); + // Anything occupying the target — a tree we left behind, one the + // operator made, an unrelated directory — is sidestepped with a + // numeric suffix on the directory; the branch stays whatever was + // requested (#77). The daemon never adopts what it finds. + let mut path = self.target_path(base_repo, branch); + let mut n = 1; + while path.symlink_metadata().is_ok() { + n += 1; + path = self.target_path_suffixed(base_repo, branch, n); } // The parent (`/worktrees` or the override root) may not exist // yet; the target itself must not. @@ -192,34 +185,6 @@ fn main_checkout(path: &Path) -> PathBuf { .unwrap_or_else(|| path.to_path_buf()) } -/// True when `path` is already a registered worktree of `base_repo` holding -/// `branch` — i.e. one this daemon created for this branch and later declined -/// to remove. -fn registered_for(base_repo: &Path, path: &Path, branch: &str) -> bool { - let Ok(out) = Command::new("git") - .arg("-C") - .arg(base_repo) - .args(["worktree", "list", "--porcelain"]) - .output() - else { - return false; - }; - let want = path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); - let text = String::from_utf8_lossy(&out.stdout); - let mut here = false; - for line in text.lines() { - if let Some(p) = line.strip_prefix("worktree ") { - let p = PathBuf::from(p); - here = p.canonicalize().unwrap_or(p) == want; - } else if here { - if let Some(r) = line.strip_prefix("branch ") { - return r.trim() == format!("refs/heads/{branch}"); - } - } - } - false -} - /// `Some(inner)` when a registered worktree lives under `path`. Git omits /// those directories from `git status`, so the dirty-work guard cannot see /// them and removal would delete them (#54 review). @@ -268,9 +233,7 @@ fn short_hash(s: &str) -> String { /// `.git/refs/heads/feat/example` — so it is used verbatim and `feat/example` /// nests under `worktrees/feat/`. That keeps the path readable *and* keeps the /// mapping injective: flattening `/` and `.` to `-` made `feat/x`, `feat.x` -/// and `feat-x` share one directory, and with deterministic paths plus -/// `PathExists` as a hard error the losers could never get a worktree at all -/// (#54 review). +/// and `feat-x` share one directory (#54 review). /// /// Git already forbids holding both `feat` and `feat/x`, so the directory /// collision that nesting could cause cannot arise from valid refs. Only diff --git a/crates/maestro-daemon/tests/worktree.rs b/crates/maestro-daemon/tests/worktree.rs index 0c59bc9..36615a5 100644 --- a/crates/maestro-daemon/tests/worktree.rs +++ b/crates/maestro-daemon/tests/worktree.rs @@ -95,28 +95,26 @@ fn worktree_root_override_redirects_placement() { assert!(path.exists()); } +/// Anything at all occupying the target path — ours, the operator's, or an +/// unrelated directory — is sidestepped, not adopted and not reported (#77). #[test] -fn existing_directory_at_target_path_is_an_error_carrying_the_path() { +fn an_occupied_target_path_gets_a_numeric_suffix() { let repo_dir = TempDir::new().unwrap(); let repo = repo_dir.path().to_path_buf(); init_repo(&repo); - // A predictable path means collisions are possible; they must be - // reported, never silently relocated. - std::fs::create_dir_all(repo.join("worktrees").join("feat").join("collide")).unwrap(); + // A non-git directory sits exactly where the worktree would go. + let squatter = repo.join("worktrees").join("feat").join("collide"); + std::fs::create_dir_all(&squatter).unwrap(); + std::fs::write(squatter.join("operator-notes.txt"), b"mine\n").unwrap(); let mgr = WorktreeManager::new(None); - let err = mgr.add_new(&repo, "feat/collide").unwrap_err(); - let msg = format!("{err}"); + let path = mgr.add_new(&repo, "feat/collide").expect("sidesteps"); + assert_eq!(path, repo.join("worktrees").join("feat").join("collide-2")); + assert!(path.exists()); assert!( - msg.contains( - repo.join("worktrees") - .join("feat") - .join("collide") - .to_str() - .unwrap() - ), - "{msg}" + squatter.join("operator-notes.txt").exists(), + "the occupant is untouched" ); } @@ -381,9 +379,8 @@ fn override_root_keeps_repos_apart() { assert_ne!(a, b, "same branch name in two repos must not collide"); } -/// Flattening `/` and `.` to `-` made these three share one directory, and a -/// deterministic path plus a hard `PathExists` meant the losers could never -/// get a worktree (#54 review). +/// Flattening `/` and `.` to `-` made these three share one directory, so the +/// losers could never get a worktree (#54 review). #[test] fn similar_branch_names_get_distinct_directories() { let repo_dir = TempDir::new().unwrap(); @@ -400,12 +397,11 @@ fn similar_branch_names_get_distinct_directories() { assert!(a.ends_with("feat/x"), "{a:?}"); } -/// The dead end the cleanup guard created: it refuses to delete unsaved work, -/// the path is deterministic, and `PathExists` is a hard error — so one stray -/// untracked file used to burn that branch permanently, with no way to clear -/// it from the product. The leftover is adopted instead (#67 review). +/// A guarded leftover stays on disk untouched, and is never adopted (#77). +/// Re-spawning its branch is refused by git itself — the branch is still +/// checked out inside the leftover — which the caller surfaces. #[test] -fn a_guarded_leftover_is_adopted_rather_than_burning_the_branch() { +fn a_guarded_leftover_is_never_adopted() { let repo_dir = TempDir::new().unwrap(); let repo = repo_dir.path().to_path_buf(); init_repo(&repo); @@ -417,28 +413,39 @@ fn a_guarded_leftover_is_adopted_rather_than_burning_the_branch() { // The guard refuses, leaving it on disk. mgr.remove(&repo, &first) .expect_err("dirty tree is guarded"); - assert!(first.exists()); - // Spawning that branch again lands in the operator's own work, not an error. - let again = mgr + let err = mgr .add_existing(&repo, "feat/resume") - .expect("leftover is adopted"); - assert_eq!(again, first); + .expect_err("a branch already checked out cannot be spawned twice"); + assert!(format!("{err}").contains("git exited"), "{err}"); assert!( - again.join("notes.txt").exists(), - "their work is still there" + first.join("notes.txt").exists(), + "the leftover's unsaved work is untouched" ); } -/// Adoption must not extend to a directory that merely sits in the way. +/// The `worktree_root` override gets the same collision suffix (#77). #[test] -fn an_unrelated_directory_at_the_target_path_is_still_an_error() { +fn the_override_root_suffixes_collisions_too() { let repo_dir = TempDir::new().unwrap(); let repo = repo_dir.path().to_path_buf(); init_repo(&repo); - std::fs::create_dir_all(repo.join("worktrees").join("feat").join("squatter")).unwrap(); - let mgr = WorktreeManager::new(None); - mgr.add_new(&repo, "feat/squatter") - .expect_err("a non-worktree directory must not be adopted"); + let wt_root = TempDir::new().unwrap(); + let mgr = WorktreeManager::new(Some(wt_root.path().to_path_buf())); + + // First add settles `/-/` so the squatter + // can be created at the exact target. + mgr.add_new(&repo, "feat/first").expect("add first"); + let repo_dir_in_root = std::fs::read_dir(wt_root.path()) + .unwrap() + .next() + .unwrap() + .unwrap() + .path(); + let squatter = repo_dir_in_root.join("feat").join("collide"); + std::fs::create_dir_all(&squatter).unwrap(); + + let path = mgr.add_new(&repo, "feat/collide").expect("sidesteps"); + assert_eq!(path, squatter.with_file_name("collide-2"), "{path:?}"); }