diff --git a/.reports/dub-61-adversarial-review.md b/.reports/dub-61-adversarial-review.md new file mode 100644 index 00000000..65e4035f --- /dev/null +++ b/.reports/dub-61-adversarial-review.md @@ -0,0 +1,44 @@ +# Adversarial Review - DUB-61 + +Target: staged diff against `origin/main` +Focus: correctness, branch-safety side effects, test coverage +Review artifact: `/tmp/adversarial-review-h0FXjd/diff.patch` + +## Summary + +Iterations: 1 + +- Critical: 0 remaining +- Major: 0 remaining +- Minor: 0 remaining +- Nitpick: 0 remaining + +## Review Notes + +Reviewer A focus: command safety and mutation ordering. + +- Confirmed the shared guard runs before undo entries, cleanup journals, state + writes, branch rewrites, submit pushes, and PR mutations for the mutating + paths covered by DUB-61. +- Flagged one no-op nuance in `move`: refusing a branch checked out elsewhere + before the existing no-op return would make a non-mutating no-op stricter than + necessary. Fixed before this report by moving the guard after the no-op + branch. + +Reviewer B focus: coverage and user-facing recovery. + +- Confirmed the new regression suite covers every named Tier 3 command plus + submit with real sibling git worktrees. +- Confirmed the `DubError.recovery` hints include the exact worktree path. +- Confirmed existing fold parent-worktree coverage was updated to the shared + error wording. + +## Remaining Findings + +None. + +## Verification After Review + +- `pnpm checks` passed. +- `pnpm typecheck` passed. +- `pnpm test` passed. diff --git a/.reports/dub-61-qa.md b/.reports/dub-61-qa.md new file mode 100644 index 00000000..98adf5ee --- /dev/null +++ b/.reports/dub-61-qa.md @@ -0,0 +1,33 @@ +# Self-QA fallback - DUB-61 + +> This work item has no useful browser recording surface, so this file replaces +> the video and records deterministic proof instead. + +## Why no video + +DUB-61 changes TypeScript CLI branch-safety behavior only. No `.tsx` files or +browser-demoable surfaces changed, so the useful proof is command-level test +coverage and repo gates. + +## What was verified + +- A shared worktree guard refuses branch mutations before undo, journal, state, + push, or PR side effects. +- Tier 3 commands `split`, `absorb`, `squash`, `fold`, `pop`, `rename`, `move`, + `reorder`, and `unlink` refuse when their target branch is checked out in a + sibling worktree. +- `submit` refuses before pushing when a branch in its resolved submit scope is + checked out in a sibling worktree. +- The `DubError` recovery hints include the exact sibling worktree path. + +## Evidence + +- `pnpm checks` passed. +- `pnpm typecheck` passed. +- `pnpm test` passed: 124 test files, 1354 tests. +- Focused regression coverage: `packages/cli/src/commands/worktree-guards.test.ts` + passed all 11 scenarios using real git worktrees. + +## Follow-up flag + +None. diff --git a/packages/cli/src/commands/absorb.ts b/packages/cli/src/commands/absorb.ts index e3c13c8b..c8c4838d 100644 --- a/packages/cli/src/commands/absorb.ts +++ b/packages/cli/src/commands/absorb.ts @@ -31,6 +31,7 @@ import { topologicalOrder, } from '../lib/state'; import { saveUndoEntry } from '../lib/undo-log'; +import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards'; import { restack } from './restack'; export type AbsorbMode = 'auto' | 'ai' | 'stack'; @@ -146,6 +147,14 @@ export async function absorb( const mode: AbsorbMode = options.stack ? 'stack' : options.ai ? 'ai' : 'auto'; + if (!options.dryRun && mode !== 'stack') { + await assertBranchesNotCheckedOutElsewhere( + cwd, + [originalBranch], + 'dub absorb', + ); + } + await saveUndoEntry( { operation: 'absorb', @@ -385,6 +394,12 @@ async function runStackMode( }; } + await assertBranchesNotCheckedOutElsewhere( + cwd, + crossFixups.flatMap((fixup) => [fixup.sourceBranch, fixup.targetBranch]), + 'dub absorb --stack', + ); + await writeAbsorbProgress(cwd, { mode: 'stack', originalBranch, diff --git a/packages/cli/src/commands/move.test.ts b/packages/cli/src/commands/move.test.ts index a2089d57..a6d6610c 100644 --- a/packages/cli/src/commands/move.test.ts +++ b/packages/cli/src/commands/move.test.ts @@ -9,6 +9,7 @@ vi.mock('../lib/git.js', async () => { getBranchTip: vi.fn(), getCurrentBranch: vi.fn(), isWorkingTreeClean: vi.fn(), + listWorktreeCheckouts: vi.fn(), }; }); @@ -52,6 +53,7 @@ import { getBranchTip, getCurrentBranch, isWorkingTreeClean, + listWorktreeCheckouts, } from '../lib/git'; import { checkGhAuth, @@ -88,6 +90,7 @@ function makeState(specs: BranchSpec[]): DubState { beforeEach(() => { vi.clearAllMocks(); vi.mocked(isWorkingTreeClean).mockResolvedValue(true); + vi.mocked(listWorktreeCheckouts).mockResolvedValue(new Map()); vi.mocked(branchExists).mockResolvedValue(true); vi.mocked(getCurrentBranch).mockResolvedValue('main'); vi.mocked(getBranchTip).mockImplementation(async (name) => `sha:${name}`); diff --git a/packages/cli/src/commands/move.ts b/packages/cli/src/commands/move.ts index d34d4452..097bbad9 100644 --- a/packages/cli/src/commands/move.ts +++ b/packages/cli/src/commands/move.ts @@ -26,6 +26,7 @@ import { writeState, } from '../lib/state'; import { saveUndoEntry } from '../lib/undo-log'; +import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards'; import { restack } from './restack'; export interface MoveOptions { @@ -195,6 +196,11 @@ export async function move( noOpReason: reason, }; } + await assertBranchesNotCheckedOutElsewhere( + cwd, + [branch, target, ...reparents.map((reparent) => reparent.branch)], + 'dub move', + ); // Validate the planned mutation is acyclic on a clone before touching disk. const probeStack: Stack = structuredClone(stack); diff --git a/packages/cli/src/commands/pop.ts b/packages/cli/src/commands/pop.ts index e98e04f0..356babae 100644 --- a/packages/cli/src/commands/pop.ts +++ b/packages/cli/src/commands/pop.ts @@ -8,6 +8,7 @@ import { } from '../lib/git'; import { getParent, readState } from '../lib/state'; import { saveUndoEntry } from '../lib/undo-log'; +import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards'; interface PopOptions { /** Number of commits to pop. Defaults to 1. */ @@ -63,6 +64,7 @@ export async function pop( "Run 'dub log' to inspect the stack and confirm tracking state.", ]); } + await assertBranchesNotCheckedOutElsewhere(cwd, [branch], 'dub pop'); const branchCommitCount = await countCommitsAhead(branch, parent, cwd); if (branchCommitCount === 0) { diff --git a/packages/cli/src/commands/rename.ts b/packages/cli/src/commands/rename.ts index 73927645..bca6a2c2 100644 --- a/packages/cli/src/commands/rename.ts +++ b/packages/cli/src/commands/rename.ts @@ -5,7 +5,6 @@ import { getCurrentBranch, isValidBranchName, lastPushedRef, - listWorktreeCheckouts, pushBranch, readLastPushedSha, renameBranch, @@ -13,6 +12,7 @@ import { } from '../lib/git'; import { findStackForBranch, readState, writeState } from '../lib/state'; import { clearUndoEntry, saveUndoEntry } from '../lib/undo-log'; +import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards'; interface RenameOptions { /** @@ -119,20 +119,7 @@ export async function rename( ]); } - // Refuse to rename a branch that's checked out in another worktree — - // `git branch -m` would fail mid-operation, and undo would point to a - // rename that never happened. - const worktreeCheckouts = await listWorktreeCheckouts(cwd); - const otherWorktree = worktreeCheckouts.get(oldName); - if (otherWorktree) { - throw new DubError( - `Branch '${oldName}' is checked out in another worktree (${otherWorktree}).`, - [ - `Run 'git worktree remove ${otherWorktree}' to drop the worktree, then retry.`, - `Switch off '${oldName}' in the other worktree, then retry.`, - ], - ); - } + await assertBranchesNotCheckedOutElsewhere(cwd, [oldName], 'dub rename'); const childBranches = sourceStack.branches.filter( (b) => b.parent === oldName, diff --git a/packages/cli/src/commands/reorder.ts b/packages/cli/src/commands/reorder.ts index 1a7d41aa..227118f2 100644 --- a/packages/cli/src/commands/reorder.ts +++ b/packages/cli/src/commands/reorder.ts @@ -8,12 +8,10 @@ import chalk from 'chalk'; import { DubError } from '../lib/errors'; import { execa } from '../lib/exec'; import { - formatWorktreeCheckoutSkipMessage, getBranchTip, getCurrentBranch, getMergeBase, isWorkingTreeClean, - listWorktreeCheckouts, } from '../lib/git'; import { buildRebaseTodo, @@ -27,6 +25,7 @@ import { import { rollbackRestack } from '../lib/restack-rollback'; import { findStackForBranch, getParent, readState } from '../lib/state'; import { saveUndoEntry } from '../lib/undo-log'; +import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards'; import { restack } from './restack'; /** @@ -175,27 +174,11 @@ export async function reorder( ); } - // Defence in depth: refuse to reorder a branch git reports as also checked - // out in another worktree. In practice git enforces that the same branch - // can't be checked out twice (and `getCurrentBranch` already errored on - // detached HEAD above), so this branch is rarely reached — keeping it - // guards against forced/inconsistent setups (e.g. `worktree add --force` - // followed by a manual ref edit) before `git rebase -i` would otherwise - // fail with a confusing message (Tier 3 pattern §5). - const worktreeCheckouts = await listWorktreeCheckouts(cwd); - const otherWorktree = worktreeCheckouts.get(currentBranch); - if (otherWorktree) { - throw new DubError( - `Cannot reorder '${currentBranch}': branch is checked out in another worktree.`, - [ - formatWorktreeCheckoutSkipMessage( - currentBranch, - otherWorktree, - 'dub reorder', - ), - ], - ); - } + await assertBranchesNotCheckedOutElsewhere( + cwd, + [currentBranch], + 'dub reorder', + ); const parent = getParent(state, currentBranch); if (!parent) { diff --git a/packages/cli/src/commands/split.ts b/packages/cli/src/commands/split.ts index d30f5330..c016eb20 100644 --- a/packages/cli/src/commands/split.ts +++ b/packages/cli/src/commands/split.ts @@ -60,6 +60,7 @@ import { readState, writeState, } from '../lib/state'; +import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards'; import { restack } from './restack'; export type SplitMode = 'by-commit' | 'by-file' | 'by-hunk' | 'ai'; @@ -191,6 +192,7 @@ export async function split( ]); } const parentBranch = sourceMeta.parent; + await assertBranchesNotCheckedOutElsewhere(cwd, [sourceBranch], 'dub split'); const parentTip = await getBranchTip(parentBranch, cwd); const sourceTipBefore = await getBranchTip(sourceBranch, cwd); const existingPrNumber = sourceMeta.pr_number ?? null; diff --git a/packages/cli/src/commands/squash.ts b/packages/cli/src/commands/squash.ts index 2dc764a0..3878d08f 100644 --- a/packages/cli/src/commands/squash.ts +++ b/packages/cli/src/commands/squash.ts @@ -20,6 +20,7 @@ import { } from '../lib/git'; import { getParent, readState } from '../lib/state'; import { withTempMarkdownFile } from '../lib/temp-text-file'; +import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards'; import { restack } from './restack'; export interface SquashOptions { @@ -114,6 +115,7 @@ export async function squash( "Run 'dub log' to inspect the stack and confirm tracking state.", ]); } + await assertBranchesNotCheckedOutElsewhere(cwd, [branch], 'dub squash'); const commitCount = await countCommitsAhead(branch, parent, cwd); if (commitCount <= 1) { diff --git a/packages/cli/src/commands/submit.test.ts b/packages/cli/src/commands/submit.test.ts index c4913bf8..7c8c4152 100644 --- a/packages/cli/src/commands/submit.test.ts +++ b/packages/cli/src/commands/submit.test.ts @@ -9,6 +9,7 @@ vi.mock('../lib/git.js', () => ({ getDiff: vi.fn(), getDiffBetween: vi.fn(), getLastCommitMessage: vi.fn(), + listWorktreeCheckouts: vi.fn(), pushBranch: vi.fn(), })); @@ -53,6 +54,7 @@ import { getDiff, getDiffBetween, getLastCommitMessage, + listWorktreeCheckouts, pushBranch, } from '../lib/git'; import { @@ -83,6 +85,9 @@ const mockGetDiffBetween = getDiffBetween as ReturnType; const mockGetLastCommitMessage = getLastCommitMessage as ReturnType< typeof vi.fn >; +const mockListWorktreeCheckouts = listWorktreeCheckouts as ReturnType< + typeof vi.fn +>; const mockPushBranch = pushBranch as ReturnType; const mockEnsureGhInstalled = ensureGhInstalled as ReturnType; const mockCheckGhAuth = checkGhAuth as ReturnType; @@ -196,6 +201,7 @@ beforeEach(() => { mockGetDiff.mockResolvedValue('diff --git a/file.ts b/file.ts'); mockGetDiffBetween.mockResolvedValue('diff --git a/file.ts b/file.ts'); mockGetLastCommitMessage.mockResolvedValue('feat: existing title'); + mockListWorktreeCheckouts.mockResolvedValue(new Map()); mockAddPrReviewers.mockResolvedValue(undefined); mockGetRepositoryWebUrl.mockResolvedValue('https://github.com/o/r'); mockMarkPrReady.mockResolvedValue(undefined); diff --git a/packages/cli/src/commands/submit.ts b/packages/cli/src/commands/submit.ts index ab3c8f5a..ab6fe670 100644 --- a/packages/cli/src/commands/submit.ts +++ b/packages/cli/src/commands/submit.ts @@ -57,6 +57,7 @@ import { writeState, } from '../lib/state'; import { withTempMarkdownFile } from '../lib/temp-text-file'; +import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards'; /** @deprecated Use SubmitScope. Retained for the v1 `--path` deprecation window. */ export type SubmitPathMode = 'current' | 'stack'; @@ -179,6 +180,13 @@ export async function submit( } const plan = await getSubmitPlan(cwd, options); + if (!dryRun) { + await assertBranchesNotCheckedOutElsewhere( + cwd, + plan.branches.map((branch) => branch.name), + 'dub submit', + ); + } const config = await readConfig(cwd); const lifecycle = await resolveSubmitLifecycle( cwd, diff --git a/packages/cli/src/commands/unlink.test.ts b/packages/cli/src/commands/unlink.test.ts index 8f97c037..82d5cd04 100644 --- a/packages/cli/src/commands/unlink.test.ts +++ b/packages/cli/src/commands/unlink.test.ts @@ -7,6 +7,7 @@ vi.mock('../lib/git.js', async () => { ...actual, getCurrentBranch: vi.fn(), isWorkingTreeClean: vi.fn(), + listWorktreeCheckouts: vi.fn(), }; }); @@ -41,7 +42,11 @@ import { clearCleanupJournal, startCleanupJournal, } from '../lib/cleanup-journal'; -import { getCurrentBranch, isWorkingTreeClean } from '../lib/git'; +import { + getCurrentBranch, + isWorkingTreeClean, + listWorktreeCheckouts, +} from '../lib/git'; import { checkGhAuth, ensureGhInstalled, @@ -76,6 +81,7 @@ function makeState(specs: BranchSpec[]): DubState { beforeEach(() => { vi.clearAllMocks(); vi.mocked(isWorkingTreeClean).mockResolvedValue(true); + vi.mocked(listWorktreeCheckouts).mockResolvedValue(new Map()); vi.mocked(getCurrentBranch).mockResolvedValue('main'); vi.mocked(writeState).mockResolvedValue(undefined); vi.mocked(startCleanupJournal).mockResolvedValue({ diff --git a/packages/cli/src/commands/unlink.ts b/packages/cli/src/commands/unlink.ts index 39c159ea..fe649f9b 100644 --- a/packages/cli/src/commands/unlink.ts +++ b/packages/cli/src/commands/unlink.ts @@ -21,6 +21,7 @@ import { writeState, } from '../lib/state'; import { saveUndoEntry } from '../lib/undo-log'; +import { assertBranchesNotCheckedOutElsewhere } from '../lib/worktree-guards'; export interface UnlinkOptions { /** Skip PR retargeting; print a warning that the PR base is now out of sync. */ @@ -110,6 +111,7 @@ export async function unlink( `Run 'dub untrack ${branch}' to drop the root from tracking entirely.`, ]); } + await assertBranchesNotCheckedOutElsewhere(cwd, [branch], 'dub unlink'); const previousParent = entry.parent; if (!previousParent) { diff --git a/packages/cli/src/commands/worktree-guards.test.ts b/packages/cli/src/commands/worktree-guards.test.ts new file mode 100644 index 00000000..f8af40de --- /dev/null +++ b/packages/cli/src/commands/worktree-guards.test.ts @@ -0,0 +1,171 @@ +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { + createTestRepo, + gitInRepo, + withBranchWorktree, +} from '../../test/helpers'; +import { DubError } from '../lib/errors'; +import { foldBranch } from '../lib/fold'; +import { absorb } from './absorb'; +import { create } from './create'; +import { init } from './init'; +import { move } from './move'; +import { pop } from './pop'; +import { rename } from './rename'; +import { reorder } from './reorder'; +import { split } from './split'; +import { squash } from './squash'; +import { submit } from './submit'; +import { unlink } from './unlink'; + +let dir: string; +let cleanup: () => Promise; + +async function writeAndCommit( + cwd: string, + file: string, + contents: string, + message: string, +): Promise { + fs.writeFileSync(path.join(cwd, file), contents); + await gitInRepo(cwd, ['add', file]); + await gitInRepo(cwd, ['commit', '-m', message]); +} + +async function expectWorktreeRefusal( + branch: string, + command: string, + run: () => Promise, +): Promise { + await withBranchWorktree(dir, branch, async (worktreeDir) => { + await expect(run()).rejects.toMatchObject({ + message: `Cannot run '${command}': branch '${branch}' is checked out in another worktree.`, + recovery: expect.arrayContaining([ + `The other worktree is '${worktreeDir}'.`, + ]), + }); + }); +} + +beforeEach(async () => { + const repo = await createTestRepo(); + dir = repo.dir; + cleanup = repo.cleanup; + await init(dir); + await gitInRepo(dir, ['add', '.']); + await gitInRepo(dir, ['commit', '-m', 'init dubstack']); +}); + +afterEach(async () => { + await cleanup(); +}); + +describe('worktree guards for mutating commands', () => { + it('split refuses when the source branch is checked out elsewhere', async () => { + await create('feat/a', dir); + await writeAndCommit(dir, 'a.txt', 'a', 'feat: add a'); + + await expectWorktreeRefusal('feat/a', 'dub split', () => + split(dir, { + mode: 'by-file', + files: ['a.txt'], + name: 'feat/split-a', + }), + ); + }); + + it('absorb refuses when the current branch is checked out elsewhere', async () => { + await create('feat/a', dir); + await writeAndCommit(dir, 'a.txt', 'a', 'feat: add a'); + await writeAndCommit(dir, 'a.txt', 'aa', 'fixup! feat: add a'); + + await expectWorktreeRefusal('feat/a', 'dub absorb', () => absorb(dir)); + }); + + it('squash refuses when the current branch is checked out elsewhere', async () => { + await create('feat/a', dir); + await writeAndCommit(dir, 'a.txt', 'a', 'feat: add a'); + await writeAndCommit(dir, 'b.txt', 'b', 'feat: add b'); + + await expectWorktreeRefusal('feat/a', 'dub squash', () => squash(dir)); + }); + + it('fold refuses when the folded branch is checked out elsewhere', async () => { + await create('feat/a', dir); + await writeAndCommit(dir, 'a.txt', 'a', 'feat: add a'); + await create('feat/b', dir); + await writeAndCommit(dir, 'b.txt', 'b', 'feat: add b'); + + await expectWorktreeRefusal('feat/b', 'dub fold', () => foldBranch(dir)); + }); + + it('pop refuses when the current branch is checked out elsewhere', async () => { + await create('feat/a', dir); + await writeAndCommit(dir, 'a.txt', 'a', 'feat: add a'); + + await expectWorktreeRefusal('feat/a', 'dub pop', () => pop(dir)); + }); + + it('rename refuses when the source branch is checked out elsewhere', async () => { + await create('feat/a', dir); + await writeAndCommit(dir, 'a.txt', 'a', 'feat: add a'); + + await expectWorktreeRefusal('feat/a', 'dub rename', () => + rename(dir, 'feat/a', 'feat/renamed-a'), + ); + }); + + it('move refuses when a reparented branch is checked out elsewhere', async () => { + await create('feat/a', dir); + await writeAndCommit(dir, 'a.txt', 'a', 'feat: add a'); + await create('feat/b', dir); + await writeAndCommit(dir, 'b.txt', 'b', 'feat: add b'); + + await expectWorktreeRefusal('feat/b', 'dub move', () => + move(dir, 'feat/b', { after: 'main' }), + ); + }); + + it('reorder refuses when the current branch is checked out elsewhere', async () => { + await create('feat/a', dir); + await writeAndCommit(dir, 'a.txt', 'a', 'feat: add a'); + await writeAndCommit(dir, 'b.txt', 'b', 'feat: add b'); + + await expectWorktreeRefusal('feat/a', 'dub reorder', () => reorder(dir)); + }); + + it('unlink refuses when the target branch is checked out elsewhere', async () => { + await create('feat/a', dir); + await writeAndCommit(dir, 'a.txt', 'a', 'feat: add a'); + + await expectWorktreeRefusal('feat/a', 'dub unlink', () => + unlink(dir, 'feat/a'), + ); + }); + + it('submit refuses when a branch in scope is checked out elsewhere', async () => { + await create('feat/a', dir); + await writeAndCommit(dir, 'a.txt', 'a', 'feat: add a'); + + await expectWorktreeRefusal('feat/a', 'dub submit', () => + submit(dir, false, { noAi: true }), + ); + }); + + it('throws a DubError with the worktree path in recovery hints', async () => { + await create('feat/a', dir); + await writeAndCommit(dir, 'a.txt', 'a', 'feat: add a'); + + await withBranchWorktree(dir, 'feat/a', async (worktreeDir) => { + try { + await pop(dir); + throw new Error('expected pop to refuse'); + } catch (error) { + expect(error).toBeInstanceOf(DubError); + expect((error as DubError).recovery.join('\n')).toContain(worktreeDir); + } + }); + }); +}); diff --git a/packages/cli/src/lib/fold.ts b/packages/cli/src/lib/fold.ts index c48c87d2..1672b627 100644 --- a/packages/cli/src/lib/fold.ts +++ b/packages/cli/src/lib/fold.ts @@ -3,13 +3,11 @@ import { checkoutBranch, deleteLocalBranch, fastForwardBranchToRef, - formatWorktreeCheckoutSkipMessage, getBranchTip, getCommitSubjectsBetween, getCurrentBranch, getMergeBase, isWorkingTreeClean, - listWorktreeCheckouts, mergeSquashAndCommit, } from './git'; import { assertStateInvariants } from './invariants'; @@ -21,6 +19,7 @@ import { type Stack, writeState, } from './state'; +import { assertBranchesNotCheckedOutElsewhere } from './worktree-guards'; export interface FoldPreview { branch: string; @@ -201,35 +200,11 @@ export async function foldBranch( parent: parentName, } = await resolveFoldTarget(cwd, options.branch); - const worktreeCheckouts = await listWorktreeCheckouts(cwd); - const branchWorktree = worktreeCheckouts.get(branchName); - if (branchWorktree) { - throw new DubError( - `Cannot fold '${branchName}': it is checked out in another worktree.`, - [ - formatWorktreeCheckoutSkipMessage( - branchName, - branchWorktree, - 'dub fold', - ), - `Close the worktree at '${branchWorktree}' or fold from that worktree instead.`, - ], - ); - } - const parentWorktree = worktreeCheckouts.get(parentName); - if (parentWorktree) { - throw new DubError( - `Cannot fold into '${parentName}': it is checked out in another worktree.`, - [ - formatWorktreeCheckoutSkipMessage( - parentName, - parentWorktree, - 'dub fold', - ), - `Close the worktree at '${parentWorktree}' or run 'dub fold' from that worktree.`, - ], - ); - } + await assertBranchesNotCheckedOutElsewhere( + cwd, + [branchName, parentName], + 'dub fold', + ); const parentTip = await getBranchTip(parentName, cwd); const branchTip = await getBranchTip(branchName, cwd); diff --git a/packages/cli/src/lib/worktree-guards.ts b/packages/cli/src/lib/worktree-guards.ts new file mode 100644 index 00000000..b7c376ce --- /dev/null +++ b/packages/cli/src/lib/worktree-guards.ts @@ -0,0 +1,49 @@ +import { DubError } from './errors'; +import { listWorktreeCheckouts } from './git'; + +export interface WorktreeCheckoutConflict { + branch: string; + worktree: string; +} + +/** + * Refuses commands that would mutate a branch checked out in another worktree. + * + * Git rejects many of these operations mid-command. Surfacing the conflict + * before writing undo/journal/state keeps recovery straightforward. + */ +export async function assertBranchesNotCheckedOutElsewhere( + cwd: string, + branches: Iterable, + command: string, +): Promise { + const conflict = await findWorktreeCheckoutConflict(cwd, branches); + if (!conflict) return; + + throw new DubError( + `Cannot run '${command}': branch '${conflict.branch}' is checked out in another worktree.`, + [ + `The other worktree is '${conflict.worktree}'.`, + `Run '${command}' from that worktree, or switch that worktree off '${conflict.branch}' and retry.`, + ], + ); +} + +export async function findWorktreeCheckoutConflict( + cwd: string, + branches: Iterable, +): Promise { + const checkouts = await listWorktreeCheckouts(cwd); + const seen = new Set(); + + for (const branch of branches) { + if (seen.has(branch)) continue; + seen.add(branch); + const worktree = checkouts.get(branch); + if (worktree) { + return { branch, worktree }; + } + } + + return null; +} diff --git a/packages/cli/test/commands/fold-tree.test.ts b/packages/cli/test/commands/fold-tree.test.ts index d67cb84b..75ff0e04 100644 --- a/packages/cli/test/commands/fold-tree.test.ts +++ b/packages/cli/test/commands/fold-tree.test.ts @@ -368,7 +368,7 @@ describe('dub fold', () => { await expect( fold(dir, { branch: 'feat/child', force: true }), ).rejects.toThrow( - /Cannot fold into 'feat\/base'.*checked out in another worktree/s, + /Cannot run 'dub fold': branch 'feat\/base' is checked out in another worktree/, ); } finally { await gitInRepo(dir, ['worktree', 'remove', '--force', worktreePath]); diff --git a/packages/cli/test/helpers.ts b/packages/cli/test/helpers.ts index 87ababf7..6f6643bd 100644 --- a/packages/cli/test/helpers.ts +++ b/packages/cli/test/helpers.ts @@ -85,3 +85,22 @@ export async function attachBareRemote(localDir: string): Promise<{ }, }; } + +export async function withBranchWorktree( + dir: string, + branch: string, + run: (worktreeDir: string) => Promise, +): Promise { + const safeBranch = branch.replace(/[^a-zA-Z0-9_-]+/g, '-'); + const worktreeDir = `${dir}-${safeBranch}-worktree`; + + await gitInRepo(dir, ['worktree', 'add', '--force', worktreeDir, branch]); + try { + return await run(await fs.promises.realpath(worktreeDir)); + } finally { + await gitInRepo(dir, ['worktree', 'remove', '--force', worktreeDir]).catch( + () => undefined, + ); + await fs.promises.rm(worktreeDir, { recursive: true, force: true }); + } +}