From 5900e554294d457030f4194a101224688b20624c Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Tue, 1 Sep 2026 13:21:06 -0700 Subject: [PATCH 1/5] Stop a failed fork PR switch from deleting the user's branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `CreationMethod::ForkRef` arm of `wt switch pr:N` / `mr:N` built the branch, its tracking config, and its worktree as four separate git calls, and compensated on any error by force-deleting the branch and discarding the result: `let _ = repo.run_command(&["branch", "-D", "--", &branch])`. Only the first call created anything, and its likeliest failure is git refusing a name that is already taken — the branch is chosen from the PR's head ref, and the check that it is free runs before the `pre-switch` hooks, so anything that claims the name in between (a hook, a concurrent session) lands there. In that case the rollback deleted a branch wt had not created, taking any commits only it held, and the error printed straight afterwards read "a branch named 'X' already exists" — as if nothing had happened. Re-running erased the evidence. Collapse the arm onto the shape the sibling `Regular` arm already uses: a single `git worktree add -b -- FETCH_HEAD`, which writes nothing at all when the name is taken. That leaves nothing half-created, so the rollback goes away rather than being guarded. Tracking config now follows the worktree; if one of those writes fails the user is left with a branch and worktree that aren't wired to the PR, which the next `wt switch pr:N` picks up on its existing-branch path. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01X9MfPQZNDkqUSGxeqgH5TQ --- src/commands/worktree/switch.rs | 82 ++++++++++--------- tests/integration_tests/switch.rs | 131 ++++++++++++++++++++++++++++++ 2 files changed, 174 insertions(+), 39 deletions(-) diff --git a/src/commands/worktree/switch.rs b/src/commands/worktree/switch.rs index db76b77e37..96b775a23e 100644 --- a/src/commands/worktree/switch.rs +++ b/src/commands/worktree/switch.rs @@ -803,17 +803,21 @@ fn validate_worktree_creation( .into()) } -/// Set up a local branch for a fork PR or MR. +/// Set up a local branch and its worktree for a fork PR or MR. /// -/// Creates the branch from FETCH_HEAD, configures tracking (remote, merge ref, -/// pushRemote), and creates the worktree. Returns an error if any step fails - -/// caller is responsible for cleanup. +/// One `git worktree add -b -- FETCH_HEAD` creates the branch +/// and the worktree together, exactly as the [`CreationMethod::Regular`] arm +/// does, so git owns the atomicity: a name already taken fails before anything +/// is written, and there is nothing for a caller to roll back. Tracking +/// (`remote`, `merge`, `pushRemote`) is configured afterwards; a failure there +/// leaves a branch and worktree that exist but aren't wired to the PR/MR — an +/// ordinary state the user can re-run into, since the next `wt switch pr:N` +/// takes the existing-branch path. /// /// # Arguments /// /// * `remote_ref` - The ref to track (e.g., "pull/123/head" or "merge-requests/101/head") /// * `fork_push_url` - URL to push to, or `None` if push isn't supported (prefixed branch) -/// * `label` - Human-readable label for error messages (e.g., "PR #123" or "MR !101") fn setup_fork_branch( repo: &Repository, branch: &str, @@ -821,18 +825,34 @@ fn setup_fork_branch( remote_ref: &str, fork_push_url: Option<&str>, worktree_path: &Path, - label: &str, ) -> anyhow::Result<()> { - // Create local branch from FETCH_HEAD - // Use -- to prevent branch names starting with - from being interpreted as flags - repo.run_command(&["branch", "--", branch, "FETCH_HEAD"]) - .with_context(|| { - cformat!( - "Failed to create local branch {} from {}", - branch, - label - ) - })?; + // Create branch and worktree in one command (delayed streaming: silent if + // fast, shows progress if slow). `-b ` keeps the branch name as the + // value of a flag, and `--` separates the path and start point, so neither + // can be read as an option when it begins with `-`. + // + // No `-c branch.autoSetupMerge=…` here, unlike the `Regular` arm: the + // setting only fires when the start point is a remote-tracking branch, and + // `FETCH_HEAD` isn't one — under every value of it git writes no tracking + // config, leaving the `set_config` calls below as the only writers. + let worktree_path_str = worktree_path.to_string_lossy(); + let git_args = [ + "worktree", + "add", + "-b", + branch, + "--", + worktree_path_str.as_ref(), + "FETCH_HEAD", + ]; + repo.run_command_delayed_stream( + &git_args, + Repository::SLOW_OPERATION_DELAY_MS, + Some( + progress_message(cformat!("Creating worktree for {}...", branch)).to_string(), + ), + ) + .map_err(|e| worktree_creation_error(&e, branch.to_string(), None))?; // Configure branch tracking for pull and push let branch_remote_key = format!("branch.{}.remote", branch); @@ -851,19 +871,6 @@ fn setup_fork_branch( .with_context(|| format!("Failed to configure branch.{}.pushRemote", branch))?; } - // Create worktree (delayed streaming: silent if fast, shows progress if slow) - // Use -- to prevent branch names starting with - from being interpreted as flags - let worktree_path_str = worktree_path.to_string_lossy(); - let git_args = ["worktree", "add", "--", worktree_path_str.as_ref(), branch]; - repo.run_command_delayed_stream( - &git_args, - Repository::SLOW_OPERATION_DELAY_MS, - Some( - progress_message(cformat!("Creating worktree for {}...", branch)).to_string(), - ), - ) - .map_err(|e| worktree_creation_error(&e, branch.to_string(), None))?; - Ok(()) } @@ -1186,22 +1193,19 @@ fn execute_switch( repo.run_command(&["fetch", "--", remote, ref_path]) .with_context(|| format!("Failed to fetch {} from {}", label, remote))?; - // Execute branch creation and configuration with cleanup on failure. - let setup_result = setup_fork_branch( + // No rollback on failure — `setup_fork_branch` leaves + // nothing half-written. A rollback here once deleted the + // branch on any error, so the "a branch named 'X' already + // exists" case force-deleted a branch `wt` had not created, + // taking any commits only it held. + setup_fork_branch( repo, &branch, remote, ref_path, fork_push_url.as_deref(), &worktree_path, - &label, - ); - - if let Err(e) = setup_result { - // Cleanup: try to delete the branch if it was created - let _ = repo.run_command(&["branch", "-D", "--", &branch]); - return Err(e); - } + )?; // Show push configuration or warning about prefixed branch if let Some(url) = fork_push_url { diff --git a/tests/integration_tests/switch.rs b/tests/integration_tests/switch.rs index f06503cd78..78439ca6d4 100644 --- a/tests/integration_tests/switch.rs +++ b/tests/integration_tests/switch.rs @@ -3526,6 +3526,137 @@ fn test_switch_pr_fork(#[from(repo_with_remote)] repo: TestRepo) { }); } +/// A failed fork `pr:N` switch leaves a branch it did not create alone. +/// +/// The fork path used to create the branch, its tracking config, and its +/// worktree as four separate git calls, compensating with a force-delete of the +/// branch on any error. Only the first call created anything, and its likeliest +/// failure is git refusing a name that is already taken — so the rollback +/// deleted a branch someone else owned, took whatever commits only it held with +/// it, and then printed "a branch named 'X' already exists", which reads as if +/// nothing happened. `git worktree add -b` creates the branch and the worktree +/// in one call, and writes nothing when the name is taken, so there is nothing +/// to roll back. +/// +/// The `pre-switch` hook claims the name after the forge answered (which is +/// what selects the fork path) and before the worktree is created, standing in +/// for the concurrent session that does the same in real use. It renames a +/// never-pushed local branch onto the PR's branch name, so `feature-fix` is the +/// only ref holding that commit when the switch fails. +#[rstest] +fn test_switch_pr_fork_failure_keeps_existing_branch(#[from(repo_with_remote)] repo: TestRepo) { + // Same fork-PR fixture as test_switch_pr_fork: refs/pull/42/head on the + // bare remote, origin redirected to a GitHub-style URL, `gh api` mocked. + repo.run_git(&["checkout", "-b", "pr-source"]); + fs::write(repo.root_path().join("pr-file.txt"), "PR content").unwrap(); + repo.run_git(&["add", "pr-file.txt"]); + repo.run_git(&["commit", "-m", "PR commit"]); + let sha = String::from_utf8_lossy( + &repo + .git_command() + .args(["rev-parse", "HEAD"]) + .run() + .unwrap() + .stdout, + ) + .trim() + .to_string(); + repo.run_git(&["push", "origin", &format!("{}:refs/pull/42/head", sha)]); + repo.run_git(&["checkout", "main"]); + + // Local work that exists nowhere else. + repo.run_git(&["checkout", "-b", "stale-work"]); + fs::write(repo.root_path().join("stale.txt"), "unpushed work").unwrap(); + repo.run_git(&["add", "stale.txt"]); + repo.run_git(&["commit", "-m", "Unpushed local work"]); + let stale_sha = String::from_utf8_lossy( + &repo + .git_command() + .args(["rev-parse", "HEAD"]) + .run() + .unwrap() + .stdout, + ) + .trim() + .to_string(); + repo.run_git(&["checkout", "main"]); + + fs::create_dir_all(repo.root_path().join(".config")).unwrap(); + fs::write( + repo.root_path().join(".config/wt.toml"), + "pre-switch = \"git -C '{{ repo_path }}' branch -m stale-work feature-fix\"\n", + ) + .unwrap(); + + let bare_url = String::from_utf8_lossy( + &repo + .git_command() + .args(["config", "remote.origin.url"]) + .run() + .unwrap() + .stdout, + ) + .trim() + .to_string(); + repo.run_git(&[ + "remote", + "set-url", + "origin", + "https://github.com/owner/test-repo.git", + ]); + repo.run_git(&[ + "config", + &format!("url.{}.insteadOf", bare_url), + "https://github.com/owner/test-repo.git", + ]); + + let gh_response = r#"{ + "title": "Add feature fix for edge case", + "user": {"login": "contributor"}, + "state": "open", + "draft": false, + "head": { + "ref": "feature-fix", + "repo": {"name": "test-repo", "owner": {"login": "contributor"}} + }, + "base": { + "ref": "main", + "repo": {"name": "test-repo", "owner": {"login": "owner"}} + }, + "html_url": "https://github.com/owner/test-repo/pull/42" + }"#; + let mock_bin = setup_mock_gh_for_pr(&repo, gh_response); + + let mut cmd = repo.wt_command(); + cmd.args(["switch", "pr:42", "--yes"]); + configure_mock_cli_env(&mut cmd, &mock_bin); + let output = cmd.output().expect("wt switch pr:42 should run"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !output.status.success(), + "wt switch pr:42 should fail once the branch name is taken: stderr={stderr}" + ); + assert!( + stderr.contains("already exists"), + "the failure should name the branch collision: stderr={stderr}" + ); + + let head = repo + .git_command() + .args(["rev-parse", "--verify", "refs/heads/feature-fix"]) + .run() + .unwrap(); + assert!( + head.status.success(), + "feature-fix must survive the failed switch: stderr={stderr}" + ); + assert_eq!( + String::from_utf8_lossy(&head.stdout).trim(), + stale_sha, + "feature-fix must still point at the unpushed commit" + ); +} + /// Every hook on a PR/MR-created worktree — `pre-switch`, `pre-start`, /// `post-start`, `post-switch` — sees `pr_number` and `pr_url` in its template /// context. Both GitHub PRs and GitLab MRs canonicalize to the same `pr_*` From ebb2538b050c01eb55cc7f51657026daad2e4a72 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Tue, 1 Sep 2026 13:32:35 -0700 Subject: [PATCH 2/5] Correct the recovery claim and narrow the atomicity claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the `setup_fork_branch` docstring overstating two things. A failed `branch..remote` or `.merge` write does not leave a branch the next `wt switch pr:N` adopts: `branch_tracks_ref` requires both keys to match, so it reports the branch as tracking something else and the next run takes the prefixed-branch path (GitHub/Gitea) or reports `BranchTracksDifferentRef` (GitLab/Azure DevOps). Only a failed `pushRemote` write leaves the state the docstring described. Nothing is lost either way, which is the point, but say what actually happens. And `git worktree add -b` is not atomic in general: with the destination path occupied it prints "Preparing worktree", creates the branch, then dies on the path — verified on git 2.55. It is atomic for exactly the branch-name collision this fix is about, which is the claim the code rests on. Also record why the creating call must come first: configuring tracking ahead of it would rewrite the existing branch's upstream on that same collision. `branch.autoSetupMerge`'s rule is the start point resolving under `refs/heads/` or `refs/remotes/`, not "a remote-tracking branch" — `always` tracks a local branch start point too. `FETCH_HEAD` qualifies under neither, so the conclusion stands. The test reuses `set_github_remote_url` rather than hand-rolling it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01X9MfPQZNDkqUSGxeqgH5TQ --- src/commands/worktree/switch.rs | 41 +++++++++++++++++++++---------- tests/integration_tests/switch.rs | 25 +++---------------- 2 files changed, 31 insertions(+), 35 deletions(-) diff --git a/src/commands/worktree/switch.rs b/src/commands/worktree/switch.rs index 96b775a23e..4d0e5ab01d 100644 --- a/src/commands/worktree/switch.rs +++ b/src/commands/worktree/switch.rs @@ -807,12 +807,25 @@ fn validate_worktree_creation( /// /// One `git worktree add -b -- FETCH_HEAD` creates the branch /// and the worktree together, exactly as the [`CreationMethod::Regular`] arm -/// does, so git owns the atomicity: a name already taken fails before anything -/// is written, and there is nothing for a caller to roll back. Tracking -/// (`remote`, `merge`, `pushRemote`) is configured afterwards; a failure there -/// leaves a branch and worktree that exist but aren't wired to the PR/MR — an -/// ordinary state the user can re-run into, since the next `wt switch pr:N` -/// takes the existing-branch path. +/// does. Git rejects a name that is already taken before writing anything, so +/// the one failure that lands on a branch this function did not create leaves +/// it untouched, and no caller needs a rollback. (Git is not atomic in +/// general: a destination path occupied between planning and here leaves the +/// new branch behind — the leftover the `Regular` arm already accepts, a stray +/// branch at the PR head rather than someone's work.) +/// +/// The creating call comes first so nothing is written until the name is won. +/// Configuring tracking (`remote`, `merge`, `pushRemote`) ahead of it would +/// rewrite the *existing* branch's upstream on exactly the collision above — +/// the same class of bug as the rollback this replaced. +/// +/// A tracking write that fails afterwards leaves a branch and worktree at the +/// PR/MR head with incomplete config. Nothing is lost and a re-run is +/// non-destructive, but it isn't a no-op either: `branch_tracks_ref` reads that +/// branch as tracking something else, so the next `wt switch pr:N` takes the +/// prefixed-branch path (GitHub/Gitea) or reports `BranchTracksDifferentRef` +/// (GitLab/Azure DevOps). Only a failed `pushRemote` write — the last one, with +/// the other two landed — leaves a branch the next run adopts directly. /// /// # Arguments /// @@ -831,10 +844,12 @@ fn setup_fork_branch( // value of a flag, and `--` separates the path and start point, so neither // can be read as an option when it begins with `-`. // - // No `-c branch.autoSetupMerge=…` here, unlike the `Regular` arm: the - // setting only fires when the start point is a remote-tracking branch, and - // `FETCH_HEAD` isn't one — under every value of it git writes no tracking - // config, leaving the `set_config` calls below as the only writers. + // No `-c branch.autoSetupMerge=…` here, unlike the `Regular` arm: git sets + // up tracking only where the start point resolves under `refs/heads/` or + // `refs/remotes/`, and `FETCH_HEAD` resolves as itself. Under every value + // of the setting — `always` included — git writes no tracking config from + // this start point, leaving the `set_config` calls below as the only + // writers. let worktree_path_str = worktree_path.to_string_lossy(); let git_args = [ "worktree", @@ -1193,11 +1208,11 @@ fn execute_switch( repo.run_command(&["fetch", "--", remote, ref_path]) .with_context(|| format!("Failed to fetch {} from {}", label, remote))?; - // No rollback on failure — `setup_fork_branch` leaves - // nothing half-written. A rollback here once deleted the + // No rollback on failure. One here used to delete the // branch on any error, so the "a branch named 'X' already // exists" case force-deleted a branch `wt` had not created, - // taking any commits only it held. + // taking any commits only it held. `setup_fork_branch` + // leaves nothing for a rollback to clean up. setup_fork_branch( repo, &branch, diff --git a/tests/integration_tests/switch.rs b/tests/integration_tests/switch.rs index 78439ca6d4..7b8baa5503 100644 --- a/tests/integration_tests/switch.rs +++ b/tests/integration_tests/switch.rs @@ -3584,31 +3584,12 @@ fn test_switch_pr_fork_failure_keeps_existing_branch(#[from(repo_with_remote)] r fs::create_dir_all(repo.root_path().join(".config")).unwrap(); fs::write( repo.root_path().join(".config/wt.toml"), - "pre-switch = \"git -C '{{ repo_path }}' branch -m stale-work feature-fix\"\n", + r#"pre-switch = "git -C '{{ repo_path }}' branch -m stale-work feature-fix" +"#, ) .unwrap(); - let bare_url = String::from_utf8_lossy( - &repo - .git_command() - .args(["config", "remote.origin.url"]) - .run() - .unwrap() - .stdout, - ) - .trim() - .to_string(); - repo.run_git(&[ - "remote", - "set-url", - "origin", - "https://github.com/owner/test-repo.git", - ]); - repo.run_git(&[ - "config", - &format!("url.{}.insteadOf", bare_url), - "https://github.com/owner/test-repo.git", - ]); + set_github_remote_url(&repo); let gh_response = r#"{ "title": "Add feature fix for edge case", From 3feab178f83a2aa5e14944be8b350a41a6aa7137 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Tue, 1 Sep 2026 15:11:07 -0700 Subject: [PATCH 3/5] Name the conflicting branch when a fork PR's head ref is taken The ForkRef arm now creates its branch with the same `git worktree add -b` as the Regular arm, so it hits the same namespace collision: git stores refs as file paths, and a fork PR whose head ref is `feature` can't create a branch in a repo that already has `feature/x`. Route the failure through `detect_branch_namespace_conflict` as the Regular arm does, so the user sees which local branch is in the way instead of git's "cannot lock ref" text. --- src/commands/worktree/switch.rs | 14 ++++++- tests/integration_tests/switch.rs | 70 +++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/commands/worktree/switch.rs b/src/commands/worktree/switch.rs index 4d0e5ab01d..5d2f72a5ff 100644 --- a/src/commands/worktree/switch.rs +++ b/src/commands/worktree/switch.rs @@ -867,7 +867,19 @@ fn setup_fork_branch( progress_message(cformat!("Creating worktree for {}...", branch)).to_string(), ), ) - .map_err(|e| worktree_creation_error(&e, branch.to_string(), None))?; + .map_err(|e| { + // Same mapping as the `Regular` arm: git stores refs as file paths, so + // a fork PR whose head ref is `feature` cannot create a branch in a + // repo that already has `feature/x`. Name the conflicting branch + // instead of passing on git's raw "cannot lock ref" text. + match detect_branch_namespace_conflict(repo, branch) { + Some(conflicting) => GitError::BranchNamespaceConflict { + branch: branch.to_string(), + conflicting, + }, + None => worktree_creation_error(&e, branch.to_string(), None), + } + })?; // Configure branch tracking for pull and push let branch_remote_key = format!("branch.{}.remote", branch); diff --git a/tests/integration_tests/switch.rs b/tests/integration_tests/switch.rs index 7b8baa5503..2b506ada08 100644 --- a/tests/integration_tests/switch.rs +++ b/tests/integration_tests/switch.rs @@ -3638,6 +3638,76 @@ fn test_switch_pr_fork_failure_keeps_existing_branch(#[from(repo_with_remote)] r ); } +/// A fork PR whose head ref collides with an existing branch's namespace names +/// the conflicting branch rather than passing on git's raw "cannot lock ref". +/// +/// Git stores refs as file paths, so `feature-fix` and `feature-fix/nested` +/// can't both exist. The fork path creates its branch with the same +/// `git worktree add -b` as the `Regular` arm, so it maps the failure the same +/// way; the user can't rename a PR's head ref, so the message has to point at +/// the local branch that's in the way. +#[rstest] +fn test_switch_pr_fork_namespace_conflict(#[from(repo_with_remote)] repo: TestRepo) { + // Same fork-PR fixture as test_switch_pr_fork: refs/pull/42/head on the + // bare remote, origin redirected to a GitHub-style URL, `gh api` mocked. + repo.run_git(&["checkout", "-b", "pr-source"]); + fs::write(repo.root_path().join("pr-file.txt"), "PR content").unwrap(); + repo.run_git(&["add", "pr-file.txt"]); + repo.run_git(&["commit", "-m", "PR commit"]); + let sha = String::from_utf8_lossy( + &repo + .git_command() + .args(["rev-parse", "HEAD"]) + .run() + .unwrap() + .stdout, + ) + .trim() + .to_string(); + repo.run_git(&["push", "origin", &format!("{}:refs/pull/42/head", sha)]); + repo.run_git(&["checkout", "main"]); + + // Occupy the namespace the PR's branch name needs. + repo.run_git(&["branch", "feature-fix/nested"]); + + set_github_remote_url(&repo); + + let gh_response = r#"{ + "title": "Add feature fix for edge case", + "user": {"login": "contributor"}, + "state": "open", + "draft": false, + "head": { + "ref": "feature-fix", + "repo": {"name": "test-repo", "owner": {"login": "contributor"}} + }, + "base": { + "ref": "main", + "repo": {"name": "test-repo", "owner": {"login": "owner"}} + }, + "html_url": "https://github.com/owner/test-repo/pull/42" + }"#; + let mock_bin = setup_mock_gh_for_pr(&repo, gh_response); + + let mut cmd = repo.wt_command(); + cmd.args(["switch", "pr:42", "--yes"]); + configure_mock_cli_env(&mut cmd, &mock_bin); + let output = cmd.output().expect("wt switch pr:42 should run"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !output.status.success(), + "wt switch pr:42 should fail when the branch name is unavailable: stderr={stderr}" + ); + assert!( + stderr.contains("feature-fix/nested"), + "the failure should name the branch in the way: stderr={stderr}" + ); + assert!( + !stderr.contains("cannot lock ref"), + "git's raw ref-lock text should not reach the user: stderr={stderr}" + ); +} + /// Every hook on a PR/MR-created worktree — `pre-switch`, `pre-start`, /// `post-start`, `post-switch` — sees `pr_number` and `pr_url` in its template /// context. Both GitHub PRs and GitLab MRs canonicalize to the same `pr_*` From 6992a31643280af6b6c23e7f8ec9b6f1e885de5f Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Tue, 1 Sep 2026 15:23:49 -0700 Subject: [PATCH 4/5] Assert the mapped wording, not just the conflicting branch name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `stderr.contains("feature-fix/nested")` passed against the unmapped error too — git names the conflicting ref in its own "cannot lock ref" text — so the test only discriminated through its negative assertion. Assert `BranchNamespaceConflict`'s own wording as well. Verified by reverting the mapping: the test fails. --- tests/integration_tests/switch.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/integration_tests/switch.rs b/tests/integration_tests/switch.rs index 2b506ada08..a9de6bd4bf 100644 --- a/tests/integration_tests/switch.rs +++ b/tests/integration_tests/switch.rs @@ -3698,8 +3698,11 @@ fn test_switch_pr_fork_namespace_conflict(#[from(repo_with_remote)] repo: TestRe !output.status.success(), "wt switch pr:42 should fail when the branch name is unavailable: stderr={stderr}" ); + // `collides with existing branch` is `BranchNamespaceConflict`'s own + // wording. Asserting only on `feature-fix/nested` would pass against the + // unmapped error too — git names the conflicting ref in its raw text. assert!( - stderr.contains("feature-fix/nested"), + stderr.contains("collides with existing branch") && stderr.contains("feature-fix/nested"), "the failure should name the branch in the way: stderr={stderr}" ); assert!( From f23ed0bdfe882f322bfc75d89e519634ee17c9f4 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Tue, 1 Sep 2026 18:03:14 -0700 Subject: [PATCH 5/5] Say what the path-race leftover costs, and drop a wrong count The docstring called the branch left behind by an occupied destination path "a stray branch at the PR head" and stopped there, while spelling out the same follow-on cost two paragraphs later for a failed tracking write: no config means the next `wt switch pr:N` takes the prefixed-branch path or errors. Name it in both places, and note why the old code's clean rollback of this one case isn't worth recovering. The test's "four separate git calls" was five under its own fixture, which has a fork URL and so writes three tracking keys. The count adds nothing. --- src/commands/worktree/switch.rs | 7 ++++++- tests/integration_tests/switch.rs | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/commands/worktree/switch.rs b/src/commands/worktree/switch.rs index 5d2f72a5ff..7588749355 100644 --- a/src/commands/worktree/switch.rs +++ b/src/commands/worktree/switch.rs @@ -812,7 +812,12 @@ fn validate_worktree_creation( /// it untouched, and no caller needs a rollback. (Git is not atomic in /// general: a destination path occupied between planning and here leaves the /// new branch behind — the leftover the `Regular` arm already accepts, a stray -/// branch at the PR head rather than someone's work.) +/// branch at the PR head rather than someone's work. It carries the same +/// follow-on cost as a failed tracking write, below: no config was written, so +/// the next `wt switch pr:N` takes the prefixed-branch path or errors. The old +/// code did roll this one case back cleanly, its own branch being the only +/// thing it could have deleted — but distinguishing it needs the very +/// did-we-create-it reasoning that produced the bug.) /// /// The creating call comes first so nothing is written until the name is won. /// Configuring tracking (`remote`, `merge`, `pushRemote`) ahead of it would diff --git a/tests/integration_tests/switch.rs b/tests/integration_tests/switch.rs index a9de6bd4bf..633905b9e2 100644 --- a/tests/integration_tests/switch.rs +++ b/tests/integration_tests/switch.rs @@ -3529,7 +3529,7 @@ fn test_switch_pr_fork(#[from(repo_with_remote)] repo: TestRepo) { /// A failed fork `pr:N` switch leaves a branch it did not create alone. /// /// The fork path used to create the branch, its tracking config, and its -/// worktree as four separate git calls, compensating with a force-delete of the +/// worktree as separate git calls, compensating with a force-delete of the /// branch on any error. Only the first call created anything, and its likeliest /// failure is git refusing a name that is already taken — so the rollback /// deleted a branch someone else owned, took whatever commits only it held with