From f801b12e877f4779975df32833ae8d62ede748f7 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Fri, 4 Sep 2026 13:29:11 +0100 Subject: [PATCH] fix(maestro): suffix branch name on new-branch worktree collision git worktree add -b fails outright when the branch already exists, even if the target directory is free. A random fast-forward handle commonly collides with a branch an earlier, already-removed worktree left behind. Co-Authored-By: Claude Sonnet 5 --- crates/maestro-daemon/src/worktree.rs | 52 ++++++++++++++++++++------- crates/maestro-e2e/tests/e2e.rs | 51 ++++++++++++++++++++++++++ docs/WORKTREES.md | 16 ++++++--- 3 files changed, 103 insertions(+), 16 deletions(-) diff --git a/crates/maestro-daemon/src/worktree.rs b/crates/maestro-daemon/src/worktree.rs index 4f97c33..508dadd 100644 --- a/crates/maestro-daemon/src/worktree.rs +++ b/crates/maestro-daemon/src/worktree.rs @@ -111,16 +111,33 @@ impl WorktreeManager { // Clear registrations whose directory is gone first, so a // previously-leaked entry doesn't push this branch to a suffix. prune(base_repo)?; - // 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); - } + let (branch, path) = if create_branch { + // Worktrees are commonly abandoned without deleting their branch, + // so a random handle can collide with one of its own past runs. + // Unlike the `add_existing` arm below, the suffix goes on the + // branch name itself — the path is derived from it anyway. + let mut candidate = branch.to_string(); + let mut path = self.target_path(base_repo, &candidate); + let mut n = 1; + while path.symlink_metadata().is_ok() || branch_exists(base_repo, &candidate) { + n += 1; + candidate = format!("{branch}-{n}"); + path = self.target_path(base_repo, &candidate); + } + (candidate, path) + } else { + // Existing branch: the name must stay exact, so only the + // directory — anything occupying it, ours or the operator's — is + // sidestepped with a numeric suffix (#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); + } + (branch.to_string(), path) + }; // The parent (`/worktrees` or the override root) may not exist // yet; the target itself must not. if let Some(parent) = path.parent() { @@ -129,10 +146,10 @@ impl WorktreeManager { let mut cmd = Command::new("git"); cmd.arg("-C").arg(base_repo).args(["worktree", "add"]); if create_branch { - cmd.arg("-b").arg(branch).arg(&path); + cmd.arg("-b").arg(&branch).arg(&path); } else { // Existing branch: `worktree add ` with no `-b`. - cmd.arg(&path).arg(branch); + cmd.arg(&path).arg(&branch); } let out = cmd.output()?; check(out)?; @@ -266,6 +283,17 @@ fn branch_relpath(branch: &str) -> Option { Some(candidate.to_path_buf()) } +/// Whether `refs/heads/` already exists in `base_repo`. +fn branch_exists(base_repo: &Path, branch: &str) -> bool { + Command::new("git") + .arg("-C") + .arg(base_repo) + .args(["show-ref", "--verify", "--quiet"]) + .arg(format!("refs/heads/{branch}")) + .status() + .is_ok_and(|s| s.success()) +} + /// `git -C worktree prune` — drops registrations whose directory /// no longer exists. fn prune(base_repo: &Path) -> Result<(), WorktreeError> { diff --git a/crates/maestro-e2e/tests/e2e.rs b/crates/maestro-e2e/tests/e2e.rs index 7c77395..d883908 100644 --- a/crates/maestro-e2e/tests/e2e.rs +++ b/crates/maestro-e2e/tests/e2e.rs @@ -497,6 +497,57 @@ async fn occupied_path_gets_suffixed_directory_and_both_worktrees_work() { ); } +/// A random handle can reuse the name of a branch left behind by an earlier, +/// already-removed worktree — `git worktree add -b` refuses outright in that +/// case, even though the directory is free. The daemon suffixes the branch +/// itself (not just the directory) and both land on `-2`. +#[tokio::test(flavor = "multi_thread")] +async fn colliding_branch_name_gets_suffixed_too() { + let (_daemon, client) = bring_up().await; + + let base = TempDir::new().unwrap(); + init_repo(base.path()); + + // No worktree directory in the way, but the branch already exists. + run_git(&["branch", "handle"], base.path()); + + let s = client + .spawn_session(&worktree_req(base.path(), "handle", true)) + .await + .expect("spawn onto a taken branch name"); + + let suffixed = base.path().join("worktrees").join("handle-2"); + assert_eq!( + s.cwd, suffixed, + "the directory must follow the suffixed branch name" + ); + assert!(suffixed.is_dir()); + + let registrations = git_worktree_list(base.path()); + assert!( + registrations.iter().any(|l| l.contains("handle-2")), + "the -2 branch's worktree must be registered: {registrations:?}" + ); + + let branches = String::from_utf8( + std::process::Command::new("git") + .arg("-C") + .arg(base.path()) + .args(["branch", "--list"]) + .output() + .unwrap() + .stdout, + ) + .unwrap(); + assert!( + branches.contains("handle-2"), + "the branch itself must be named handle-2: {branches}" + ); + + let _origin = throwaway_origin(base.path()); + client.kill_session(s.id).await.expect("kill"); +} + /// #77/#88: the target path holds a plain directory the daemon never made and /// git has never heard of. The daemon neither adopts it nor errors — it builds /// beside it and leaves every byte of it alone. diff --git a/docs/WORKTREES.md b/docs/WORKTREES.md index 4f10835..fff8b03 100644 --- a/docs/WORKTREES.md +++ b/docs/WORKTREES.md @@ -35,10 +35,18 @@ The branch name is used verbatim as a relative path, so `feature/login` becomes back to a short hash of the branch. If that path exists, the daemon appends `-2`, then `-3`, and so on until it finds -a free one. The suffix names the **directory only**. The branch is whatever was -requested; a second worktree of the same branch is not what happens here, because -git refuses to check the same branch out twice and the branch picker already -marks branches that are checked out elsewhere. +a free one. + +For a *new* branch (`create_branch: true`), the suffix goes on the branch name +itself, and the directory follows since it is derived from the branch — a +random handle can otherwise reuse the name of a branch some earlier, already +removed worktree left behind, and `git worktree add -b` refuses outright even +though the directory is free. + +For an *existing* branch (`create_branch: false`) the name must stay exact, so +only the directory is suffixed — a second worktree of the same branch is not +what happens here, because git refuses to check the same branch out twice and +the branch picker already marks branches that are checked out elsewhere. ### The `worktree_root` override