Skip to content

Commit c0357e8

Browse files
committed
Handle the origin/-qualified default branch on both read paths
`resolveDefaultBranch` deliberately returns a remote-only default QUALIFIED (`origin/release`) because git cannot resolve a bare name against `refs/remotes/origin/`. Two consumers assumed the bare form: - repo_service.repairVanishedTargets built `live` from `listRemoteBranchNames`, which STRIPS the prefix — so `live` held `release` while `defaultBranch` was `origin/release`. `resolveThroughChain` rejected a perfectly live default as "gone" and skipped the repair, leaving the worktree on a broken target. It now records both spellings. - target_candidates built the candidate list from local branches only, so a remote-only default was omitted entirely and NO candidate received the `default` group — the picker could not offer the branch every diff and new worktree measures against, recreating the exact disagreement the override-threading was meant to fix. It is now offered, and marked `onRemote` (it is on the remote by definition). Coverage note: the picker test fails without its fix (verified). The `repointVanishedTargets` unit tests pin the CONTRACT (the core honours a qualified default; a default absent from `live` is never invented) but pass `live` in by hand, so they do not by themselves cover the caller's `live` construction — the existing "a target that still exists on origin is NOT repaired away" test is what exercises that path.
1 parent 2d94ccd commit c0357e8

4 files changed

Lines changed: 80 additions & 4 deletions

File tree

server/src/repo_service.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,3 +379,37 @@ test("repointVanishedTargets: leaves a broken target in place when nothing resol
379379
});
380380
assert.deepEqual(writes, [], "no default to fall back to: surface targetResolved:false instead");
381381
});
382+
383+
test("repointVanishedTargets: repairs to a remote-only, origin/-qualified default", () => {
384+
// `resolveDefaultBranch` returns a remote-only default QUALIFIED (`origin/release`)
385+
// because git cannot resolve a bare name against `refs/remotes/origin/`, while
386+
// `listRemoteBranchNames` strips the prefix. The caller therefore records BOTH
387+
// spellings in `live`; with only the bare one, `resolveThroughChain` rejected a
388+
// live default as "gone" and the repair was skipped, leaving a broken target.
389+
const writes = repointVanishedTargets({
390+
here: new Set(["/wt"]),
391+
persisted: { "/wt": { target: "feat/gone" } },
392+
live: new Set(["release", "origin/release"]),
393+
branchTarget: {},
394+
ownBranch: { "/wt": "feat/child" },
395+
defaultBranch: "origin/release",
396+
});
397+
assert.deepEqual(writes, [
398+
{ path: "/wt", target: "origin/release", retargetedFrom: "feat/gone" },
399+
]);
400+
});
401+
402+
test("repointVanishedTargets: a default absent from `live` is never invented", () => {
403+
// Pins WHY the caller must record both spellings: a `live` set carrying only the
404+
// bare `release` cannot honour an `origin/release` default, and inventing a
405+
// destination is worse than reporting `targetResolved: false`.
406+
const writes = repointVanishedTargets({
407+
here: new Set(["/wt"]),
408+
persisted: { "/wt": { target: "feat/gone" } },
409+
live: new Set(["release"]),
410+
branchTarget: {},
411+
ownBranch: { "/wt": "feat/child" },
412+
defaultBranch: "origin/release",
413+
});
414+
assert.deepEqual(writes, []);
415+
});

