From 6f3847cfc70770dd1869a1f44cefd15376c51b07 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:56:47 -0700 Subject: [PATCH] fix(worktree): only suppress the stale local-base warning on a proven missing ref The suppression probe used `rev-parse --verify --quiet`, which exits 1 both when the ref is absent and when the probe cannot run at all, so a broken repo or a dropped relay connection silently swallowed a real divergence warning. Probe with `for-each-ref` instead: it exits 0 whether or not the pattern matches, so an empty result proves absence while a rejection stays "unknown" and keeps the existing `skipped_not_fast_forward` status. Same helper on the local and SSH paths. --- .../worktree-add-local-base-refresh.test.ts | 40 ++++++++++++++----- src/main/git/worktree-base-ref-probe.ts | 29 ++++++++++++++ src/main/git/worktree.ts | 9 ++++- src/main/ipc/worktree-remote.ts | 10 ++++- .../worktrees-ssh-local-base-refresh.test.ts | 33 ++++++++++++--- 5 files changed, 102 insertions(+), 19 deletions(-) diff --git a/src/main/git/worktree-add-local-base-refresh.test.ts b/src/main/git/worktree-add-local-base-refresh.test.ts index bd189ca5ed6..cec1151e0eb 100644 --- a/src/main/git/worktree-add-local-base-refresh.test.ts +++ b/src/main/git/worktree-add-local-base-refresh.test.ts @@ -347,7 +347,7 @@ describe('addWorktree', () => { it('skips updating the local branch when it has diverged', async () => { gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} gitExecFileAsyncMock.mockRejectedValueOnce(new Error('not a fast-forward')) - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'local-main\n' }) // rev-parse --verify --quiet refs/heads/main^{commit} (exists) + gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'refs/heads/main\n' }) // for-each-ref refs/heads/main (exists) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // worktree add resolveCreationBaseConfigWrite() gitExecFileAsyncMock.mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) @@ -370,7 +370,7 @@ describe('addWorktree', () => { expect.objectContaining({ cwd: '/repo' }) ], [ - ['rev-parse', '--verify', '--quiet', 'refs/heads/main^{commit}'], + ['for-each-ref', '--count=1', '--format=%(refname)', 'refs/heads/main'], expect.objectContaining({ cwd: '/repo' }) ], [ @@ -415,7 +415,7 @@ describe('addWorktree', () => { "fatal: ambiguous argument 'refs/heads/feature-x...refs/remotes/origin/feature-x': unknown revision or path not in the working tree." ) ) // rev-list: refs/heads/feature-x does not exist yet - .mockResolvedValueOnce({ stdout: '' }) // rev-parse --verify --quiet refs/heads/feature-x^{commit} (missing) + .mockResolvedValueOnce({ stdout: '' }) // for-each-ref refs/heads/feature-x (missing) .mockResolvedValueOnce({ stdout: '' }) // worktree add .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) @@ -449,7 +449,7 @@ describe('addWorktree', () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse --verify --quiet refs/remotes/origin/main^{commit} .mockRejectedValueOnce(new Error('unknown revision refs/heads/main')) // rev-list: no local main - .mockResolvedValueOnce({ stdout: '' }) // rev-parse --verify --quiet refs/heads/main^{commit} (missing) + .mockResolvedValueOnce({ stdout: '' }) // for-each-ref refs/heads/main (missing) .mockResolvedValueOnce({ stdout: '' }) // worktree add .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) @@ -459,13 +459,33 @@ describe('addWorktree', () => { expect(result.localBaseRefRefresh).toBeUndefined() expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).toContainEqual([ - 'rev-parse', - '--verify', - '--quiet', - 'refs/heads/main^{commit}' + 'for-each-ref', + '--count=1', + '--format=%(refname)', + 'refs/heads/main' ]) }) + // A failed probe is not proof of absence, so the warning must survive it. + it('keeps the warning when the local base ref probe itself fails', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse --verify --quiet refs/remotes/origin/main^{commit} + .mockRejectedValueOnce(new Error('rev-list failed')) // drift probe + .mockRejectedValueOnce(new Error('fatal: not a git repository')) // for-each-ref probe could not run + .mockResolvedValueOnce({ stdout: '' }) // worktree add + .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base + .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) + .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote + + const result = await addWorktree('/repo', '/repo-feature', 'my-feature', 'origin/main', true) + + expect(result.localBaseRefRefresh).toEqual({ + status: 'skipped_not_fast_forward', + baseRef: 'origin/main', + localBranch: 'main' + }) + }) + it('still suggests nothing but keeps the warning when the local base ref exists and diverged', async () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse --verify --quiet refs/remotes/origin/main^{commit} @@ -491,7 +511,7 @@ describe('addWorktree', () => { gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'new-local\n' }) // rev-parse refs/heads/main^{commit} gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse refs/remotes/origin/main^{commit} gitExecFileAsyncMock.mockRejectedValueOnce(new Error('not an ancestor')) // merge-base captured OIDs - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'new-local\n' }) // rev-parse --verify --quiet refs/heads/main^{commit} (exists) + gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'refs/heads/main\n' }) // for-each-ref refs/heads/main (exists) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // worktree add resolveCreationBaseConfigWrite() gitExecFileAsyncMock.mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) @@ -526,7 +546,7 @@ describe('addWorktree', () => { expect.objectContaining({ cwd: '/repo' }) ], [ - ['rev-parse', '--verify', '--quiet', 'refs/heads/main^{commit}'], + ['for-each-ref', '--count=1', '--format=%(refname)', 'refs/heads/main'], expect.objectContaining({ cwd: '/repo' }) ], [ diff --git a/src/main/git/worktree-base-ref-probe.ts b/src/main/git/worktree-base-ref-probe.ts index 44b8d4d85e9..85c8c8baa8f 100644 --- a/src/main/git/worktree-base-ref-probe.ts +++ b/src/main/git/worktree-base-ref-probe.ts @@ -37,3 +37,32 @@ export async function hasWorktreeBaseCommitRef( ): Promise { return (await resolveWorktreeBaseCommitOid(repoPath, qualifiedRef, options)) !== null } + +export type WorktreeBaseRefPresence = 'present' | 'absent' | 'unknown' + +/** + * Distinguish "the ref does not exist" from "the probe itself failed". + * + * Why for-each-ref: it exits 0 whether or not the pattern matches, so an empty result + * proves absence while a rejection still means the probe never ran (broken repo, dead + * SSH transport). `rev-parse --verify --quiet` exits 1 for both, and reading that as + * "absent" would silently drop warnings the caller must still surface. + * + * Executor-injected so the SSH path can route the same argv through the relay. + */ +export async function probeWorktreeBaseRefPresence( + runGit: (args: string[]) => Promise<{ stdout: string }>, + qualifiedRef: string +): Promise { + try { + const { stdout } = await runGit([ + 'for-each-ref', + '--count=1', + '--format=%(refname)', + qualifiedRef + ]) + return stdout.trim() === qualifiedRef ? 'present' : 'absent' + } catch { + return 'unknown' + } +} diff --git a/src/main/git/worktree.ts b/src/main/git/worktree.ts index 36b0e0521ce..fcd2a0067fa 100644 --- a/src/main/git/worktree.ts +++ b/src/main/git/worktree.ts @@ -33,7 +33,7 @@ import { import { withLocalGitCapabilityCacheForExecution } from './git-capability-state' import { gitExecFileAsync, translateWslOutputPaths } from './runner' import { resolveGitDir, runWithGitReadCacheInvalidation } from './status' -import { hasWorktreeBaseCommitRef } from './worktree-base-ref-probe' +import { hasWorktreeBaseCommitRef, probeWorktreeBaseRefPresence } from './worktree-base-ref-probe' export type AddWorktreeResult = { localBaseRefRefresh?: LocalBaseRefRefreshResult @@ -267,7 +267,12 @@ async function evaluateLocalBaseRefRefreshability( } catch { // Why (#15331): the probes above also fail when refs/heads/ is simply absent; a branch that // does not exist yet cannot be stale, so report nothing instead of a bogus divergence warning. - if (!(await hasWorktreeBaseCommitRef(repoPath, parsed.fullRef, options))) { + // Only a proven absence suppresses: an unusable repo leaves the warning alone. + const presence = await probeWorktreeBaseRefPresence( + (args) => gitExecFileAsync(args, gitExecOptions(repoPath, options)), + parsed.fullRef + ) + if (presence === 'absent') { return undefined } return { refreshable: false, result: { ...resultBase, status: 'skipped_not_fast_forward' } } diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index 2bed434fac6..1d63cf97deb 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -35,6 +35,7 @@ import { } from '../git/repo' import { resolveLocalGitUsername, getSshGitUsername } from '../git/git-username' import { hasCommitObjectViaGitExec } from '../git/commit-object-ref' +import { probeWorktreeBaseRefPresence } from '../git/worktree-base-ref-probe' import { resolveWorktreeCreateBase } from '../worktree-create-base' import { resolveWorktreeAddBaseRef } from '../../shared/worktree/base-ref' import { getHostedReviewForBranch } from '../source-control/hosted-review' @@ -1394,8 +1395,13 @@ async function evaluateRemoteLocalBaseRefRefreshability( } } catch { // Why (#15331): the probes above also fail when refs/heads/ is simply absent; the relay's - // `worktree add -b` is about to create it, so there is nothing stale to warn about. - if (!(await hasCommitRefSsh(provider, repoPath, fullRef))) { + // `worktree add -b` is about to create it, so there is nothing stale to warn about. Only a proven + // absence suppresses: a dropped relay connection is not evidence the branch is missing. + const presence = await probeWorktreeBaseRefPresence( + (args) => provider.exec(args, repoPath), + fullRef + ) + if (presence === 'absent') { return { refreshable: false, result: undefined } } return { refreshable: false, result: { ...resultBase, status: 'skipped_not_fast_forward' } } diff --git a/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts b/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts index bbb056f057f..1a908e38376 100644 --- a/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts +++ b/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts @@ -475,7 +475,7 @@ describe('registerWorktreeHandlers', () => { expect(result.localBaseRefUpdateSuggestion).toBeUndefined() }) // #15331: the pre-create merge-base probe fails when refs/heads/ does not exist yet. - const buildMissingLocalBaseSshCase = (localHeadOid: string) => { + const buildMissingLocalBaseSshCase = (presence: 'absent' | 'present' | 'probe-failed') => { const repo = { id: 'repo-ssh', path: '/remote/repo', @@ -490,10 +490,16 @@ describe('registerWorktreeHandlers', () => { if (args[0] === 'remote') { return { stdout: 'origin\n', stderr: '' } } + if (args[0] === 'for-each-ref' && args.at(-1) === 'refs/heads/main') { + if (presence === 'probe-failed') { + throw new Error('ssh: connection closed by remote host') + } + return { stdout: presence === 'present' ? 'refs/heads/main\n' : '', stderr: '' } + } if (args[0] === 'rev-parse') { const ref = args.at(-1) ?? '' if (ref.startsWith('refs/heads/main')) { - return { stdout: localHeadOid, stderr: '' } + return { stdout: presence === 'present' ? 'local-main\n' : '', stderr: '' } } // Other refs/heads probes are the new-branch conflict check; it must stay unresolvable. return { stdout: ref.startsWith('refs/heads/') ? '' : 'remote-main\n', stderr: '' } @@ -535,7 +541,7 @@ describe('registerWorktreeHandlers', () => { } it('does not report an SSH local base refresh when the local base branch does not exist', async () => { - const provider = buildMissingLocalBaseSshCase('') + const provider = buildMissingLocalBaseSshCase('absent') const result = (await handlers['worktrees:create'](null, { repoId: 'repo-ssh', @@ -543,7 +549,7 @@ describe('registerWorktreeHandlers', () => { })) as CreateWorktreeResult expect(provider.exec).toHaveBeenCalledWith( - ['rev-parse', '--verify', '--quiet', 'refs/heads/main^{commit}'], + ['for-each-ref', '--count=1', '--format=%(refname)', 'refs/heads/main'], '/remote/repo' ) expect(result.localBaseRefRefresh).toBeUndefined() @@ -551,7 +557,24 @@ describe('registerWorktreeHandlers', () => { }) it('keeps the SSH not-fast-forward status when the local base branch exists and diverged', async () => { - const provider = buildMissingLocalBaseSshCase('local-main\n') + const provider = buildMissingLocalBaseSshCase('present') + + const result = (await handlers['worktrees:create'](null, { + repoId: 'repo-ssh', + name: 'improve-dashboard' + })) as CreateWorktreeResult + + expect(result.localBaseRefRefresh).toEqual({ + status: 'skipped_not_fast_forward', + baseRef: 'origin/main', + localBranch: 'main' + }) + expect(provider.refreshLocalBaseRefForWorktreeCreate).not.toHaveBeenCalled() + }) + + // Losing the relay mid-probe is not evidence the branch is missing. + it('keeps the SSH not-fast-forward status when the local base ref probe fails', async () => { + const provider = buildMissingLocalBaseSshCase('probe-failed') const result = (await handlers['worktrees:create'](null, { repoId: 'repo-ssh',