Refresh cleanup base before forced branch deletion (#4462)

This commit is contained in:
Jinjing 2026-06-02 01:08:11 -07:00 committed by GitHub
parent fd20889f9b
commit d21e73301e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 214 additions and 2 deletions

View File

@ -389,6 +389,77 @@ branch refs/heads/main
expect(calls).toContain('git config --remove-section branch.feature/test')
})
it('refreshes the saved remote base before deleting a safe-delete-rejected branch', async () => {
mockGitCommands({
'git worktree list --porcelain -z': {
stdout: `worktree /repo
HEAD abc123
branch refs/heads/main
worktree /repo-feature
HEAD def456
branch refs/heads/feature/test
`
},
'git worktree list --porcelain -z#2': {
stdout: `worktree /repo
HEAD abc123
branch refs/heads/main
`
},
'git worktree list --porcelain': {
stdout: `worktree /repo
HEAD abc123
branch refs/heads/main
`
},
'git worktree list --porcelain#2': {
stdout: `worktree /repo
HEAD abc123
branch refs/heads/main
`
},
'git branch -d -- feature/test': {
error: new Error('branch delete failed'),
stderr: 'error: the branch feature/test is not fully merged'
},
'git config --get branch.feature/test.base': {
stdout: 'refs/remotes/origin/main\n'
},
'git remote': {
stdout: 'origin\n'
},
'git fetch --prune origin': {
stdout: ''
},
'git rev-parse --verify --quiet refs/remotes/origin/main^{commit}': {
stdout: 'base123\n'
},
'git merge-tree --write-tree base123 refs/heads/feature/test': {
stdout: 'tree123\n'
},
'git rev-parse --verify --quiet base123^{tree}': {
stdout: 'tree123\n'
}
})
await expect(removeWorktree('/repo', '/repo-feature')).resolves.toEqual({})
const calls = getGitCalls()
expect(calls).toContain('git fetch --prune origin')
expect(calls).toContain('git update-ref -d refs/heads/feature/test def456')
expectGitCallOrder(
calls,
'git fetch --prune origin',
'git merge-tree --write-tree base123 refs/heads/feature/test'
)
expectGitCallOrder(
calls,
'git fetch --prune origin',
'git update-ref -d refs/heads/feature/test def456'
)
})
it('preserves an already-merged branch when cleanup races after worktree removal', async () => {
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
mockGitCommands({

View File

@ -3,7 +3,8 @@ import { stat } from 'fs/promises'
import { join, posix, win32 } from 'path'
import {
branchHasNoUnmergedChangesOnAnyTarget,
getBranchCleanupTargetRefs
getBranchCleanupTargetRefs,
refreshBranchCleanupTargetRefs
} from '../../shared/git-branch-cleanup'
import { resolveWorktreeAddBaseRef } from '../../shared/worktree-base-ref'
import type {
@ -622,6 +623,7 @@ async function deleteAlreadyMergedBranchAfterSafeDeleteFailure(
): Promise<boolean> {
const runGit = (args: string[]) => gitExecFileAsync(args, { cwd: repoPath })
const targetRefs = await getBranchCleanupTargetRefs(runGit, branchName)
await refreshBranchCleanupTargetRefs(runGit, targetRefs)
// Why: squash merges rewrite commit IDs, so `branch -d` can reject a branch
// whose changes are already on the base ref. Delete only when Git can prove
// the branch contributes no tree changes to that base.

View File

@ -72,6 +72,73 @@ describe('removeWorktreeOp branch cleanup', () => {
)
})
it('refreshes the saved remote base before deleting a safe-delete-rejected SSH branch', async () => {
const calls: { args: string[]; cwd: string }[] = []
let zListCount = 0
const git = vi.fn<GitExec>(async (args, cwd) => {
calls.push({ args, cwd })
if (args[0] === 'rev-parse' && args[1] === '--git-common-dir') {
return { stdout: '/repo/.git\n', stderr: '' }
}
if (args[0] === 'worktree' && args[1] === 'list' && args.includes('-z')) {
zListCount += 1
return {
stdout:
zListCount === 1
? worktreeList(
{ path: '/repo', branch: 'main' },
{ path: '/repo-feature', branch: 'feature/test' }
)
: worktreeList({ path: '/repo', branch: 'main' }),
stderr: ''
}
}
if (args[0] === 'worktree' && args[1] === 'list') {
return { stdout: worktreeList({ path: '/repo', branch: 'main' }), stderr: '' }
}
if (args[0] === 'branch' && args[1] === '-d') {
throw new Error('error: the branch feature/test is not fully merged')
}
if (args[0] === 'config' && args[1] === '--get') {
return { stdout: 'refs/remotes/origin/main\n', stderr: '' }
}
if (args[0] === 'remote') {
return { stdout: 'origin\n', stderr: '' }
}
if (args[0] === 'fetch') {
return { stdout: '', stderr: '' }
}
if (args[0] === 'rev-parse' && args.includes('refs/remotes/origin/main^{commit}')) {
return { stdout: 'base123\n', stderr: '' }
}
if (args[0] === 'merge-tree') {
return { stdout: 'tree123\n', stderr: '' }
}
if (args[0] === 'rev-parse' && args.includes('base123^{tree}')) {
return { stdout: 'tree123\n', stderr: '' }
}
return { stdout: '', stderr: '' }
})
await expect(removeWorktreeOp(git, { worktreePath: '/repo-feature' })).resolves.toEqual({})
const commandIndex = (expectedArgs: string[]) =>
calls.findIndex(({ args }) => JSON.stringify(args) === JSON.stringify(expectedArgs))
const fetchIndex = commandIndex(['fetch', '--prune', 'origin'])
const mergeTreeIndex = commandIndex([
'merge-tree',
'--write-tree',
'base123',
'refs/heads/feature/test'
])
const updateRefIndex = commandIndex(['update-ref', '-d', 'refs/heads/feature/test', '1'])
expect(fetchIndex).toBeGreaterThanOrEqual(0)
expect(calls[fetchIndex]?.cwd).toBe('/repo')
expect(fetchIndex).toBeLessThan(mergeTreeIndex)
expect(fetchIndex).toBeLessThan(updateRefIndex)
})
it('preserves an already-merged SSH branch when cleanup races after worktree removal', async () => {
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
let zListCount = 0

View File

@ -1,6 +1,7 @@
import {
branchHasNoUnmergedChangesOnAnyTarget,
getBranchCleanupTargetRefs
getBranchCleanupTargetRefs,
refreshBranchCleanupTargetRefs
} from '../shared/git-branch-cleanup'
import type { GitExec } from './git-handler-ops'
import { parseWorktreeList } from './git-handler-utils'
@ -13,6 +14,7 @@ export async function deleteAlreadyMergedRelayBranchAfterSafeDeleteFailure(
): Promise<boolean> {
const runGit = (args: string[]) => git(args, repoPath)
const targetRefs = await getBranchCleanupTargetRefs(runGit, branchName)
await refreshBranchCleanupTargetRefs(runGit, targetRefs)
// Why: SSH worktrees hit the same squash-merge shape as local worktrees.
// Git's no-op merge proof lets us clean up only branches whose changes
// already exist on the saved base ref.

View File

@ -0,0 +1,45 @@
import { describe, expect, it, vi } from 'vitest'
import { refreshBranchCleanupTargetRefs, type GitBranchCleanupExec } from './git-branch-cleanup'
describe('refreshBranchCleanupTargetRefs', () => {
it('fetches each remote-tracking target remote once and prefers slashed remote names', async () => {
const runGit = vi.fn<GitBranchCleanupExec>(async (args) => {
if (args[0] === 'remote') {
return { stdout: 'origin\nfoo\nfoo/bar\n' }
}
return { stdout: '' }
})
await refreshBranchCleanupTargetRefs(runGit, [
'refs/remotes/origin/main',
'refs/remotes/foo/bar/feature',
'refs/remotes/foo/bar/another',
'HEAD'
])
expect(runGit.mock.calls.map((call) => call[0])).toEqual([
['remote'],
['fetch', '--prune', 'origin'],
['fetch', '--prune', 'foo/bar']
])
})
it('keeps cleanup non-fatal when listing or fetching remotes fails', async () => {
const remoteListFails = vi.fn<GitBranchCleanupExec>().mockRejectedValue(new Error('offline'))
await expect(
refreshBranchCleanupTargetRefs(remoteListFails, ['refs/remotes/origin/main'])
).resolves.toBeUndefined()
const fetchFails = vi.fn<GitBranchCleanupExec>(async (args) => {
if (args[0] === 'remote') {
return { stdout: 'origin\n' }
}
throw new Error('offline')
})
await expect(
refreshBranchCleanupTargetRefs(fetchFails, ['refs/remotes/origin/main'])
).resolves.toBeUndefined()
})
})

View File

@ -37,6 +37,31 @@ export async function getBranchCleanupTargetRefs(
return candidates
}
export async function refreshBranchCleanupTargetRefs(
runGit: GitBranchCleanupExec,
targetRefs: readonly string[]
): Promise<void> {
const remotesStdout = await readOptionalGitStdout(runGit, ['remote'])
const remotes = (remotesStdout ?? '')
.split(/\r?\n/)
.map((line) => line.trim())
.filter((remote) => remote && !remote.startsWith('-'))
.sort((left, right) => right.length - left.length)
const fetchedRemotes = new Set<string>()
for (const targetRef of targetRefs) {
const remote = remotes.find((candidate) => targetRef.startsWith(`refs/remotes/${candidate}/`))
if (!remote || fetchedRemotes.has(remote)) {
continue
}
fetchedRemotes.add(remote)
// Why: deleting a worktree often follows a PR merge. Refresh the saved base
// before deciding a local branch is unpublished, but keep network failures
// non-fatal so offline cleanup preserves today's safe behavior.
await readOptionalGitStdout(runGit, ['fetch', '--prune', remote])
}
}
async function resolveCommitOid(runGit: GitBranchCleanupExec, ref: string): Promise<string | null> {
return readOptionalGitStdout(runGit, ['rev-parse', '--verify', '--quiet', `${ref}^{commit}`])
}