mirror of
https://github.com/max-sixty/worktrunk.git
synced 2026-09-14 20:00:38 +08:00
Maximilian Roos
7aba380f0c
`wt remove --force` deleted a live worktree of this same repository, uncommitted work included, whenever that worktree had been moved onto another worktree's registered path. The guard added in #3785 asks which *repository* the occupant answers to: a linked worktree's git dir sits under `<common>/worktrees/`, the main worktree's *is* the common dir, anything else is someone else's. A sibling worktree moved onto the path satisfies that — its git dir sits under `<common>/worktrees/` like any worktree of this repo — so it passed, and the fast path renamed the directory into trash and handed the `rm -rf` to a detached process. It is not prunable either: its gitdir file points at a location that exists, so the `is_prunable` arm from the same PR doesn't catch it. Git's own validation is one level finer. `validate_worktree` requires the directory to point back at *this registration*, and refuses this removal with `--force`: ```console $ git -C repo worktree remove --force ../repo.feature fatal: validation failed, cannot remove working tree: '.../repo.feature' does not point back to '.git/worktrees/repo.feature' ``` <details> <summary>Reproducer, verified against a build of main</summary> The occupant has to be *moved* onto the path rather than created there — `git worktree add` refuses a registered path, which is what leaves a plain `mv` as the way this state arises. ```console $ git -C repo worktree add ../repo.feature -b feature $ git -C repo worktree add ../repo.bar -b bar $ rm -rf ../repo.feature && mv ../repo.bar ../repo.feature $ echo PRECIOUS > ../repo.feature/precious.txt $ wt remove --force --yes feature ◎ Removing feature worktree (--force) & branch in background (same commit as main, _) $ ls ../repo.feature ls: ../repo.feature: No such file or directory ``` </details> ## The fix The gate is now git's comparison at git's granularity: the directory's `.git` must name *this* registration, and that registration's `gitdir` file must name the directory back. Repository-level ownership stays as the weaker half of the conjunction — it is what rejects a `.git` file pointing at another repository — and the main worktree is the same test where there is no registration to point back at. `ensure_belongs_to_repo` becomes `ensure_holds_this_worktree`, since it no longer merely asks about repository membership. Resolution moves to `Repository::git_dir_at`, the fs-only resolver the `wt list` prewarm already used (`derive_worktree_git_dir`), generalized to answer for a directory rather than for a known worktree of this repo: its main-worktree branch returned `git_common_dir()` on trust, and now canonicalizes the `.git` it actually found. It also never walks up to a parent, which is what git reads too — `git rev-parse --git-dir` in an emptied worktree can resolve the *enclosing* repository. That settles a second thing the old docstring got wrong. It claimed the plan→rename window was "narrower than `ensure_clean`'s"; in fact `ensure_clean` re-runs `git status` while this gate answered from `GIT_DIRS`, memoized process-wide, so the second call was vacuous and the window — which contains the approval prompt and the `pre-remove` hook — was unguarded. `git_dir_at` reads the filesystem on every call, so the check at the rename now re-decides. The refusal was `Directory @ … is not this repository's worktree`, which is false in the sibling case: it *is* one of this repository's worktrees, just not the one registered there. Its hint didn't fit either — "move the directory aside, then run `git worktree prune`" is a repo-wide prune, and with a sibling moved aside *both* registrations are prunable, so following it clears both and leaves a live checkout that has stopped being a worktree: ```console $ mv ../repo.feature ../repo.aside && git worktree prune -v Removing worktrees/repo.feature: gitdir file points to non-existent location Removing worktrees/repo.other: gitdir file points to non-existent location $ git -C ../repo.aside status fatal: not a git repository: (null) ``` So the error carries where the occupant's own registration records it, and each case gets the remedy that fits. Moving it back to that path leaves prune with only the stale entry to clear: ```console $ wt remove --force --yes feature ✗ Directory @ ../repo.feature does not hold the worktree registered there ↳ Removing it could destroy the worktree registered @ ../repo.other; move the directory back there, then run git worktree prune ``` That path is read through `canonicalize_with_parents`, because a relative `gitdir` entry resolves against `<common>/worktrees/<id>` and would otherwise reach the hint with the `..` chain still in it — and plain canonicalization can't normalize a directory that no longer exists, which is the case the arm is reached for. Normalizing there also makes `crate::path::paths_match`, the crate's canonicalizing comparison over that same helper, the right test for the gate, so there is no second comparison beside it. The gate's fail-closed behavior now rests on that helper resolving `..` through the filesystem rather than collapsing it lexically — across a symlink the two readings name different directories — so `src/path.rs` records the constraint where a lexical rewrite would otherwise read as a tidy-up. The FAQ's "What can Worktrunk delete?" paragraph carried the same "a *different* repository" framing and is corrected. ## Scope Pre-existing, and 0.73.0 already narrowed it — 0.72.0 had no ownership check at all and deleted foreign clones too. The guard has two call sites (`prepare_worktree_removal` at planning, `stage_worktree_removal` at the rename), so this reaches `wt merge --remove`, `wt step prune`, and picker removal, not only `wt remove`. One incidental tightening: `wt remove <bare-repo-path>` previously passed the guard (a bare root's git dir *is* the common dir) and was stopped only by the dirty check, which `--force` skips. It now refuses at the guard. <details> <summary>One residual, left alone</summary> `git worktree repair <path>` after the `mv` produces a *double registration*: both `worktrees/repo.bar/gitdir` and `worktrees/repo.feature/gitdir` come to record the same path, and `git worktree list` reports two worktrees there. In that state the new gate accepts the removal — the occupant does point at the `feature` registration, and that registration does point back — while git refuses, because its path→worktree lookup happens to match the `bar` entry first. Closing it means knowing the registration id at the gate, or scanning every `worktrees/*/gitdir` for duplicate claims. Unchanged by this PR, and reachable only via `mv` followed by `repair`. </details> ## Testing Five new tests, each confirmed to fail with the line it covers reverted and to leave the others passing. Three drive the binary: - **the sibling case** — follows #3785's data-safety model: asserts the filesystem afterwards, not just the exit code, since removal stages by rename and deletes in a detached process. Snapshots the refusal, so the hint and the path it names are pinned. Fails with the pointer-back conjunct removed, while the foreign-repo test still passes without it — the two cover different halves. - **the re-check at the rename** — a `pre-remove` hook repoints the worktree's `.git` at a sibling's registration after planning has already cleared it, which is what makes the second gate's freshness observable. Fails when resolution routes back through the `GIT_DIRS`-cached `git_dir()`. - **a relative `gitdir` entry** — removal succeeds, and git reads the rewritten entry back, which is what makes it the form git itself writes. Rewriting the entry rather than setting `worktree.useRelativePaths` keeps the test independent of the git version that introduced the option. Two sit at the gate, where the CLI can't reach: - **both worktree shapes are accepted** — including the main worktree, whose git dir *is* the common dir. `wt remove` rejects the main worktree well upstream of this gate and a bare repository's worktrees are all linked, so nothing through the CLI would notice that arm inverting. - **the refusal names a normalized path** — asserted against `Diagnostic::render`, since the path is in the hint and `Display` carries only the title. Local gate green: 4607 tests, lints, doctests, rustdoc under `-Dwarnings`. > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
167 KiB
167 KiB