feat(checkout): route PR checkout through worktree topology - #67
Merged
Conversation
`:AdoPr` / `:AdoPrReview` silently no-op'd when the PR's branch was already
checked out non-detached in another worktree: git's exclusivity lock blocked
the second checkout and `az repos pr checkout` surfaced nothing.
The checkout moves off `az repos pr checkout` entirely to an explicit
`git fetch` of the PR's sourceRefName plus a checkout this plugin controls,
routed on the repo's worktree topology (new lua/ado-pr/worktree.lua):
- One `git worktree list` entry (a plain clone): the branch is checked out in
place, exactly as before. An existing local branch is never force-moved; a
stale one fails loud instead of opening a review of the wrong commits.
- More than one entry: the PR head goes into a dedicated DETACHED review
worktree (`prreview` by default, sibling to the invoking worktree), created
on first use and reused afterwards, and Neovim's cwd moves there once every
fallible step has succeeded. `worktree add --detach` succeeds even when the
branch is checked out non-detached elsewhere.
The routing decision (topology + config -> route + target path) is pure and
unit-tested; the git calls are smoke-tested. Every routing/fetch/checkout
failure aborts with a vim.notify ERROR naming the failed step.
Every git invocation carries `-c submodule.recurse=false`, so an unreachable
submodule can no longer block a review checkout; a PR that moves a submodule
pointer is reported instead as a one-line INFO notice (lua/ado-pr/submodule.lua,
pure detector over `git diff --raw`).
`setup({ review_worktree = ... })` configures the review worktree: a bare name
sits next to the invoking worktree, an absolute path is used as-is.
az.checkout and its cmd.exe-guard test are removed with the call site -- git is
spawned as a real .exe with no cmd.exe re-parse to defend against.
Closes #16
Refs: AB#16
The first routed checkout leaves Neovim's cwd in the review worktree, so the next :AdoPr computes a target equal to where it already is. That case was rejected as a misconfiguration, aborting every checkout after the first -- exactly the reuse the ticket asks for. Treat it as reuse: the existing worktree is found in the listing and checked out into, in place, with no relocation. It is indistinguishable by path from a user who pointed `review_worktree` at their own worktree, so the reuse reading wins. Also add the review-worktree glossary entry to CONTEXT.md. Refs: AB#16
…ore mutating [#g01][#g03]
Two coupled defects in the plain route's existing-local-branch arm.
The command this replaced, `az repos pr checkout --id`, ran fetch -> checkout
-> `git pull <remote> <branch>`: a stale but fast-forwardable local branch was
silently brought up to the PR head and opened. The new code instead checked the
branch out and then ABORTED on any HEAD/head mismatch, so the common
re-review-a-branch-you-already-have case failed outright -- a regression against
acceptance criterion 2 of docs/specs/pr-checkout-robustness.md ("the plain route
behaves exactly as it does today").
Worse, that abort fired AFTER the checkout, leaving the invoking worktree
switched onto the stale branch with no rollback.
Both are fixed by taking the verdict up front. The branch's current tip comes
free from the `rev-parse --verify --quiet refs/heads/<branch>` that already runs
(its stdout IS the tip). When the tip is not the PR head, `merge-base
--is-ancestor <tip> <head>` decides fast-forwardability BEFORE anything is
checked out:
- tip == head -> plain `git checkout <branch>`, unchanged.
- fast-forwardable -> `git checkout <branch>` + `git merge --ff-only <head>`.
- genuinely diverged -> abort naming the branch and both SHAs, nothing run.
Deciding first rather than rolling back is what makes the diverged case safe:
there is no checkout to undo, so no rollback path can itself fail. `--ff-only`
is used rather than `checkout -B` so local commits are never force-discarded and
no merge commit is ever invented; verified empirically, a refused `--ff-only`
does not move HEAD either.
Neither `rev-parse --verify --quiet` nor `merge-base --is-ancestor` goes through
the `run` helper: both use exit 1 as a legitimate answer ("no such branch" /
"not an ancestor"), which `run` would misreport as a command failure. Every call
still carries `-c submodule.recurse=false` via git_argv.
Also retargets four fetch-argv assertions in this spec at git_args(), so they
pin the subcommand and operands rather than the exact `-c` prefix.
A review worktree deleted on disk without `git worktree remove` stays
REGISTERED, and `git worktree list --porcelain` reports it with a
`prunable <reason>` marker line. M.parse_worktrees dropped that line, so the
entry looked like any live worktree, M.route reported `exists = true`, and the
adapter ran `git checkout --detach` with a directory that isn't there as `cwd`.
vim.system RAISES on a spawn failure of that kind rather than returning a result
table, and `run` has no pcall around it, so the user got an unhandled Lua stack
trace instead of this module's promised ERROR notify.
parse_worktrees now records `prunable` per entry and route treats a prunable
match as not existing, so the ordinary "recreate via worktree add" path runs --
which is the right answer anyway: a worktree whose directory is gone should be
recreated, not spawned into. Preferable to wrapping `run` in pcall, which would
only convert the crash into an error about a directory the plugin itself should
have noticed was missing.
`worktree add` alone is not enough: git refuses a path it still has registered
("is a missing but already registered worktree; use 'add -f' to override, or
'prune' or 'remove' to clear" -- reproduced locally, exit 128). The adapter
therefore runs `git worktree prune` first, and only when the listing actually
reported a prunable entry, so the common first-run path gains no extra git call.
The adapter half of this is asserted in tests/worktree_route_spec.lua rather
than the smoke spec because it is meaningless apart from the parser change it
completes -- routing to "absent" without the prune just trades a stack trace for
a spurious hard error.
…rent worktree [#g06] M.parse_worktrees discarded each entry's HEAD state, so M.route had no way to tell a reusable review worktree from a worktree someone actually works in -- it matched on path alone. Point `review_worktree` at an absolute path equal to the current, attached worktree and route reported it as a reusable target; the adapter then ran `git checkout --detach <head>` there, silently detaching the user's real checkout and repointing it at the PR. Path cannot distinguish the two cases, but HEAD state can. A review worktree is always DETACHED, because `worktree add --detach` is what creates it; a worktree a user works in is on a `branch <ref>`. parse_worktrees now records `detached` from the porcelain output, and route refuses to reuse a target that matches a non-prunable, non-detached entry, naming the path and saying reviewing there would detach it. The rule is deliberately not limited to target == current: any attached worktree would be detached the same way, so any attached match is refused. This does not regress the case fd44a30 added -- :AdoPr re-invoked from inside the review worktree, where target == current. That worktree is detached, so it still reads as reuse; tests/review_worktree_spec.lua's "second checkout" case and a new pure case both cover it. Prunable entries are skipped before the check: their directory is gone, so there is nothing there to clobber.
…omments The restructured M.checkout_branch grew a distinct "no local branch yet" arm (`checkout -b <branch> <head>`) that no case reached -- every case in review_base_spec had `rev-parse --verify` succeeding. Covered now, pinning that the branch is created at the fetched PR head and that neither the ff-ability check nor a merge runs when there is no existing tip to compare. Two comments that the preceding three commits made inaccurate: - worktree_route_spec's header claimed the file tests only the pure half; it now carries one deliberate adapter case (the prunable-entry recreation). - review_base_spec called review.lua's fetch "plain"; it carries `-c submodule.recurse=false` too. That guard stays pinned exactly in tests/review_diff_base_spec.lua, which is why the fetch-argv assertions here compare through git_args instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #16.
What
:AdoPr/:AdoPrReviewnow route checkout through a detachedprreviewworktree when other worktrees exist (git's exclusivity lock only blocks a second non-detached checkout of a branch), and behave exactly as before in a plain single-worktree repo. Checkout moves off the opaqueaz repos pr checkoutonto explicitgit fetch+git worktree add/checkout, always with-c submodule.recurse=falseso an unreachable submodule can never block a review again.lua/ado-pr/worktree.lua(new) — pureroute()(topology + config → route + target path, unit-tested) + a thin git adapter (fetch/worktree add/checkout, smoke-tested).lua/ado-pr/submodule.lua(new) — pure submodule pointer-change detector +vim.notifyINFO notice.review.lua— checkout flow reordered: fetch PR payload → resolve route →chdironly once every fallible step has succeeded → open diff → submodule notice.config.lua— newreview_worktreesetup option (default'prreview', bare name or absolute path).az.lua— oldM.checkout(az repos pr checkout) removed.Acceptance criteria
:AdoPron a branch already checked out elsewhere opens via the detachedprreviewworktree instead of no-op'ing.az repos pr checkout's fetch+checkout+pull).prreviewworktree is reused across checkouts in the same session (including when:AdoPris invoked from inside it).setup().vim.notifyERROR naming the failed step.Review
Ran
/review-fix-loop(Fable dimension reviewers + Codex at high reasoning effort, per standing project override). Cycle 1 found and fixed 6 above-floor findings, all closed same session:azCLI's update behavior — fixed to fast-forward instead (verified against the installedazext_devopsCLI source).review.lua's own fetch calls bypassed thesubmodule.recurse=falseguard — closed.prunable) review worktree crashed instead of erroring cleanly — now pruned + recreated.M.open's diff-open call had no rollback on failure — now pcall-guarded with cwd/state rollback.review_worktreecolliding with your own attached worktree silently detached it — now rejected.Full suite green (19/19 spec files),
stylua --checkclean.luacheckunverified locally (not installed on this machine — same as CI-only verification for prior PRs).