fix(relocate): skip worktree blocked by an immovable occupant - #3530
Conversation
When a worktree's target is occupied by another worktree that is itself blocked (its own target is a non-worktree path with no --clobber), the dependent could never reach its target. The no-progress branch treated it as a cycle, temp-moved it into the staging dir, and finalize then failed moving it into the still-occupied target — erroring out and stranding the worktree in .git/wt/staging/relocate/. Detect a blocked occupant and propagate the block to its dependents so they are skipped cleanly instead of temp-moved.
The guard's only caller (is_target_empty's Some(false) arm) already guarantees the target exists, and canonicalize's fallback returns None for an absent path anyway — so the early return was dead code and the one line codecov/patch flagged as uncovered. Remove it.
worktrunk-bot
left a comment
There was a problem hiding this comment.
The fix is correct: blocked_occupant cleanly diverts a dependent away from break_cycle when its occupant is in the blocked set, and setting made_progress = true re-drives the loop so a longer chain (A → B → C-blocked) blocks transitively. Traced the classification-then-execute ordering, the Some(false) arm, and the finalize stranding path — all sound. This is a self-review, so submitting as a comment rather than an approval.
One coverage note: test_relocate_blocked_occupant_skips_dependent is a 2-level chain (alpha → beta-blocked), where beta is already in blocked from new()'s classification. In that shape alpha is blocked on the first pass, and nothing else remains — so the test still passes even if made_progress = true were removed (the loop would fall straight into break_cycle, find everything blocked, and return false). The transitive re-drive the PR body calls out is only exercised by a 3+ level chain (A → B → C-blocked), where B must be blocked in one pass before A can be blocked in the next. Consider adding a 3-worktree variant so a future refactor can't silently drop the re-drive without a failing test. Not blocking — happy to push that test if useful.
The existing test_relocate_blocked_occupant_skips_dependent is a 2-level chain (alpha -> beta-blocked) where alpha is blocked on the first pass, so it passes even without the made_progress re-drive. This adds a 3-level chain (alpha -> beta -> gamma-blocked) that forces the transitive re-drive to be load-bearing: alpha is parked at a path (aaa-alpha) whose basename sorts before beta's repo.alpha, so worktree-list order visits the dependent before its occupant. That leaves alpha pending after pass 1 and only the re-drive blocks it in pass 2. Without the re-drive, break_cycle temp-moves alpha and finalize misplaces it into the occupied repo.alpha.
|
Pushed the 3-level chain test in b1c85e6 — but building it surfaced a wrinkle worth recording: a naive Worktrees are processed in Verified it's genuinely load-bearing: with |
Problem
wt step relocatecould strand a worktree in the internal staging directory and then error out.When worktree A's target is occupied by worktree B, A depends on B vacating. But if B is itself blocked — its own target is a non-worktree path and
--clobberwasn't passed — B never moves. The dependency-resolution loop's no-progress branch assumes "no progress ⟹ a cycle to break," so it temp-moved A into.git/wt/staging/relocate/.finalize_temp_relocationsthen rangit worktree move <temp> <A's target>into the still-occupied path, which fails and propagates via?— leaving A relocated into the staging dir rather than at either its original or expected path.Reproduction
/w/awants/w/b; B at/w/bwants/w/c;/w/cis a plain non-worktree file, no--clobber.target_blockedand never moves.break_cycle, thenfinalizefails with "Failed to move worktree from temp to final location", stranding A in staging.Fix
is_target_emptyonly distinguished "occupant moved" from "occupant present"; it couldn't tell a still-pending occupant from a permanently-blocked one. A newblocked_occupanthelper detects when a dependent's target is held by a worktree already in theblockedset, and the loop propagates the block to the dependent (skipping it cleanly) instead of handing it tobreak_cycle. Settingmade_progressre-drives the loop so a chain of dependents (A → B → C-blocked) is blocked transitively.After the fix both worktrees are skipped and the command succeeds:
Test
test_relocate_blocked_occupant_skips_dependentsets up exactly this shape and asserts the command succeeds, both worktrees stay put, and nothing is stranded in.git/wt/staging/relocate/. Before the fix the command exited non-zero andalphawas left in the staging dir, so both the snapshot (success: true) and the location assertions would have failed.Found during the nightly code-quality survey of
src/commands/relocate.rs.