fix(git): resolve the base branch from the repository instead of its name
Follow-up to #2629, which stopped the walkthrough from comparing against a branch that does not exist. The same guessing, and the same near-misses in how the answer was applied, were left elsewhere: - The default branch travelled as `rootBranchHint`, whose documented meaning is "the branch the project root worktree is on". It gets its own option, because a parameter that means two things is one the next caller gets wrong. - A candidate equal to the branch being compared is skipped. In a plain checkout the root hint *is* the current branch, so it won every time and produced a comparison with itself; the repository default now wins there. - The Changes and pull-request surfaces read the default branch too. A pull request opened against a branch that does not exist is a worse failure than a walkthrough that will not generate. - `hasResolvableBaseBranch` matched `origin/feature/main` for a base of `main`, passing the check and then failing the comparison it exists to prevent. - `getRangeDiff` promoted only `origin/<base>`. A base carried by any other remote stayed a bare name, which git resolves against refs/heads and nowhere else, so it failed exactly as before. - `getBranches` dropped every branch of a remote that did not answer, turning "we could not ask" into "these branches are gone" — offline, that silently removed comparisons that work fine against local remote-tracking refs. - A remote with no `remote/HEAD` is asked once with `ls-remote --symref` rather than falling back to the guess this data exists to replace. The `defaultBranches` contract was documented under the status response; it belongs to the branches response, which now has a section of its own.
This commit is contained in:
@@ -1417,12 +1417,32 @@ export const GitView: React.FC<GitViewProps> = ({ isActive }) => {
|
||||
}));
|
||||
}, [remotes, remoteBranches, remoteUrl, status?.tracking]);
|
||||
|
||||
const currentBranch = status?.current ?? null;
|
||||
|
||||
// The repository's own default branch, so a repo whose default is neither
|
||||
// main, master nor develop stops being compared against a branch that does
|
||||
// not exist.
|
||||
const defaultBranch = React.useMemo(() => {
|
||||
const trackingRemote = status?.tracking?.trim().split('/')[0];
|
||||
return (trackingRemote && branches?.defaultBranches?.[trackingRemote])
|
||||
?? branches?.defaultBranches?.origin;
|
||||
}, [branches, status?.tracking]);
|
||||
|
||||
const baseBranch = React.useMemo(() => deriveBaseBranch({
|
||||
remoteNames: new Set(effectiveRemotes.map((remote) => remote.name)),
|
||||
localBranches,
|
||||
worktreeCreatedFromBranch: worktreeMetadata?.createdFromBranch,
|
||||
rootBranchHint,
|
||||
}), [effectiveRemotes, localBranches, rootBranchHint, worktreeMetadata?.createdFromBranch]);
|
||||
defaultBranch,
|
||||
headBranch: currentBranch,
|
||||
}), [
|
||||
currentBranch,
|
||||
defaultBranch,
|
||||
effectiveRemotes,
|
||||
localBranches,
|
||||
rootBranchHint,
|
||||
worktreeMetadata?.createdFromBranch,
|
||||
]);
|
||||
|
||||
const updateTargetBranch = React.useMemo(() => {
|
||||
const remoteNames = effectiveRemotes.map((remote) => remote.name);
|
||||
@@ -1511,7 +1531,6 @@ export const GitView: React.FC<GitViewProps> = ({ isActive }) => {
|
||||
|
||||
const stagedCount = stagedChangeEntries.length;
|
||||
const isBusy = isLoading || syncAction !== null || commitAction !== null;
|
||||
const currentBranch = status?.current ?? null;
|
||||
const canShowIntegrateCommitsSection = Boolean(
|
||||
worktreeMetadata && repoRootForIntegrate && sourceBranchForIntegrate && shouldShowIntegrateCommits
|
||||
);
|
||||
|
||||
@@ -213,14 +213,32 @@ export const PullRequestView: React.FC = () => {
|
||||
}));
|
||||
}, [remotes, remoteBranches, remoteUrl, status?.tracking]);
|
||||
|
||||
const currentBranch = status?.current ?? null;
|
||||
|
||||
// A pull request opened against a branch that does not exist is worse than a
|
||||
// broken walkthrough, so this surface reads the repository's default branch
|
||||
// too rather than guessing at main/master/develop.
|
||||
const defaultBranch = React.useMemo(() => {
|
||||
const trackingRemote = status?.tracking?.trim().split('/')[0];
|
||||
return (trackingRemote && branches?.defaultBranches?.[trackingRemote])
|
||||
?? branches?.defaultBranches?.origin;
|
||||
}, [branches, status?.tracking]);
|
||||
|
||||
const baseBranch = React.useMemo(() => deriveBaseBranch({
|
||||
remoteNames: new Set(effectiveRemotes.map((remote) => remote.name)),
|
||||
localBranches,
|
||||
worktreeCreatedFromBranch: worktreeMetadata?.createdFromBranch,
|
||||
rootBranchHint,
|
||||
}), [effectiveRemotes, localBranches, rootBranchHint, worktreeMetadata?.createdFromBranch]);
|
||||
|
||||
const currentBranch = status?.current ?? null;
|
||||
defaultBranch,
|
||||
headBranch: currentBranch,
|
||||
}), [
|
||||
currentBranch,
|
||||
defaultBranch,
|
||||
effectiveRemotes,
|
||||
localBranches,
|
||||
rootBranchHint,
|
||||
worktreeMetadata?.createdFromBranch,
|
||||
]);
|
||||
|
||||
if (!currentDirectory || !currentBranch) {
|
||||
return (
|
||||
|
||||
@@ -2,13 +2,49 @@ import { describe, expect, test } from 'bun:test';
|
||||
import { deriveBaseBranch, hasResolvableBaseBranch } from './baseBranch';
|
||||
|
||||
describe('deriveBaseBranch', () => {
|
||||
test('prefers the remote default branch hint over conventional fallbacks', () => {
|
||||
test('prefers the repository default branch over conventional fallbacks', () => {
|
||||
expect(deriveBaseBranch({
|
||||
remoteNames: new Set(['origin']),
|
||||
localBranches: ['next'],
|
||||
rootBranchHint: 'origin/react',
|
||||
defaultBranch: 'react',
|
||||
})).toBe('react');
|
||||
});
|
||||
|
||||
test('accepts a remote-qualified default branch', () => {
|
||||
expect(deriveBaseBranch({
|
||||
remoteNames: new Set(['origin']),
|
||||
localBranches: ['next'],
|
||||
defaultBranch: 'origin/react',
|
||||
})).toBe('react');
|
||||
});
|
||||
|
||||
test('keeps the more specific worktree origin ahead of the default branch', () => {
|
||||
expect(deriveBaseBranch({
|
||||
remoteNames: new Set(['origin']),
|
||||
localBranches: ['next', 'react', 'feature'],
|
||||
worktreeCreatedFromBranch: 'feature',
|
||||
defaultBranch: 'react',
|
||||
})).toBe('feature');
|
||||
});
|
||||
|
||||
test('skips a hint that is the branch being compared', () => {
|
||||
// In a plain checkout the project root is the current worktree, so the root
|
||||
// branch hint is the current branch — a branch is never its own base.
|
||||
expect(deriveBaseBranch({
|
||||
remoteNames: new Set(['origin']),
|
||||
localBranches: ['next', 'react'],
|
||||
rootBranchHint: 'next',
|
||||
defaultBranch: 'react',
|
||||
headBranch: 'next',
|
||||
})).toBe('react');
|
||||
});
|
||||
|
||||
test('falls back to conventional names when nothing is known', () => {
|
||||
expect(deriveBaseBranch({
|
||||
remoteNames: new Set(['origin']),
|
||||
localBranches: ['master', 'next'],
|
||||
})).toBe('master');
|
||||
});
|
||||
});
|
||||
|
||||
describe('hasResolvableBaseBranch', () => {
|
||||
@@ -21,10 +57,28 @@ describe('hasResolvableBaseBranch', () => {
|
||||
});
|
||||
|
||||
test('accepts a base branch available through a remote-tracking ref', () => {
|
||||
// Safe because getRangeDiff resolves a base that exists only on a remote
|
||||
// through that remote rather than passing the bare name to git.
|
||||
expect(hasResolvableBaseBranch({
|
||||
baseBranch: 'main',
|
||||
localBranches: ['next'],
|
||||
remoteBranches: ['origin/main', 'origin/next'],
|
||||
})).toBe(true);
|
||||
});
|
||||
|
||||
test('does not accept a differently-scoped branch that merely ends the same way', () => {
|
||||
expect(hasResolvableBaseBranch({
|
||||
baseBranch: 'main',
|
||||
localBranches: ['next'],
|
||||
remoteBranches: ['origin/feature/main'],
|
||||
})).toBe(false);
|
||||
});
|
||||
|
||||
test('matches a base branch whose own name contains a slash', () => {
|
||||
expect(hasResolvableBaseBranch({
|
||||
baseBranch: 'release/2.0',
|
||||
localBranches: ['next'],
|
||||
remoteBranches: ['origin/release/2.0'],
|
||||
})).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -8,8 +8,30 @@ export const deriveBaseBranch = (options: {
|
||||
localBranches: readonly string[];
|
||||
worktreeCreatedFromBranch?: string | null;
|
||||
rootBranchHint?: string | null;
|
||||
/**
|
||||
* The repository's own default branch, read from a `remote/HEAD` symbolic
|
||||
* ref. Its own option rather than another hint: `rootBranchHint` means "the
|
||||
* branch the project root worktree is on", and a parameter that means two
|
||||
* things is one the next caller gets wrong.
|
||||
*/
|
||||
defaultBranch?: string | null;
|
||||
/**
|
||||
* The branch being compared. A branch is never its own base, so a candidate
|
||||
* equal to it is skipped — in a plain checkout `rootBranchHint` *is* the
|
||||
* current branch, and taking it produced a comparison with itself.
|
||||
*/
|
||||
headBranch?: string | null;
|
||||
}): string => {
|
||||
const { remoteNames, localBranches, worktreeCreatedFromBranch, rootBranchHint } = options;
|
||||
const {
|
||||
remoteNames,
|
||||
localBranches,
|
||||
worktreeCreatedFromBranch,
|
||||
rootBranchHint,
|
||||
defaultBranch,
|
||||
headBranch,
|
||||
} = options;
|
||||
|
||||
const head = typeof headBranch === 'string' ? headBranch.trim() : '';
|
||||
|
||||
const normalizeBaseCandidate = (value: string): string => {
|
||||
if (!value) {
|
||||
@@ -49,14 +71,22 @@ export const deriveBaseBranch = (options: {
|
||||
return normalized;
|
||||
};
|
||||
|
||||
const fromMeta = normalizeBaseCandidate(
|
||||
typeof worktreeCreatedFromBranch === 'string' ? worktreeCreatedFromBranch : ''
|
||||
);
|
||||
const candidate = (value: unknown): string => {
|
||||
const normalized = normalizeBaseCandidate(typeof value === 'string' ? value : '');
|
||||
return normalized && normalized !== head ? normalized : '';
|
||||
};
|
||||
|
||||
const fromMeta = candidate(worktreeCreatedFromBranch);
|
||||
if (fromMeta) return fromMeta;
|
||||
|
||||
const fromHint = normalizeBaseCandidate(typeof rootBranchHint === 'string' ? rootBranchHint : '');
|
||||
const fromHint = candidate(rootBranchHint);
|
||||
if (fromHint) return fromHint;
|
||||
|
||||
// Authoritative where the hints are guesses: this is what the repository says
|
||||
// its default branch is, so it outranks the conventional names below.
|
||||
const fromDefault = candidate(defaultBranch);
|
||||
if (fromDefault) return fromDefault;
|
||||
|
||||
if (localBranches.includes('main')) return 'main';
|
||||
if (localBranches.includes('master')) return 'master';
|
||||
if (localBranches.includes('develop')) return 'develop';
|
||||
@@ -67,6 +97,11 @@ export const deriveBaseBranch = (options: {
|
||||
* Whether a base branch can be resolved locally or through one of the active
|
||||
* remote-tracking refs. Callers must not offer comparisons against the `main`
|
||||
* fallback when that ref does not actually exist in the repository.
|
||||
*
|
||||
* `remoteBranches` are remote-relative (`origin/main`, `origin/feature/x`), so
|
||||
* the remote name is dropped and the rest compared whole. A suffix test matched
|
||||
* `origin/feature/main` for a base of `main`, which passes the check and then
|
||||
* fails the comparison it was meant to prevent.
|
||||
*/
|
||||
export const hasResolvableBaseBranch = (options: {
|
||||
baseBranch: string;
|
||||
@@ -74,6 +109,9 @@ export const hasResolvableBaseBranch = (options: {
|
||||
remoteBranches: readonly string[];
|
||||
}): boolean => {
|
||||
const { baseBranch, localBranches, remoteBranches } = options;
|
||||
return localBranches.includes(baseBranch)
|
||||
|| remoteBranches.some((branch) => branch.endsWith(`/${baseBranch}`));
|
||||
if (localBranches.includes(baseBranch)) return true;
|
||||
return remoteBranches.some((branch) => {
|
||||
const slashIndex = branch.indexOf('/');
|
||||
return slashIndex > 0 && branch.slice(slashIndex + 1) === baseBranch;
|
||||
});
|
||||
};
|
||||
|
||||
@@ -185,9 +185,14 @@ export const WalkthroughView = ({ directory }: WalkthroughViewProps) => {
|
||||
.filter(Boolean)
|
||||
);
|
||||
const trackingRemote = status?.tracking?.split('/')[0];
|
||||
const rootBranchHint = (trackingRemote && branches?.defaultBranches?.[trackingRemote])
|
||||
const defaultBranch = (trackingRemote && branches?.defaultBranches?.[trackingRemote])
|
||||
?? branches?.defaultBranches?.origin;
|
||||
const baseRef = deriveBaseBranch({ remoteNames, localBranches, rootBranchHint });
|
||||
const baseRef = deriveBaseBranch({
|
||||
remoteNames,
|
||||
localBranches,
|
||||
defaultBranch,
|
||||
headBranch: headRef,
|
||||
});
|
||||
if (!baseRef || baseRef === headRef || !hasResolvableBaseBranch({ baseBranch: baseRef, localBranches, remoteBranches })) {
|
||||
return null;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user