fix(worktree): offer Force Delete with a plain warning when a checkout has submodules (#28878)

* fix(worktree): remove a clean checkout that contains submodules

Git refuses to remove any worktree with an initialized submodule unless --force is set, even when nothing is uncommitted. After that specific refusal, re-check the checkout including files inside the submodule and pass --force once. A dirty submodule still blocks the delete. A second --force is not added, because that would also clear a Git lock.

* fix(worktree): offer Force Delete for a checkout Git refuses over submodules

Drop the automatic --force retry. A forced removal deletes the submodule's
repository under .git/worktrees/<id>/modules, including commits, local
branches and stashes that were never pushed, and the parent's dirty check
cannot see them (the retry #24602 removed for the same reason).

Instead classify Git's "working trees containing submodules cannot be moved
or removed" refusal as a new Force Delete reason, 'submodules', after the
lock check and the force guard. The renderer classifies the error string,
so older SSH and paired hosts get it too with no wire change. The toast,
the delete dialog and the Space manager row show one plain warning naming
what Force Delete would lose instead of Git's text.

Keep --ignore-submodules=none in the pre-teardown dirty check so a user's
ignore setting cannot hide submodule edits from it.

Replace the retry test with one real-Git test: a clean checkout whose
submodule holds a local-only commit is refused without --force, stays
registered, keeps the commit, and its refusal classifies as 'submodules'.

* fix(worktree): clarify the submodule Force Delete warning and hide View for it

The warning now says Git deletes the workspace only with Force Delete, the submodule
refusal no longer offers View (there are no changes to show), the real-Git test pins
that Force Delete then succeeds, and a comment no longer describes the removed retry.

* fix(worktree): name submodules when a changed submodule blocks the delete, and clear a stale delete toast

With a commit inside a submodule that the parent has not recorded, the parent sees `M vendor`, so
Orca's dirty check refused first with the generic "It has changed files". Force Delete then destroyed
the never-pushed submodule commit without warning. On a dirty refusal the check now asks
`git status --porcelain=v2` whether a changed entry is a submodule, and if so reports a refusal the
app shows with the submodule Force Delete warning. The detail keeps the dirty sentence, so older
apps still offer Force Delete.

After the Space manager deleted a workspace (its row Force, or a later delete), the failure toast for
that workspace stayed up and still offered Force Delete. A Space delete now dismisses it.

* fix(worktree): warn about submodules on any refused delete of a checkout that has them

Review found the warning still vanished when a submodule commit was already recorded and some other
file was dirty: the parent's status shows no submodule change, yet Force Delete destroys the commit.
The changed-files check now asks whether the checkout has a populated submodule (a .gitmodules path
holding a .git, as Git's own refusal checks) instead of whether one changed. The refusal keeps the
changed paths after the detail, and View stays available since there are changes to view.

* test(worktree): mock the .gitmodules probe as missing in the preflight unit tests

* refactor(worktree): attach the dirty-check status without a type assertion

* fix(worktree): key the delete failure toast by host and worktree id

A Space delete on one host must not dismiss another host's failure toast for a worktree with the same id.

* fix(worktree): dismiss the failure toast by the store row's host after a Space row Force
This commit is contained in:
Brennan Benson
2026-10-16 15:23:13 -07:00
committed by GitHub
parent be033c2ab2
commit 23b60a5d07
14 changed files with 223 additions and 29 deletions
@@ -43,7 +43,8 @@ beforeEach(() => {
describe('assertWorktreeCleanForRemoval', () => {
beforeEach(() => {
gitExecFileAsyncMock.mockReset()
// Calls after the status read are the dirty refusal's `.gitmodules` probe: no file, exit 1.
gitExecFileAsyncMock.mockReset().mockRejectedValue(new Error('git config exited with code 1'))
})
it('returns without checking git status for force removals', async () => {
@@ -57,7 +58,7 @@ describe('assertWorktreeCleanForRemoval', () => {
await expect(assertWorktreeCleanForRemoval('/repo-feature')).resolves.toBeUndefined()
expect(gitExecFileAsyncMock).toHaveBeenCalledWith(
['status', '--porcelain', '--untracked-files=all'],
['status', '--porcelain', '--untracked-files=all', '--ignore-submodules=none'],
{ cwd: '/repo-feature', timeout: WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS }
)
})
@@ -81,7 +82,7 @@ describe('assertWorktreeCleanForRemoval', () => {
).resolves.toBeUndefined()
expect(gitExecFileAsyncMock).toHaveBeenCalledWith(
['status', '--porcelain', '-z', '--untracked-files=all'],
['status', '--porcelain', '-z', '--untracked-files=all', '--ignore-submodules=none'],
{ cwd: '/repo-feature', timeout: WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS }
)
})
@@ -94,7 +95,7 @@ describe('assertWorktreeCleanForRemoval', () => {
).rejects.toThrow('git timed out.')
expect(gitExecFileAsyncMock).toHaveBeenCalledWith(
['status', '--porcelain', '--untracked-files=all'],
['status', '--porcelain', '--untracked-files=all', '--ignore-submodules=none'],
{ cwd: '/repo-feature', timeout: 25 }
)
})
+54 -6
View File
@@ -1,3 +1,6 @@
import { stat } from 'node:fs/promises'
import { join } from 'node:path'
import { SUBMODULE_DIRTY_REMOVAL_DETAIL } from '../../shared/worktree/removal'
import { gitExecFileAsync } from './runner'
import type { WorktreeRemovalPreflightOptions } from './worktree-operation-options'
import { WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS, gitExecOptions } from './worktree-operation-options'
@@ -17,7 +20,14 @@ export async function assertWorktreeCleanForRemoval(
const { ignoredUntrackedPaths = [], ...gitOptions } = options
const useNullTerminatedStatus = ignoredUntrackedPaths.length > 0
const { stdout } = await gitExecFileAsync(
['status', '--porcelain', ...(useNullTerminatedStatus ? ['-z'] : []), '--untracked-files=all'],
[
'status',
'--porcelain',
...(useNullTerminatedStatus ? ['-z'] : []),
'--untracked-files=all',
// Why: a user's ignore-submodules setting would hide submodule edits from this dirty check.
'--ignore-submodules=none'
],
{
...gitExecOptions(worktreePath, gitOptions),
timeout: gitOptions.timeout ?? WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS
@@ -32,15 +42,53 @@ export async function assertWorktreeCleanForRemoval(
return
}
const error = new Error('Worktree has uncommitted or untracked changes.')
const blockingStatus = blockingEntries ? blockingEntries.join('\n') : stdout
if (await hasPopulatedSubmodule(worktreePath, gitOptions)) {
// Why: Force Delete also destroys the submodules' never-pushed commits, which the parent's
// status cannot show, so the refusal must say submodules. Status lines go after the detail
// because the formatter prefers stdout over the message.
throw new Error(`${SUBMODULE_DIRTY_REMOVAL_DETAIL}\n${blockingStatus.trim()}`)
}
// Why not the raw stdout: `-z` output is NUL-delimited and `.trim()` leaves
// interior NULs, so attaching it verbatim put raw control bytes into the
// user-facing removal error — and listed the tolerated shared link, the one
// entry that is not the user's work and cannot be committed away.
;(error as Error & { stdout?: string }).stdout = blockingEntries
? blockingEntries.join('\n')
: stdout
throw error
throw Object.assign(new Error('Worktree has uncommitted or untracked changes.'), {
stdout: blockingStatus
})
}
/** Git's main submodule refusal test: a `.gitmodules` path whose directory holds a `.git`. */
async function hasPopulatedSubmodule(
worktreePath: string,
gitOptions: Omit<WorktreeRemovalPreflightOptions, 'ignoredUntrackedPaths'>
): Promise<boolean> {
try {
const { stdout } = await gitExecFileAsync(
['config', '-z', '--file', '.gitmodules', '--get-regexp', '^submodule\\..*\\.path$'],
{
...gitExecOptions(worktreePath, gitOptions),
timeout: gitOptions.timeout ?? WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS
}
)
const paths = stdout
.split('\0')
.map((entry) => entry.slice(entry.indexOf('\n') + 1))
.filter(Boolean)
const populated = await Promise.all(
paths.map((path) =>
stat(join(worktreePath, path, '.git')).then(
() => true,
() => false
)
)
)
return populated.includes(true)
} catch {
// No .gitmodules exits non-zero. Any failure keeps the plain dirty refusal, which still blocks.
return false
}
}
/** The `git status --porcelain -z` entries that genuinely block removal:
+64 -1
View File
@@ -9,7 +9,9 @@ import { performance } from 'node:perf_hooks'
import { promisify } from 'node:util'
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
import { removeTree } from '../../shared/windows-transient-lock-removal'
import { listWorktreesStrict, removeWorktree } from './worktree'
import { classifyWorktreeForceDeleteReason } from '../../shared/worktree/removal'
import { formatWorktreeRemovalError } from '../ipc/worktree-logic'
import { assertWorktreeCleanForRemoval, listWorktreesStrict, removeWorktree } from './worktree'
import { areWorktreePathsEqual } from './worktree-path-comparison'
import { isPrunableGitFileWorktree } from '../worktree-prunable-git-file'
import { removeStaleLocalWorktreeRegistration } from '../local-worktree-removal-recovery'
@@ -95,6 +97,67 @@ describe('worktree removal against the real Git binary', () => {
expect(await isRegistered(worktreePath)).toBe(true)
})
// A checkout whose initialized submodule holds a commit only it has; the parent does not record it.
async function addSubmoduleCheckoutWithLocalCommit(): Promise<{
subWorktreePath: string
vendorPath: string
localCommit: string
}> {
const emptyCommit = ['-c', 'user.name=o', '-c', 'user.email=o@example.invalid', 'commit']
const originPath = join(scratchDir, 'origin')
await mkdir(originPath)
await git(['init', '-q'], originPath)
await git([...emptyCommit, '-q', '--allow-empty', '-m', 'origin'], originPath)
await git(
['-c', 'protocol.file.allow=always', 'submodule', 'add', '-q', originPath, 'vendor'],
repoPath
)
await git(['commit', '-qm', 'add vendor'], repoPath)
const subWorktreePath = join(workspaceRoot, 'repo', 'with-sub')
await git(['worktree', 'add', '-q', subWorktreePath, '-b', 'with-sub'], repoPath)
await git(
['-c', 'protocol.file.allow=always', 'submodule', 'update', '-q', '--init'],
subWorktreePath
)
const vendorPath = join(subWorktreePath, 'vendor')
await git([...emptyCommit, '-q', '--allow-empty', '-m', 'local only'], vendorPath)
const localCommit = (await git(['rev-parse', 'HEAD'], vendorPath)).trim()
return { subWorktreePath, vendorPath, localCommit }
}
it('refuses a clean checkout with a submodule and offers Force Delete for it', async () => {
// Why: --force would delete submodule commits that exist only in this checkout, so Git's
// refusal must reach the user as a Force Delete choice rather than be retried away.
const { subWorktreePath, vendorPath, localCommit } = await addSubmoduleCheckoutWithLocalCommit()
// Recorded by the parent so its status stays clean.
await git(['commit', '-qam', 'bump vendor'], subWorktreePath)
const refusal = await removeWorktree(repoPath, subWorktreePath, false).catch((error) => error)
expect(refusal).toBeInstanceOf(Error)
expect(
classifyWorktreeForceDeleteReason(formatWorktreeRemovalError(refusal, subWorktreePath, false))
).toBe('submodules')
expect(await isRegistered(subWorktreePath)).toBe(true)
expect((await git(['cat-file', '-t', localCommit], vendorPath)).trim()).toBe('commit')
await removeWorktree(repoPath, subWorktreePath, true)
expect(await isRegistered(subWorktreePath)).toBe(false)
})
it('names submodules when the dirty check refuses a checkout that has them', async () => {
// Why: the parent's status shows only its own file, yet Force Delete destroys the submodule commit.
const { subWorktreePath } = await addSubmoduleCheckoutWithLocalCommit()
await git(['commit', '-qam', 'bump vendor'], subWorktreePath)
await writeFile(join(subWorktreePath, 'notes.txt'), 'draft\n')
const refusal = await assertWorktreeCleanForRemoval(subWorktreePath).catch((error) => error)
const message = formatWorktreeRemovalError(refusal, subWorktreePath, false)
expect(classifyWorktreeForceDeleteReason(message)).toBe('submodules')
expect(message).toContain('?? notes.txt')
// Older apps offer Force Delete only for this sentence.
expect(message).toContain('Worktree has uncommitted or untracked changes')
})
it('does not delete a checkout through a malformed registration that names its git file', async () => {
const markerPath = join(worktreePath, '.git')
const marker = await readFile(markerPath, 'utf8')