Stop a failed fork PR switch from deleting the user's branch

src/commands/worktree/switch.rs

@@ -803,36 +803,83 @@ 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 <branch> -- <path> FETCH_HEAD` creates the branch
+/// and the worktree together, exactly as the [`CreationMethod::Regular`] arm
+/// 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
 ///
 /// * `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,
     remote: &str,
     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 <bold>{}</> from {}",
-                branch,
-                label
-            )
-        })?;
+    // Create branch and worktree in one command (delayed streaming: silent if
+    // fast, shows progress if slow). `-b <branch>` 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: 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",
+        "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 <bold>{}</>...", branch)).to_string(),
+        ),
+    )
+    .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);
@@ -851,19 +898,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 <bold>{}</>...", branch)).to_string(),
-        ),
-    )
-    .map_err(|e| worktree_creation_error(&e, branch.to_string(), None))?;
-
     Ok(())
 }
 
@@ -1186,22 +1220,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. 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. `setup_fork_branch`
+                    // leaves nothing for a rollback to clean up.
+                    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 {

tests/integration_tests/switch.rs

@@ -3526,6 +3526,188 @@ 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"),
+        r#"pre-switch = "git -C '{{ repo_path }}' branch -m stale-work feature-fix"
+"#,
+    )
+    .unwrap();
+
+    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 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"
+    );
+}
+
+/// 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_*`