fix(box): worktrees with submodules can be removed when nothing would be lost

Merged
PU
1d ago
+357-4

Summary

git refuses to remove any worktree with a submodule checked out, however clean ("working trees containing submodules cannot be moved or removed"). Only --force gets past it, so in repos with submodules every removal or archive failed, and the app showed a dead-end "Couldn't remove" (its "Remove anyway" toast only matches git's dirty-files wording).

RemoveWorktree (internal/box/locations.go) now catches that refusal on an unforced removal, runs the check git skips, and forces only when nothing would be lost:

  • Changes: git status --porcelain --untracked-files=normal --ignore-submodules=none on the worktree, plus status inside each checked-out submodule, so neither status.showUntrackedFiles nor a submodule's own config can hide anything.
  • History: every submodule repository the removal would delete (the ones under the worktree's modules/, deinitialized ones included, nested ones too, and a checked-out submodule's own embedded .git) is checked with rev-list --all --not --remotes. That covers commits that only a branch, a tag or the stash holds.
  • If git can't answer either check, the worktree is kept.

When something would be lost, the box answers 409 in git's usual "ā€¦, use --force to delete it" form. The app keeps the worktree and offers Remove anyway, and its toast says "commits or stashes no remote has" rather than "uncommitted changes" when that's the reason.

Testing

  • New TestAWorktreeWithSubmodulesIsRemovedWhenNothingWouldBeLost covers each case that must keep the worktree, then checks that a clean one with a deinitialized submodule is removed. The cases: a file in a submodule, the same with untracked files hidden by config, a file in the worktree hidden by config, a submodule stash, an uncommitted and then a committed-but-unpushed submodule commit, a commit only a local tag holds, a branch in a deinitialized submodule, and a commit in a hand-cloned (embedded) submodule. With the embedded lookup turned off, the test fails at that case.
  • go vet ./..., go test -race ./..., cargo check, all the app checks, pnpm test, pnpm build, npx tsc -p e2e and the docs build pass.
  • TestReviewRoutesOverTheWire flakes on TempDir cleanup ("directory not empty"), 3 runs in 10 on both this branch and origin/main. It's unrelated to this change and left alone.

Review

Five Codex review rounds, each by a fresh reviewer:

  1. Fixed (P1): status.showUntrackedFiles=no hid untracked files from the check. Fixed with --untracked-files=normal and status in each submodule. Fixed (P1): submodule stashes weren't checked.
  2. Fixed (P1): deinitialized submodules keep their repository in the worktree's modules/, where foreach never looks. Every repository there is now walked. Fixed (P1): commits that only a local tag holds were missed. The check now uses --all instead of HEAD --branches.
  3. Fixed (P1): a submodule's embedded .git (made with git clone) wasn't checked. Each checked-out submodule's own git dir is now included.
  4. Partly rejected (P2): a local-only tag on an already-pushed commit is removed without asking. Telling a local tag from a fetched one would mean a git ls-remote (network, maybe auth) on every removal, and offline it would refuse every submodule worktree again. Such a tag holds no commits that aren't already on a remote, the same as a local branch on pushed history, whose name is also not kept. The fixed part: the message, toast and changelog had promised to keep "tags", and now say exactly what is kept.
  5. Clean.

After the PR opened:

  • cubic, P1 (rejected, maintainer agreed): ignored files aren't counted, so a submodule worktree whose only extras are ignored files is removed without asking. I left this as it is because it matches git exactly: a plain git worktree remove deletes such a worktree (reproduced), and Shipyard's ordinary path does too. Counting ignored files would refuse nearly every real worktree (node_modules, build output).
  • cubic, P2 (fixed): a "could not be checked" refusal got a plain error with no way forward. It now gets the Kept toast with Remove anyway (keptReason, unit-tested).
  • Codex round 6, P1 (fixed): a deinitialized submodule nested inside a hand-cloned one keeps its repository under that clone's own .git/modules, which nothing scanned. Those are now walked too. A new test fails without the fix.
  • Codex round 7, P2 (fixed): a nested submodule checked out at a different commit could be hidden by submodule.<name>.ignore=all in the submodule above it. Each submodule's own status now uses --ignore-submodules=none. A test covers it and fails without the fix.
  • Codex round 8: clean.
  • cubic, P2 (fixed): keptReason matched bare words (contains, modified), so an unrelated failure could have offered Remove anyway. It now matches the refusal's own words.
  • Merged origin/main. Codex round 9: clean.

šŸ¤– Generated with Claude Code