mirror of
https://github.com/stablyai/orca.git
synced 2026-10-29 08:07:18 +00:00
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.
This commit is contained in:
@@ -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.<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.<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.<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' })
|
||||
],
|
||||
[
|
||||
|
||||
@@ -37,3 +37,32 @@ export async function hasWorktreeBaseCommitRef(
|
||||
): Promise<boolean> {
|
||||
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<WorktreeBaseRefPresence> {
|
||||
try {
|
||||
const { stdout } = await runGit([
|
||||
'for-each-ref',
|
||||
'--count=1',
|
||||
'--format=%(refname)',
|
||||
qualifiedRef
|
||||
])
|
||||
return stdout.trim() === qualifiedRef ? 'present' : 'absent'
|
||||
} catch {
|
||||
return 'unknown'
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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/<branch> 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' } }
|
||||
|
||||
@@ -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/<branch> 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' } }
|
||||
|
||||
@@ -475,7 +475,7 @@ describe('registerWorktreeHandlers', () => {
|
||||
expect(result.localBaseRefUpdateSuggestion).toBeUndefined()
|
||||
})
|
||||
// #15331: the pre-create merge-base probe fails when refs/heads/<branch> 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',
|
||||
|
||||
Reference in New Issue
Block a user