mirror of
https://github.com/max-sixty/worktrunk.git
synced 2026-09-14 20:00:38 +08:00
fix(switch): name a PR base under its remote when it has no local branch (#3951)
`wt switch --create X --base pr:N` (and `--base mr:N`) against a same-repo PR failed whenever the PR's source branch was not already a local branch — the usual shape when branching off someone else's PR: ```console $ wt switch --create feat/review --base pr:101 ◎ Fetching base PR #101... Fix authentication bug in login flow (#101) by @alice · open · feature-auth · https://github.com/owner/repo/pull/101 ◎ Fetching feature-auth from origin... ✗ No branch, tag, or commit named feature-auth ``` The fetch writes only `refs/remotes/<remote>/<branch>`, and git's rev-parse never expands a bare name to a remote-tracking ref, so the base validation rejected the very name the fetch had just made available. `resolve_remote_ref_as_base` now falls back to `<remote>/<branch>` when no local branch has that name, which is what `resolve_base_ref` already does for a remote-only base — and it earns its place for the same second reason documented there, since `git worktree add -b <name> <path> <bare-remote-only-branch>` drops the `-b` and creates the remote branch's own name. A source branch that does exist locally resolves exactly as before, so no previously-working invocation changes and both `--base pr:` snapshots are untouched. <details> <summary>Adjacent problems this deliberately leaves alone</summary> Found while reviewing the change, all pre-existing and none of them the reported defect: - `{{ base_worktree_path }}` is *undefined* rather than empty whenever the base branch has no worktree, and minijinja runs SemiStrict, so a hook or `--execute` template naming it dies with `undefined value` — after `git worktree add` has already run. The pre-flight can't catch it because it validates variable names against the available set, not resolution. General to every `--base`, not just `pr:`. - `--base pr:N` bases on the local branch when one exists, even if it sits behind the head just fetched, so the new branch can start from a stale copy of the PR. Always naming the remote-tracking ref would fix this, at the cost of the case above for every `pr:` base plus a changed success line and a changed `base_branch` in `--format json`. - A local branch literally named `origin/<pr-source-branch>` shadows the remote-tracking ref in the new fallback, so `git worktree add` fails with `ambiguous object name`. Same exposure the existing remote-only base path has, and it fails loudly rather than picking the wrong commit. </details> > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -506,9 +506,10 @@ fn resolve_base_ref(
|
||||
}
|
||||
|
||||
/// Resolve `pr:{N}` / `mr:{N}` for `--base`. Same-repo returns the source
|
||||
/// branch name plus the (remote, branch) the new branch should track; fork
|
||||
/// returns the PR head SHA so we don't create a tracking branch for a ref
|
||||
/// the user hasn't asked to check out.
|
||||
/// branch — under its own name, or `<remote>/<branch>` when no local branch
|
||||
/// has that name — plus the (remote, branch) the new branch should track; fork
|
||||
/// returns the PR head SHA so we don't create a tracking branch for a ref the
|
||||
/// user hasn't asked to check out.
|
||||
fn resolve_remote_ref_as_base(
|
||||
repo: &Repository,
|
||||
provider: &dyn RemoteRefProvider,
|
||||
@@ -531,10 +532,19 @@ fn resolve_remote_ref_as_base(
|
||||
if !info.is_cross_repo {
|
||||
fetch_same_repo_branch(repo, &info)?;
|
||||
let remote = remote_ref::find_remote(repo, &info)?;
|
||||
return Ok((
|
||||
info.source_branch.clone(),
|
||||
Some((remote, info.source_branch.clone())),
|
||||
));
|
||||
let branch = &info.source_branch;
|
||||
// The fetch above writes only `refs/remotes/<remote>/<branch>`, so a
|
||||
// source branch nobody has checked out locally resolves only under its
|
||||
// remote: git's rev-parse never expands a bare name to a
|
||||
// remote-tracking ref, and the bare name would fail the base
|
||||
// validation in `resolve_switch_target`. Same rule, same spelling as
|
||||
// the remote-only base above.
|
||||
let base = if repo.ref_exists(branch)? {
|
||||
branch.clone()
|
||||
} else {
|
||||
format!("{remote}/{branch}")
|
||||
};
|
||||
return Ok((base, Some((remote, branch.clone()))));
|
||||
}
|
||||
|
||||
let remote = remote_ref::find_remote(repo, &info)?;
|
||||
|
||||
@@ -4483,6 +4483,79 @@ fn test_switch_base_pr_sets_upstream(#[from(repo_with_remote)] mut repo: TestRep
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression: `wt switch --create X --base pr:N` against a same-repo PR whose
|
||||
/// source branch exists only on the remote — the shape a fresh clone has for
|
||||
/// someone else's PR. The fetch writes just `refs/remotes/<remote>/<branch>`,
|
||||
/// and git's rev-parse never expands a bare name to a remote-tracking ref, so
|
||||
/// resolving the base to the bare name failed validation with "No branch, tag,
|
||||
/// or commit named <branch>".
|
||||
#[rstest]
|
||||
fn test_switch_base_pr_source_branch_remote_only(#[from(repo_with_remote)] repo: TestRepo) {
|
||||
// Publish the PR's source branch, then drop the local branch.
|
||||
repo.run_git(&["checkout", "-b", "feature-auth"]);
|
||||
fs::write(repo.root_path().join("auth.txt"), "auth").unwrap();
|
||||
repo.run_git(&["add", "auth.txt"]);
|
||||
repo.run_git(&["commit", "-m", "PR commit"]);
|
||||
let pr_head = repo.head_sha();
|
||||
repo.run_git(&["push", "origin", "feature-auth"]);
|
||||
repo.run_git(&["checkout", "main"]);
|
||||
repo.run_git(&["branch", "-D", "feature-auth"]);
|
||||
|
||||
set_github_remote_url(&repo);
|
||||
|
||||
let gh_response = r#"{
|
||||
"title": "Fix authentication bug in login flow",
|
||||
"user": {"login": "alice"},
|
||||
"state": "open",
|
||||
"draft": false,
|
||||
"head": {
|
||||
"ref": "feature-auth",
|
||||
"repo": {"name": "test-repo", "owner": {"login": "owner"}}
|
||||
},
|
||||
"base": {
|
||||
"ref": "main",
|
||||
"repo": {"name": "test-repo", "owner": {"login": "owner"}}
|
||||
},
|
||||
"html_url": "https://github.com/owner/test-repo/pull/101"
|
||||
}"#;
|
||||
let mock_bin = setup_mock_gh_for_pr(&repo, gh_response);
|
||||
|
||||
let mut cmd = repo.wt_command();
|
||||
cmd.args([
|
||||
"switch",
|
||||
"--create",
|
||||
"feat/review-101",
|
||||
"--base",
|
||||
"pr:101",
|
||||
"--no-cd",
|
||||
]);
|
||||
configure_mock_cli_env(&mut cmd, &mock_bin);
|
||||
let output = cmd.output().expect("wt switch should run");
|
||||
assert!(
|
||||
output.status.success(),
|
||||
"wt switch failed: {}",
|
||||
String::from_utf8_lossy(&output.stderr)
|
||||
);
|
||||
|
||||
let created_head = repo.git_output(&["rev-parse", "feat/review-101"]);
|
||||
assert_eq!(
|
||||
created_head, pr_head,
|
||||
"new branch should start at the PR's source branch"
|
||||
);
|
||||
|
||||
// Tracking still points at the PR's source branch (#2497).
|
||||
let remote = repo.git_output(&["config", "--get", "branch.feat/review-101.remote"]);
|
||||
let merge = repo.git_output(&["config", "--get", "branch.feat/review-101.merge"]);
|
||||
assert_eq!(
|
||||
remote, "origin",
|
||||
"branch.feat/review-101.remote should be set so `git push` knows where to push"
|
||||
);
|
||||
assert_eq!(
|
||||
merge, "refs/heads/feature-auth",
|
||||
"branch.feat/review-101.merge should target the PR's source branch on the remote"
|
||||
);
|
||||
}
|
||||
|
||||
/// `wt switch --create X --base pr:N` resolves a fork PR to its head commit
|
||||
/// SHA via refs/pull/N/head without creating a tracking branch.
|
||||
#[rstest]
|
||||
|
||||
Reference in New Issue
Block a user