Skip to content

Commit 3de2e67

Browse files
wenshaowenshao
authored
fix(core): validate git pull option combinations and detached HEAD (#10752)
Post-merge review follow-ups for the dirty-worktree pull (#10390): - fetchOnly combined with stash or force was silently winning: the flow performed a bare fetch and dropped the resolution the caller asked for. Both combinations are refused now — a 400 invalid_fetch_only_combination at the route and a mirrored guard in gitPull — matching the existing stash+force exclusivity. - A stash or force pull on a detached HEAD surfaced git's raw "HEAD does not point to a branch" through an unclassified 500; it is a typed pull_failed refusal now, before anything is touched. - validatedUpstream resolves the branch once and reads the configured upstream inline, so the no-upstream tail is explicit about the one probe race that can reach it instead of looking unreachable. - The resolution panel no longer offers Discard after the daemon refused it with force_unsupported: the refusal is permanent for a subdirectory workspace, so re-offering it could only loop. - Design doc: note that a stored-back stash entry lands on top of the stack, and state the plain pull's invariant precisely (same git invocation; output now path-redacted like every other response).
1 parent 8051bf0 commit 3de2e67

7 files changed

Lines changed: 113 additions & 38 deletions

File tree

‎docs/design/git-pull-dirty-worktree.md‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,17 @@ checks the SHA git reports as dropped — git has no identity-addressed
8282
drop — and if the slots shifted under it, the other entry is `git stash
8383
store`d back and ours is reported as kept; should even that store fail,
8484
the displaced entry's SHA and the command that brings it back are in the
85-
output. Every notice about a kept entry carries its SHA.
86-
87-
A plain pull — no option — is byte-for-byte the previous behavior.
85+
output. Every notice about a kept entry carries its SHA. A stored-back
86+
entry lands on top of the stack rather than in its old slot — position is
87+
not part of any identity here, and the output says what happened — so a
88+
terminal relying on `git stash pop` order should read `git stash list`
89+
first.
90+
91+
A plain pull — no option — runs the exact git invocation it always has;
92+
the only response-level change is that its `output` is now path-redacted
93+
like every other response. The three options are mutually exclusive:
94+
`stash`+`force` and `fetchOnly` combined with either are refused as 400s
95+
rather than silently dropping one of them.
8896

8997
## Non-goals (deliberate)
9098

‎packages/cli/src/serve/routes/workspace-git-branches.test.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,18 @@ describe('workspace Git branch routes', () => {
107107
expect(response.body.error).toBe('invalid_stash_force');
108108
});
109109

110+
it.each([{ stash: true }, { force: true }])(
111+
'rejects combining fetchOnly with %o with 400',
112+
async (extra) => {
113+
const response = await request(app())
114+
.post('/workspace/git/pull')
115+
.send({ fetchOnly: true, ...extra });
116+
117+
expect(response.status).toBe(400);
118+
expect(response.body.error).toBe('invalid_fetch_only_combination');
119+
},
120+
);
121+
110122
it('rejects a checkout with a missing ref with 400', async () => {
111123
const response = await request(app())
112124
.post('/workspace/git/checkout')

‎packages/cli/src/serve/routes/workspace-git-branches.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,13 @@ async function handlePull(
288288
});
289289
return;
290290
}
291+
if (fetchOnly && (stash || force)) {
292+
res.status(400).json({
293+
error: 'invalid_fetch_only_combination',
294+
message: 'fetchOnly cannot be combined with stash or force',
295+
});
296+
return;
297+
}
291298
try {
292299
const result = await gitPull(cwd, { rebase, fetchOnly, stash, force }, env);
293300
// A successful stash pull can still carry git's notice about a failed

‎packages/core/src/utils/git-branches.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -921,6 +921,38 @@ describe('gitPull with a dirty working tree', () => {
921921
);
922922
});
923923

924+
it('rejects combining fetchOnly with stash or force instead of dropping them', async () => {
925+
const dir = makeRepo();
926+
await expect(
927+
gitPull(dir, { fetchOnly: true, stash: true }),
928+
).rejects.toThrow(/cannot be combined/);
929+
await expect(
930+
gitPull(dir, { fetchOnly: true, force: true }),
931+
).rejects.toThrow(/cannot be combined/);
932+
});
933+
934+
it('types the refusal on a detached HEAD without touching anything', async () => {
935+
const { dir, clone } = makeUpstream();
936+
remoteCommit(clone, 'remote-only.txt', 'remote\n');
937+
git(dir, 'checkout', '-q', '--detach', 'HEAD');
938+
fs.writeFileSync(path.join(dir, 'a.txt'), 'local edit\n');
939+
const headBefore = headSha(dir);
940+
941+
const stash = await expectPullFailure(
942+
gitPull(dir, { stash: true }, hermeticEnv()),
943+
'pull_failed',
944+
);
945+
await expectPullFailure(
946+
gitPull(dir, { force: true }, hermeticEnv()),
947+
'pull_failed',
948+
);
949+
950+
expect(stash.message).toContain('HEAD is detached');
951+
expect(headSha(dir)).toBe(headBefore);
952+
expect(read(dir, 'a.txt')).toBe('local edit\n');
953+
expect(stashList(dir)).toEqual([]);
954+
});
955+
924956
it('refuses stash and force pulls while a merge is in progress, keeping it', async () => {
925957
const { dir, clone } = makeUpstream();
926958
remoteCommit(clone, 'a.txt', 'remote\n');

‎packages/core/src/utils/git-branches.ts‎

Lines changed: 33 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -854,47 +854,49 @@ function pullArgs(opts?: GitPullOptions): string[] {
854854
return args;
855855
}
856856

857-
/** The upstream ref configured for the checked-out branch, or ''. */
858-
async function configuredUpstream(
859-
cwd: string,
860-
env?: Readonly<Record<string, string | undefined>>,
861-
): Promise<string> {
862-
try {
863-
const branch = (
864-
await runGit(cwd, ['symbolic-ref', '--quiet', '--short', 'HEAD'], env)
865-
).trim();
866-
return (
867-
await runGit(
868-
cwd,
869-
['for-each-ref', '--format=%(upstream)', `refs/heads/${branch}`],
870-
env,
871-
)
872-
).trim();
873-
} catch {
874-
return '';
875-
}
876-
}
877-
878857
/**
879858
* SHA of the upstream tip, resolved after the flow's own fetch (so a
880859
* tracking ref pruned earlier is back if the remote branch exists again).
881-
* A branch with no upstream configured fails with git's own message, as a
882-
* plain pull always has; a configured upstream whose remote branch is
883-
* gone is a typed refusal — nothing has been touched at this point.
860+
* A detached HEAD and a configured upstream whose remote branch is gone
861+
* are typed refusals — nothing has been touched at this point; a branch
862+
* with no upstream configured fails with git's own message, as a plain
863+
* pull always has.
884864
*/
885865
async function validatedUpstream(
886866
cwd: string,
887867
env?: Readonly<Record<string, string | undefined>>,
888868
): Promise<string> {
889869
const sha = await upstreamSha(cwd, env);
890870
if (sha) return sha;
891-
if (await configuredUpstream(cwd, env)) {
871+
let branch: string;
872+
try {
873+
branch = (
874+
await runGit(cwd, ['symbolic-ref', '--quiet', '--short', 'HEAD'], env)
875+
).trim();
876+
} catch {
877+
throw new GitPullFailure(
878+
'pull_failed',
879+
'cannot update: HEAD is detached; check out a branch from a terminal first',
880+
);
881+
}
882+
const configured = (
883+
await runGit(
884+
cwd,
885+
['for-each-ref', '--format=%(upstream)', `refs/heads/${branch}`],
886+
env,
887+
).catch(() => '')
888+
).trim();
889+
if (configured) {
892890
throw new GitPullFailure(
893891
'pull_failed',
894892
'cannot update: the upstream branch no longer exists on the remote; nothing was changed',
895893
);
896894
}
895+
// No upstream is configured: surface git's own message so the route
896+
// classifies it as it always has.
897897
await runGit(cwd, ['rev-parse', '--abbrev-ref', '@{upstream}'], env);
898+
// Reachable only when the upstream appeared between the probes; refuse
899+
// conservatively instead of proceeding on a tip that was never validated.
898900
throw new GitPullFailure(
899901
'pull_failed',
900902
'cannot update: the upstream branch could not be resolved; nothing was changed',
@@ -1081,6 +1083,12 @@ export async function gitPull(
10811083
if (opts?.stash && opts?.force) {
10821084
throw new Error('stash and force are mutually exclusive');
10831085
}
1086+
if (opts?.fetchOnly && (opts?.stash || opts?.force)) {
1087+
// A fetch-only request has nothing to stash around or discard for;
1088+
// refuse rather than silently dropping the resolution the caller
1089+
// asked for.
1090+
throw new Error('fetchOnly cannot be combined with stash or force');
1091+
}
10841092
if (opts?.fetchOnly) {
10851093
const output = await runGit(cwd, ['fetch', '--all', '--prune'], env);
10861094
return { success: true, output: output.trim() };

‎packages/web-shell/client/components/BranchPickerPopover.test.tsx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -644,6 +644,9 @@ describe('BranchPickerPopover actions', () => {
644644
'cannot discard changes: the workspace is a subdirectory',
645645
);
646646
expect(footerText()).toContain('Stash Changes and Update');
647+
// The daemon declared discarding impossible for this workspace; the
648+
// action is gone rather than looping the same refusal.
649+
expect(footerText()).not.toContain('Discard Changes and Update');
647650
expect(footerText()).not.toContain('Discard and Update');
648651
});
649652

‎packages/web-shell/client/components/BranchPickerPopover.tsx‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -300,8 +300,11 @@ export function BranchPickerPopover({
300300
const [busyAction, setBusyAction] = useState<string | null>(null);
301301
const [pullBlocked, setPullBlocked] = useState(false);
302302
const [confirmDiscard, setConfirmDiscard] = useState(false);
303-
// Daemon explanation shown in the panel instead of the fixed blocked line,
304-
// when the refusal carried one worth reading (a discard the daemon refused).
303+
// Daemon explanation shown in the panel instead of the fixed blocked line
304+
// when the refusal carried one worth reading — a discard the daemon
305+
// refused (force_unsupported). While set, the Discard action is hidden:
306+
// the daemon has declared it impossible for this workspace, so offering
307+
// it again could only loop the same refusal.
305308
const [pullBlockedDetail, setPullBlockedDetail] = useState<string | null>(
306309
null,
307310
);
@@ -1007,14 +1010,16 @@ export function BranchPickerPopover({
10071010
)}
10081011
{t('branchPicker.pullStash')}
10091012
</button>
1010-
<button
1011-
type="button"
1012-
className={`${styles.pullBlockedButton} ${styles.pullBlockedButtonDanger}`}
1013-
disabled={!!busyAction}
1014-
onClick={() => setConfirmDiscard(true)}
1015-
>
1016-
{t('branchPicker.pullDiscard')}
1017-
</button>
1013+
{pullBlockedDetail === null && (
1014+
<button
1015+
type="button"
1016+
className={`${styles.pullBlockedButton} ${styles.pullBlockedButtonDanger}`}
1017+
disabled={!!busyAction}
1018+
onClick={() => setConfirmDiscard(true)}
1019+
>
1020+
{t('branchPicker.pullDiscard')}
1021+
</button>
1022+
)}
10181023
<button
10191024
type="button"
10201025
className={styles.pullBlockedButton}

0 commit comments

Comments
 (0)