diff --git a/src/commands/worktree/switch.rs b/src/commands/worktree/switch.rs index db76b77e37..7588749355 100644 --- a/src/commands/worktree/switch.rs +++ b/src/commands/worktree/switch.rs @@ -803,17 +803,39 @@ 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. 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. 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 +/// 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, @@ -821,18 +843,48 @@ 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: 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 {}...", 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 +903,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 +1225,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 { diff --git a/tests/integration_tests/switch.rs b/tests/integration_tests/switch.rs index f06503cd78..633905b9e2 100644 --- a/tests/integration_tests/switch.rs +++ b/tests/integration_tests/switch.rs @@ -3526,6 +3526,191 @@ 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 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}" + ); + // `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("collides with existing branch") && 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_*`