fix(remove): refuse to strand a detached worktree when removing by branch - #3791
fix(remove): refuse to strand a detached worktree when removing by branch#3791max-sixty wants to merge 4 commits into
Conversation
…anch Detaching a worktree's HEAD severs the only link git records between it and the branch, so `wt remove <branch>` dropped out of the branch-first lookup, degraded to a branch-only deletion, and exited 0 — deleting the ref and leaving the worktree registered. The failing half was silent. The `worktree-path` template is what still connects the two, so the removal matches against it and refuses, naming the path that reaches the worktree. Three states are deliberately not matched: a worktree on some other branch (that branch names it), the main worktree (whose path-based removal refuses too, so the hint would be a dead end), and a prunable entry (stale metadata, not a worktree on disk). The guard sits in `wt remove` rather than `prepare_worktree_removal`, which every producer of a branch-only target shares: `wt step prune` plans its whole sweep from one worktree-list snapshot, so a detached worktree it is about to remove as its own candidate is still registered when the branch's plan is built — refusing there would leave prune unable to clean up either half. `live_sibling_checkout` keeps `exists()` rather than the union predicate, and now says why: the two only disagree on a directory that is present but no longer holds its worktree, where calling it dead deletes a branch a checkout still resolves. Fixes #3769 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
Holding off on approval per the repo's data-loss policy: the diff edits src/commands/remove.rs (wt remove, the branch-deletion path) and src/git/error.rs's removal hints. The change narrows what gets deleted rather than widening it, but the deletion surface is a human's call — normally I'd request review from @max-sixty, who is the author here.
The guard itself checks out. compute_worktree_path returns the repo root for the default branch in a non-bare repo, so the main worktree does land at the expected path and is_linked() is what excludes it; unwrap_or(true) failing closed there is the right direction. I traced the wt step prune justification too — a live detached worktree enters the sweep as CheckSource::Linked with wt.head as its integration ref while the orphan branch enters as CheckSource::Orphan, so both halves really are candidates in one snapshot.
The hint double-escapes its path. Inline suggestion on src/git/error.rs. format_path_for_display already returns a shell-ready token, and suggest_command escapes it again — so the rendered hint is wt remove '~/repo.feature', not the wt remove ~/repo.feature this PR's description shows. For a path that genuinely needs escaping it stops being cosmetic. Note the same shape already exists outside this diff, in the "gained a worktree since it was selected" bail in prepare_worktree_removal (src/commands/repository_ext.rs, the arm matching RemoveTarget::BranchOnly that calls suggest_command("remove", &[&path], &[]) on a format_path_for_display result) — happy to push a commit fixing both if you want them to move together.
#3770 is still open. This supersedes it, but nothing has closed it yet, so the two now sit in the list with identical titles.
One residual asymmetry, for your judgment rather than a change request: prune reaches the stranded state this guard refuses to create, when the two halves disagree on integration. If the detached worktree's HEAD is unmerged (not removable) but the branch it used to hold is integrated, the orphan-branch candidate is deleted and the detached worktree stays registered — branch gone, worktree left, which is the shape the new error calls out. The docstring's reasoning covers the case where prune can clean up both; this is the case where it cleans up only the branch.
`format_path_for_display` already returns a shell-ready token, so routing
its result through `suggest_command` escapes it a second time. A worktree
under $HOME rendered as `wt remove '~/repo.feature'`, where the quoting
suppresses the tilde expansion the unquoted form in the line above it
relies on; a path that genuinely needs escaping came out carrying literal
quote characters and resolved to nothing.
The three hints that composed the two now interpolate the formatted path
directly, as the neighbouring `rm -rf {path}` and `git worktree unlock
{path}` hints already do. `format_path_for_display` documents the trap so
the next caller doesn't repeat it — `pr_mr_switch_hint` had recorded it
only in its own docstring.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
Two behaviors #3770's last review round turned up in the identical guard, neither of which is in this diff — flagging them here since this is the version that lands. Both were built and pinned on fix/issue-3769 (94f7f8b0, f1f29b61), green on test (linux|macos|windows) plus codecov/patch, so they carry over as-is.
1. The guard preempts the branch-existence check, and claims a branch that isn't there. prepare_worktree_removal's branch-only arm is what reports a typo or a remote-only name (the exists_locally() / RemoteOnlyBranch block in src/commands/repository_ext.rs). detached_worktree_for fires ahead of it, so a name that is not a local branch but whose templated path holds a detached worktree reports a branch that doesn't exist:
$ git branch -D feature # detach first, then drop the ref
$ wt remove feature
✗ Branch feature has no worktree; the one @ /tmp/repro/myproject.feature is detachedWithout the detached worktree at that path the same command says No branch named feature. Fix is one check at the top of detached_worktree_for:
// Downstream, `prepare_worktree_removal` is what reports a typo, a deleted
// branch, or a remote-only name; a guard that fires ahead of it would
// assert a branch that doesn't exist. `exists_locally` reports a failed
// lookup as `false` rather than an error, so this can't fail closed — but
// the same call downstream then refuses the removal, so a guard skipped
// that way still never deletes a ref.
if !repo.branch(branch).exists_locally().unwrap_or(true) {
return None;
}
let expected = compute_worktree_path(repo, branch, config).ok()?;2. It fires when the removal wasn't going to delete the ref. Branch-only removal under --no-delete-branch (or [remove] delete-branch = false) deletes nothing — it prints ○ No worktree found for branch … and exits 0. The guard makes it exit 1, so for anyone running delete-branch = false every wt remove <branch> whose worktree has been detached becomes a hard failure, protecting nothing. deletion_mode is already in scope at the call site:
if let Some(detached) = worktrees
.filter(|_| !deletion_mode.should_keep())
.and_then(|wts| detached_worktree_for(repo, config, &branch, wts))
{The cost of that gate is that what comes back is the ○ No worktree found for branch feature line, which is inaccurate for exactly the reason #3769 opens with — making it accurate without making it fatal is a separate message-design call.
One case the gate's reasoning covers but deliberately doesn't gate: an unintegrated branch under SafeDelete retains its ref too (BranchDeletionOutcome::NotDeleted, exit 0), but only the downstream deletion attempt knows that — Repository::integration_reason isn't free at guard time — so those refuse at exit 1 rather than exiting 0. That's a behavior change worth naming in a comment; refusing is the conservative direction since the path it names is still on disk.
Tests
Three on fix/issue-3769, all snapshot-based alongside the existing detached-worktree cases in tests/integration_tests/remove.rs:
test_remove_missing_branch_with_detached_worktree_at_its_path— detach,git branch -D, thenwt remove <name>reports✗ No branch named …with the branch-list hint.test_remove_branch_with_detached_worktree_keeping_branch—--no-delete-branchexits 0 with○ No worktree found for branch ….test_remove_unmerged_branch_with_detached_worktree_refuses— pins theSafeDelete-unmerged refusal at exit 1.
…e a ref The guard fired ahead of `prepare_worktree_removal`'s branch-existence check, so a name that is not a local branch but whose templated path holds a detached worktree reported `Branch <name> has no worktree` instead of `No branch named <name>`. Check `exists_locally` first. It also fired under `--no-delete-branch` / `[remove] delete-branch = false`, where the branch-only arm deletes nothing — nothing was going to strand the worktree, so the guard turned a no-op exit 0 into a hard failure. Gate on `deletion_mode.should_keep()`. An unintegrated branch under `SafeDelete` retains its ref too, but only the deletion attempt downstream knows that, so those still refuse rather than exiting 0. That is the conservative direction — the worktree the refusal names is on disk — and a test pins it as a decision rather than an accident. Co-Authored-By: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
Both behaviors from the last round are fixed and pinned, and #3770 is closed now, so that observation is stale. The exists_locally gate lands ahead of compute_worktree_path, and gating on !deletion_mode.should_keep() reads correctly against the config path too — handle_remove_command folds [remove] delete-branch into delete_branch before passing !delete_branch as keep_branch, so Keep covers the config form as well as the flag.
The help text still describes the refusal as unconditional. Inline suggestion on src/cli/mod.rs. The added paragraph in the Branch cleanup section says wt remove <branch> "refuses instead and names the path" — written when the guard was unconditional. After this push, someone running delete-branch = false gets ○ No worktree found for branch <name> and exit 0, which is the exact message #3769 opens with. The section is about branch deletion, so context does most of the work, but the sentence is the one place a delete-branch = false user would go looking, and it currently promises them a protection they don't get. Applying the suggestion desyncs the three rendered mirrors and help_remove_long.snap — cargo run -- hook pre-merge --yes regenerates both sets.
Minor, take it or leave it: detached_worktree_for now runs rev-parse --verify refs/heads/<branch> before the template match, and prepare_worktree_removal's branch-only arm runs the same lookup a few lines downstream, so every branch-only removal pays it twice. Ordering the existence check after the find is semantically identical — all three are ANDed predicates over the same inputs — and skips the subprocess entirely in the common case where no detached worktree sits at the templated path. The comment explaining why the check exists reads the same in either position.
…e a ref The paragraph was written when the guard was unconditional, so it promised a refusal that `--no-delete-branch` and `[remove] delete-branch = false` turn off — and those are the users most likely to go looking, since what they actually see is the inaccurate `○ No worktree found for branch …` that #3769 opens with. Co-Authored-By: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
The reworded paragraph matches the guard's gate — !deletion_mode.should_keep() is what makes --no-delete-branch / [remove] delete-branch = false fall through to the no-op, and all three rendered mirrors plus help_remove_long.snap moved with src/cli/mod.rs in the same commit. That was the last open finding; nothing new on this push.
Still not approving, per the repo's data-loss policy for the wt remove deletion surface — same hold as my first review, not a fresh concern.
wt remove <branch>on a worktree whose HEAD has since been detached deleted the branch, left the worktree registered, and exited 0. Detaching severs the only link git records between a worktree and its branch, so the branch-first lookup found nothing and the removal degraded to a branch-only deletion — half the operation, and the half that runs silently.The
worktree-pathtemplate is what still connects the two after a detach, so the removal matches against it and refuses, naming the path that reaches the worktree:Three states are deliberately not matched, because refusing on them would name a removal that can't happen or protect nothing: a worktree checked out on some other branch (that branch names it, so removing this one strands nothing), the main worktree (whose path-based removal refuses too, so the hint would be a dead end — the default-branch refusal stays the accurate answer), and a prunable entry (stale metadata for
wt step prune, not a worktree left on disk). Each has a test.The guard sits in
wt removerather than inprepare_worktree_removal, which every producer of a branch-only target shares.wt step pruneplans its whole sweep from one worktree-list snapshot, so a detached worktree it is about to remove as its own candidate is still registered when the branch's plan is built; refusing there would leave prune unable to clean up either half.Also folds in a docstring fix on
live_sibling_checkout, which keepsexists()rather than theworktree_is_unusableunion predicate the rest of the removal path uses. The two only disagree on a directory that is present but no longer holds its worktree, and the disagreement is asymmetric — calling a dead sibling live retains a branch nobody needed, while calling a live one dead deletes a branch a checkout still resolves. Since that predicate only ever gates a deletion, it takes the conservative test; the docstring now says so.Also fixes a double-escape the reviewer caught in the hint this PR adds.
format_path_for_displayalready returns a shell-ready token, so routing it throughsuggest_commandescaped it a second time — a worktree under$HOMErendered aswt remove '~/repo.feature', where the quoting suppresses the tilde expansion the unquoted form on the line above relies on, and a path needing real escaping came out carrying literal quote characters and resolved to nothing. Two pre-existing hints compose the same pair (prepare_worktree_removal's "gained a worktree" bail andwarn_duplicate_checkout); all three now interpolate the formatted path directly, as the neighbouringrm -rf {path}andgit worktree unlock {path}hints already do.format_path_for_displaydocuments the trap, which until now was recorded only inpr_mr_switch_hint's own docstring.Two further cases the reviewer found, both places the guard fired where the removal was never going to delete a ref. It ran ahead of
prepare_worktree_removal's branch-existence check, so a name that is not a local branch but whose templated path holds a detached worktree reportedBranch <name> has no worktreeinstead ofNo branch named <name>; and it fired under--no-delete-branch, where the branch-only arm deletes nothing, turning a no-op exit 0 into a hard failure for anyone runningdelete-branch = false. Both are gated now, and both were built on #3770 first (94f7f8b09, f1f29b61f) — that work is carried here with its tests.One case is deliberately left refusing: an unintegrated branch under
SafeDeletekeeps its ref too, but only the deletion attempt downstream knows that, so the guard can't tell it apart at plan time without paying forintegration_reason. Refusing is the conservative direction — the worktree it names is on disk — andtest_remove_unmerged_branch_with_detached_worktree_refusespins it as a decision rather than an accident.Relationship to #3770
Supersedes #3770, whose design this takes: the template match, the
DetachedWorktreeForBrancherror, the guard's placement, and three of the four tests. That branch wentCONFLICTINGwhen #3785 and #3786 renamedpath_selector_errorand folded its return intoResolvedWorktree, so the arm it patches no longer exists in that shape — this is the same fix re-derived against currentmain.One claim in it needed correcting. It documented the main-worktree exclusion as universal: "a detached main worktree matches the default branch, whose removal already reports the accurate
CannotRemoveDefaultBranch". That holds for non-bare repos, whereis_linked()excludes the main worktree. A bare repo has no main worktree, so its default-branch checkout is matched — verified by hand, and it refuses safely with a working hint. The bullet now gives the reason for the exclusion (the hint would otherwise name a command that refuses) rather than an invariant that doesn't hold.Testing
test_remove_branch_whose_worktree_was_detachedpins the state (refusal, branch survives, worktree stays registered) and is mutation-verified — invertingwt.branch.is_none()makes it fail with the exact output from the issue. Three snapshots cover the message and the two non-matched states. One existing snapshot changed:test_remove_detached_worktree_in_multiwas pinning the bug, and its branch survived only because it happened to be unmerged.Fixes #3769
Thanks to @chachi for the report — the repro runs exactly as written, and the diagnosis in it (the branch→worktree lookup, not detached removal) is what this fixes.