server/src/repo_service.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,16 @@ async function repairVanishedTargets(
229229
// it just is not here yet); a wrong redirect is unrecoverable data loss of the
230230
// user's intent. Never trade the second for the first.
231231
const remotes = await listRemoteBranchNames(repoPath);
232-
const live = new Set<string>([...locals, ...remotes]);
232+
const live = new Set<string>(locals);
233+
for (const b of remotes) {
234+
// BOTH spellings. `listRemoteBranchNames` strips the prefix, but a stored
235+
// target (and `resolveDefaultBranch`'s answer for a remote-only branch) is the
236+
// QUALIFIED `origin/<b>` — git cannot resolve a bare name against
237+
// `refs/remotes/origin/`. Recording only one form left `resolveThroughChain`
238+
// rejecting a perfectly live default as "gone" and skipping the repair.
239+
live.add(b);
240+
live.add(`origin/${b}`);
241+
}
233242
for (const e of entries) {
234243
if (!e.branch) continue;
235244
const pr = lastKnown(repoPath, e.branch);

server/src/target_candidates.test.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,3 +358,25 @@ test("resolveThroughChain refuses a default branch that does not exist either",
358358
null,
359359
);
360360
});
361+
362+
test("a remote-only default branch is still offered, and grouped as the default", async () => {
363+
// `resolveDefaultBranch` returns a remote-only override QUALIFIED (`origin/x`),
364+
// because git cannot resolve a bare name against `refs/remotes/origin/`. Building
365+
// the candidate list from local branches alone dropped it, so the picker could
366+
// not offer the very branch every diff and new worktree measures against.
367+
const s = await makeStack();
368+
try {
369+
const sha = execFileSync("git", ["rev-parse", "HEAD"], { cwd: s.repo }).toString().trim();
370+
execFileSync("git", ["remote", "add", "origin", "https://example.test/x/y.git"], {
371+
cwd: s.repo,
372+
});
373+
execFileSync("git", ["update-ref", "refs/remotes/origin/release", sha], { cwd: s.repo });
374+
const cands = await targetCandidates(s.repo, s.child, "release");
375+
const def = cands.find((c) => c.group === "default");
376+
assert.equal(def?.branch, "origin/release", "the remote-only default is offered");
377+
assert.equal(def?.onRemote, true, "it is on the remote by definition, so selectable");
378+
assert.equal(def?.isSelf, false);
379+
} finally {
380+
s.cleanup();
381+
}
382+
});

server/src/target_candidates.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -131,10 +131,19 @@ export async function targetCandidates(
131131
if (locals.length === 0) return [];
132132

133133
const self = trees.find((t) => t.path === worktreePath)?.branch ?? null;
134+
// The resolved default may be a remote-only branch, which `resolveDefaultBranch`
135+
// returns QUALIFIED (`origin/release`) because git cannot resolve a bare name
136+
// against `refs/remotes/origin/`. It is therefore absent from `locals`, and
137+
// building the list from `locals` alone dropped it entirely — no candidate got
138+
// the `default` group and the picker could not offer the very branch every diff
139+
// and new worktree measures against. Offer it explicitly.
140+
const remoteOnlyDefault =
141+
defaultBranch && !locals.includes(defaultBranch) ? defaultBranch : null;
142+
const selectable = remoteOnlyDefault ? [...locals, remoteOnlyDefault] : locals;
134143
// Candidate order matters: `closestAncestorBranch` breaks distance ties by it,
135144
// and the repo default is the tie we most want to win.
136145
const ordered = [
137-
...(defaultBranch && locals.includes(defaultBranch) ? [defaultBranch] : []),
146+
...(defaultBranch && selectable.includes(defaultBranch) ? [defaultBranch] : []),
138147
...locals.filter((b) => b !== defaultBranch),
139148
];
140149
const forkedFrom = await closestAncestorBranch(worktreePath, ordered);
@@ -159,7 +168,7 @@ export async function targetCandidates(
159168
other: 3,
160169
};
161170

162-
const candidates: TargetCandidate[] = locals.map((branch) => ({
171+
const candidates: TargetCandidate[] = selectable.map((branch) => ({
163172
branch,
164173
group: groupOf(branch),
165174
// The push-state gate exists because a PULL REQUEST base must live on the
@@ -173,7 +182,9 @@ export async function targetCandidates(
173182
// repo whose only remote is `upstream` has a remote but no origin branches, so
174183
// an "any remote" gate would switch the rule ON against an EMPTY set and
175184
// disable every candidate.
176-
onRemote: originExists ? onRemote.has(branch) : true,
185+
// A remote-only default is `origin/<b>`, which is by definition on the remote
186+
// but never in the stripped `onRemote` set — treat it as pushed.
187+
onRemote: originExists ? branch === remoteOnlyDefault || onRemote.has(branch) : true,
177188
isSelf: branch === self,
178189
}));
179190

0 commit comments

Comments
 (0)