refactor: unify subdir preservation for wt step relocate - #3346
Conversation
max-sixty asked (in #3344) whether the subdirectory-preservation logic could be unified so every path-switching command behaves the same. `wt switch` and `wt remove`/`wt merge` already share `resolve_subdir_in_target` (merge lands via the same handler). The one remaining path that hand-rolled its own `dest.join(relative)` was `wt step relocate`. Route relocate's shell `cd` through the same helper, keeping its "only cd when the user is inside the moving worktree" guard. This also gains the helper's symlink canonicalization (relocate previously did a raw non-canonical `strip_prefix`) and the read-only `is_dir()` fallback (a no-op here since `git worktree move` carries the whole subtree, but keeps behavior identical across commands). Adds `test_relocate_preserves_subdir`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
The unification is clean and the calling convention matches the switch/remove sites. One note on the stated benefits, since it's easy to over-read: the symlink-canonicalization gain doesn't actually materialize for relocate. git worktree move runs at relocate.rs:531, before the cd block at 538–550, so by the time resolve_subdir_in_target calls dunce::canonicalize on src_path and cwd, neither path exists on disk anymore — both canonicalize calls fail and fall back to the raw paths (unwrap_or_else). So relocate effectively runs the same raw strip_prefix it did before, just with the added is_dir() fallback.
That's fine — it degrades gracefully to the prior behavior and the is_dir() fallback is a strict improvement — but the PR body frames symlink survival as an active gain here, and it isn't. switch/remove canonicalize before their source is gone, so they get it; relocate can't. Worth trimming that bullet so a future reader doesn't assume relocate survives symlinked cwds when it still relies on the pre-existing starts_with/strip_prefix string match.
No code change needed — the behavior is correct.
|
The red What fails. On Windows, renaming a directory tree fails with a sharing violation if any live process holds a current directory inside that tree. Here the spawned Why The part I could not verify from CI. Relocate takes its
I'm not pushing either change since I can't confirm the real-user Windows behavior end-to-end from here — flagging it so the direction is a deliberate call. Linux and macOS (including the advisory legs) are green, so the logic itself is sound; this is purely the Windows move-while-inside constraint. |
test_relocate_preserves_subdir stands the spawned wt inside the worktree being moved — the only way to exercise subdir preservation on relocate, since the cd fires only when cwd is inside src. On Windows, git worktree move is a directory rename that fails with a sharing violation while a live process holds that cwd, so the test cannot run there. Unlike remove (where shell integration cds to main before removing), a real Windows user hits the same wall: their shell holds the cwd across the move. Gate the test with #[cfg_attr(windows, ignore)], matching the existing remove.rs pattern, and document the boundary rather than mask it by having wt release its own cwd — that would go green while the real case still fails. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The branch was 135 commits behind main and pinned skim 4.10.0, so its Windows CI had drifted red on ~40 `switch_picker` PTY tests failing with "Keyboard progressive enhancement not implemented for the legacy Windows API" — unrelated to this PR, which touches only relocate/handlers. Merging main brings the branch current (skim 5.3.1 and the picker fixes that go with it), clearing those failures. The PR's own change survives the merge: `resolve_subdir_in_target` stays `pub(crate)` and relocate still routes its cd through it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The What actually fails now: the branch's Root cause is #3526 (skim 5.1.0 → 5.3.1), not this PR. Last green Windows run on Already being fixed on main: #3537 ( Evidence
|
…indows (#3538) ## Problem skim 5.3.1 breaks the picker on the legacy Windows console. Every picker PTY test on `test (windows)` fails with: ``` ✗ interactive picker failed: Keyboard progressive enhancement not implemented for the legacy Windows API. ``` That is a crossterm keyboard-enhancement error: the picker (via skim 5.3.1) drives keyboard-enhancement handling that the legacy Windows console API doesn't support, crashing the picker at runtime. ~40 `switch_picker` tests fail as a result. ## Evidence it's skim 5.3.1 - **#3526 (bump skim 5.1.0 → 5.3.1) merged with its own `full-tests (windows)` already red** — the regression landed on main unnoticed. - **main at skim 5.1.0 (`0958a365b`) was green on `test (windows)` the same day.** The only relevant change between that green run and the failures is the skim bump. - The failure is a picker runtime crash, unrelated to any picker source change — no `switch_picker` code moved. Since it's on main, it reddens Windows CI for every open PR (e.g. #3346), not just one branch. ## Fix Pin `skim = "=5.1.0"` in `Cargo.toml` — the last Windows-green release — with a comment naming the regression. `Cargo.toml` specified `skim = "5.0"` (a caret range that `5.3.1` satisfies), so #3526 changed **only** the lockfile. A lock-only revert would silently drift back to 5.3.1 on the next `cargo update`, so the hold has to live in the manifest. The lockfile downgrade (skim 5.3.1 → 5.1.0, frizbee 0.11.0 → 0.10.0 and the deps that pulls) follows from the pin. Unpin once skim's keyboard-enhancement path is guarded for the legacy Windows console (upstream fix, or a worktrunk-side guard before enabling enhancement). ## Testing - `cargo run -- hook pre-merge --yes` — full suite green locally (4475 passed, 1 skipped), including the `switch_picker` tests that fail on Windows at 5.3.1. - CI will confirm `test (windows)` returns to green. > _This was written by Claude Code on behalf of Maximilian Roos_ Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Context
Follow-up to #3344, where @max-sixty asked:
Current state
I traced every site that emits a
cdto move the shell between worktrees (change_directorycall sites):wt switch/switch --createhandlers.rs:931resolve_subdir_in_target✅wt remove/wt mergehandlers.rs:1622resolve_subdir_in_target✅wt step relocaterelocate.rs:543dest.join(relative)❌So
wt mergewas already unified — it lands via the same handler aswt remove, so #3344 gave it subdir preservation for free. The one remaining outlier that hand-rolled its own logic waswt step relocate.Change
Route relocate's shell
cdthrough the sharedresolve_subdir_in_targethelper, keeping its existing "only cd when the user is inside the moving worktree" guard (cwd.starts_with(src)) — the helper alone would wrongly cd a user who wasn't inside.The concrete behavior change is the
is_dir()fallback: the old code did an unconditionaldest.join(relative), so a missing subdir at the destination would have produced acdto a nonexistent path; the helper falls back to the worktree root instead. It's a no-op in practice (git worktree movecarries the whole subtree, so the subdir always exists at the destination), but it keeps the behavior identical across all three commands.Note the helper's symlink canonicalization does not materialize for
relocate.git worktree moveruns (relocate.rs:531) before thiscdblock, so by the time the helper canonicalizessrc_pathandcwd, neither exists on disk — bothdunce::canonicalizecalls fail and fall back to the raw paths. Relocate therefore keeps its pre-existing rawstarts_with/strip_prefixstring match and, unlikeswitch/remove(which canonicalize while their source still exists), does not survive symlinked cwds. No behavior regression — it degrades gracefully to the prior string-matching behavior.The helper is now
pub(crate)with an updated docstring naming its three callers.Windows scoping
test_relocate_preserves_subdiris gated to Unix (#[cfg_attr(windows, ignore)]). Subdir preservation only fires when the cwd is inside the moving worktree, and that is exactly whengit worktree move— a directory rename — fails on Windows with a sharing violation, because a live process holds that cwd.This is not a test artifact. Unlike
remove(where shell integration cds to main before removing), a real Windows user standing inside a worktree they relocate hits the same wall: their shell holds the cwd across the move. Releasing wt's own cwd would turn the test green while the real case still failed, so the honest fix is to scope the test — and the inside-standing feature it covers — to the platforms where it is reachable, matching the existing#[cfg_attr(windows, ignore)]pattern inremove.rs.An unrelated skim regression briefly reddened
test (windows)While this branch was in flight, main's
test (windows)broke on ~40switch_pickertests with "Keyboard progressive enhancement not implemented for the legacy Windows API" — a skim 5.3.1 regression on the legacy Windows console (bumped in #3526). That was fixed on main by #3538 (pinning skim to 5.1.0), which this branch now carries via the merge ofmain. It never involved this change, which touches only relocate andhandlers.rs— none of the picker.Testing
test_relocate_preserves_subdir(Unix-only — see Windows scoping) — relocating a worktree from insideapps/gateway/lands thecddirective in the equivalent subdir at the new location.resolve_subdir_in_targetand theswitch/removesubdir integration tests continue to pass.relocatetest filter: 26 passed, 0 failed.🤖 Generated with Claude Code