docs(worktree): correct three specs the selector refactor left stale - #3786
Conversation
Each describes a mechanism the refactor changed, and each is wrong in a way a reader would act on rather than merely notice. `worktree_is_unusable` closed with "`false` for a path git has no registration for". The `!path.exists()` early return makes that untrue as a claim about the return value — such a path answers `true` when it is simply gone. The sentence was only ever about the `prunable` lookup, so it says so, and records that no caller reaches the combination. `normalize_selector` named three call sites, two of which no longer call it, and spent a paragraph explaining the string-comparison design the refactor deleted — documenting a removed mechanism as current. It now names its one home and says what made the single home possible. `ResolvedTarget::selector` credited only a rewrite with taking the path arm off, missing `--create`, which takes it off without rewriting anything.
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
The worktree_is_unusable and ResolvedTarget::selector rewrites both check out against the code — three callers for the former, all taking their path out of the listing (usable_worktree_for_branch, plan_switch via resolve_selector, recover::hint_for_repo), and branch_only() under --create for the latter.
One claim in the normalize_selector replacement overstates in the same shape the PR is fixing. "which every worktree selector reaches before resolution" isn't structurally true: resolve_switch_target returns out of the pr:/mr: arm before it ever calls expand_selector, so resolve_pr_target/resolve_remote_ref hand resolve_selector a Selector::rewritten_to(...) that never passed through normalization — as does resolve_target_selector(None), which wraps the cached default branch. That's correct behaviour (there's no typed token there to normalize), but a reader adding a fifth construction site would take the sentence as a guarantee the type doesn't give them, which is the failure mode the PR body names.
The same sentence's "That one home" also sits awkwardly one clause after naming two application sites; the suggestion folds both.
"every worktree selector reaches `expand_selector`" was itself the shape this change is fixing. `resolve_switch_target` returns out of its `pr:`/`mr:` arm first, and `resolve_target_selector(None)` wraps the cached default branch, so both hand back a `Selector::rewritten_to` that never passed through normalization. Correct behaviour — neither was a spelling anyone typed — but a reader adding a construction site would have read the sentence as a guarantee the type does not give.
2 participants
Three specs in #3785's selector refactor describe mechanisms that change removed or altered. Comment-only; no behavior change.
Each is wrong in a way a reader would act on rather than merely notice:
worktree_is_unusableclosed with "falsefor a path git has no registration for". The!path.exists()early return makes that untrue as a claim about the return value — such a path answerstruewhen it is simply gone. The sentence was only ever about theprunablelookup, so it now says so, and records that no caller reaches the combination (all three take their path out of the listing).normalize_selectornamed three call sites, two of which no longer call it, and spent a paragraph explaining the string-comparison design the refactor deleted — documenting a removed mechanism as current, which is the worst of the three. It now names its one home and says what made a single home possible.ResolvedTarget::selectorcredited only a rewrite with taking the path arm off, missing--create, which takes it off without rewriting anything.The first was raised on #3785 after I had worked its other threads, so it never got a reply there. The other two came from re-reading the neighbouring specs while fixing it.
Why these were worth a change rather than a note
A spec that describes a deleted mechanism is worse than no spec:
normalize_selector's explained why each assembly of the resolution ladder had to normalize for itself, which was true beforeSelectorcarried "may this token name a path?" as a fact rather than a string comparison. A reader adding a fourth entry point would have followed it and re-introduced the per-site normalization the refactor removed.