docs: record round 2's third P2 and the async-continuation census
The diff channel's missing ticket, and --- because this was the fourth recurrence of one shape --- a census of every async continuation in `git.lua` rather than another instance-by-instance note. Three continuations, one dispatcher, one synchronous impostor, each with whether it carries an invocation-time ticket, whether it needs one, and what shared state it writes. Also states why the fix is two channels sharing one mechanism rather than one shared counter: a single counter would make `d` cancel an in-flight `g`. And records that `state.diff_buffer` is deliberately still read at continuation time --- "do I already have a live diff buffer?" is a question about now, not about the invocation --- so it is not a fifth instance. P1a's citations re-pointed at `723afa7` and the untouched claim tightened from "no diff line reaches these names" to something checkable: `show_diff_buffer`'s body and `open_status_panel`'s `listview.open` are byte-identical to `4002734`, and no commit on this branch adds a `commit_to` call anywhere. Section-local: nothing outside this lane's entry is touched or reflowed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai
This commit is contained in:
parent
723afa717f
commit
7fe32e43f6
|
|
@ -269,7 +269,7 @@ census.
|
||||||
|
|
||||||
**PR #227** — https://github.com/levineuwirth/pmacs/pull/227. Opened
|
**PR #227** — https://github.com/levineuwirth/pmacs/pull/227. Opened
|
||||||
2026-08-09 at `4002734`, after the framing was approved at revision 5
|
2026-08-09 at `4002734`, after the framing was approved at revision 5
|
||||||
and the full gate suite went green. **Now at `842ec61`, MERGE-BLOCKED.**
|
and the full gate suite went green. **Now at `723afa7`, MERGE-BLOCKED.**
|
||||||
|
|
||||||
**Review round 1 found three blockers. Two are fixed; the third is why
|
**Review round 1 found three blockers. Two are fixed; the third is why
|
||||||
this lane is blocked.**
|
this lane is blocked.**
|
||||||
|
|
@ -301,14 +301,15 @@ this lane is blocked.**
|
||||||
already. Verified; no binding was added for this.
|
already. Verified; no binding was added for this.
|
||||||
- **P1a NOT fixed, and deliberately — it blocks this PR.** The async
|
- **P1a NOT fixed, and deliberately — it blocks this PR.** The async
|
||||||
completions display UI without capturing the initiating frontend
|
completions display UI without capturing the initiating frontend
|
||||||
(`builtin/runtime/git.lua:635` and `:928` at `842ec61`), so a result
|
(`builtin/runtime/git.lua:684` and `:965` at `723afa7`), so a result
|
||||||
surfaces in whichever frontend is active when git exits. `commit_to`
|
surfaces in whichever frontend is active when git exits. `commit_to`
|
||||||
is the right mechanism and is **not Lua-reachable** outside a
|
is the right mechanism and is **not Lua-reachable** outside a
|
||||||
directory open, so the fix lives in the **`destination-capture`**
|
directory open, so the fix lives in the **`destination-capture`**
|
||||||
lane. This lane adopts it after that lands. Verified untouched: no
|
lane — approved and in implementation. This lane adopts it after that
|
||||||
diff line in `ffe5ae2`, `6c1631e`, `3eca5e8` or `842ec61` reaches
|
lands. Verified untouched: `show_diff_buffer`'s body and
|
||||||
`open_status_panel`, `show_diff_buffer`, `pmacs.window.display` or
|
`open_status_panel`'s `listview.open` are byte-identical to `4002734`,
|
||||||
`commit_to`.
|
and no commit on this branch adds a `commit_to` call or a frontend
|
||||||
|
capture anywhere.
|
||||||
|
|
||||||
**The second citation written here in round 1 was wrong** — `:854`
|
**The second citation written here in round 1 was wrong** — `:854`
|
||||||
pointed at `local unstaged = …` inside `diff_plan`, not at a display
|
pointed at `local unstaged = …` inside `diff_plan`, not at a display
|
||||||
|
|
@ -320,10 +321,12 @@ this lane is blocked.**
|
||||||
**Re-gated at `6c1631e`:** all 11 steps green, acceptance now 27 tests.
|
**Re-gated at `6c1631e`:** all 11 steps green, acceptance now 27 tests.
|
||||||
Both fixes mutation-verified.
|
Both fixes mutation-verified.
|
||||||
|
|
||||||
**Review round 2 found two more P2s, both the SAME SHAPE as the round-1
|
**Review round 2 found three more P2s, all the SAME SHAPE as the round-1
|
||||||
P1: module-level mutable state read at CONTINUATION time instead of
|
P1: module-level mutable state read or written at CONTINUATION time
|
||||||
captured at INVOCATION time.** Both fixed here; the P1a block above is
|
instead of captured at INVOCATION time.** All three fixed here; the P1a
|
||||||
unchanged and still the reason this lane cannot merge.
|
block above is unchanged and still the reason this lane cannot merge.
|
||||||
|
The fourth recurrence is why the last fix generalizes the rule instead
|
||||||
|
of adding another counter, and why the census below exists.
|
||||||
|
|
||||||
- **P2 fixed (`3eca5e8`) — an unborn-repository diff could switch
|
- **P2 fixed (`3eca5e8`) — an unborn-repository diff could switch
|
||||||
repositories mid-plan.** `run_diff_plan`'s `next_step` read
|
repositories mid-plan.** `run_diff_plan`'s `next_step` read
|
||||||
|
|
@ -344,20 +347,66 @@ unchanged and still the reason this lane cannot merge.
|
||||||
**single-line status band**, where truncating is right, so folding the
|
**single-line status band**, where truncating is right, so folding the
|
||||||
two together would fix one caller and break three.
|
two together would fix one caller and break three.
|
||||||
|
|
||||||
**A THIRD instance of the shape is still open and was NOT fixed** (not
|
**A third instance of the shape was found in the same pass and reported
|
||||||
in this round's scope, and no review finding covers it): **the diff path
|
rather than fixed silently:** the diff path had no generation counter at
|
||||||
has no generation counter at all.** `run_diff_plan` has no in-flight
|
all. Review independently raised it as **P2 #3**, and it is fixed below.
|
||||||
guard, and `show_diff_buffer` writes the single `state.diff_buffer` at
|
|
||||||
completion — so two `d` presses in flight together are last-writer-wins,
|
|
||||||
and a slow first diff overwrites a fast second one. Reachable: `d`
|
|
||||||
displays into the document window, the user refocuses the panel, `d`
|
|
||||||
again. The status path solved exactly this with `reserve_generation`;
|
|
||||||
the diff path never got one.
|
|
||||||
|
|
||||||
**Re-gated at `842ec61`:** all steps green, acceptance now 29 tests.
|
- **P2 fixed (`723afa7`) — concurrent `d` requests were
|
||||||
Both round-2 fixes mutation-verified — `g6_22` fails on the second
|
last-writer-wins.** `git.diff-file` started a plan with **no request
|
||||||
spawned diff argv when the `state.root` read is restored, and `g6_14c`
|
generation** while every plan writes the **singleton** `*git-diff*`
|
||||||
resolves `<tmp>/nl` instead of `<tmp>/nl\nroot` when `first_line` is.
|
buffer through `show_diff_buffer`. `d` on A, then `d` on B before A
|
||||||
|
finishes: if A completed last, A's diff replaced B's. Reachable
|
||||||
|
without contrivance — `d` renders into the document window only at
|
||||||
|
completion, so the panel is still focused for a second press.
|
||||||
|
|
||||||
|
**Fixed by giving the rule ONE implementation instead of a fourth
|
||||||
|
hand-rolled counter.** `new_channel()` hands out a ticket at the
|
||||||
|
command and answers "is this still the request in force?" at the
|
||||||
|
completion; `state.generation` and `reserve_generation` are gone.
|
||||||
|
|
||||||
|
**Two channels, not one, and that is a design decision.** A single
|
||||||
|
module-wide counter would make `d` cancel an in-flight `g` and vice
|
||||||
|
versa. The status panel and the diff view are independent things a
|
||||||
|
user asks for, so each gets its own ordering; what is shared is the
|
||||||
|
**mechanism**, not the counter. A channel spans a whole **request**
|
||||||
|
rather than one process — a status open is `rev-parse` then `status` —
|
||||||
|
so `_deliver_root` and `_deliver_status` correctly share one ticket
|
||||||
|
while a diff plan gets its own. `g6_23` asserts the separation: two
|
||||||
|
`d` presses leave `_generation()` untouched.
|
||||||
|
|
||||||
|
The plan is restructured into the request shape the other two
|
||||||
|
continuations already use: `step_done`'s closure becomes
|
||||||
|
`pmacs.git._deliver_diff(request, step, res)`, exposed for the same
|
||||||
|
reason `_deliver_status` and `_deliver_root` are.
|
||||||
|
|
||||||
|
**The async-continuation census, so the next round is not another
|
||||||
|
instance hunt.** `git.lua` has **exactly three** async continuations,
|
||||||
|
plus one dispatcher and one synchronous impostor:
|
||||||
|
|
||||||
|
| continuation | invocation-time ticket? | needs one? | shared state written |
|
||||||
|
|---|---|---|---|
|
||||||
|
| `_deliver_root` (`rev-parse`) | yes, `status_requests`, reserved in `git.status()` | yes | `state.root`, `state.buffer`, spawns the status |
|
||||||
|
| `_deliver_status` (`git status`) | yes, `status_requests`, reserved at the command or the `g` keypress | yes | `state.branch`, `.rows`, `.display`, `.failure`, `.buffer`, the panel, the cursor |
|
||||||
|
| `_deliver_diff` (`git diff`, 1–2 steps) | **now yes**, `diff_requests`, reserved at the `d` keypress | yes | `state.diff_buffer`, the `*git-diff*` contents, the status band |
|
||||||
|
| `process.after-tick` pump | no | **no** — it is the dispatcher, not a request; it owns only the module-local `pump` table and calls each `on_done` once | `pump` |
|
||||||
|
| `run_git`'s spawn-failure path | n/a | **no** — it calls `on_done` **synchronously**, at invocation time, and that `on_done` carries the ticket anyway | none |
|
||||||
|
|
||||||
|
Everything else that looks like a callback is synchronous at the
|
||||||
|
keypress: `on_visit`/`on_refresh` on the listview spec, the `*git-diff*`
|
||||||
|
read-only intercept, and the two `pmacs.command.define` bodies.
|
||||||
|
|
||||||
|
`state.diff_buffer` is deliberately still read at continuation time and
|
||||||
|
that is correct: "do I already have a live diff buffer?" is a question
|
||||||
|
about *now*, not about the invocation. It is the state `_deliver_diff`
|
||||||
|
guards, not a second instance of the bug.
|
||||||
|
|
||||||
|
**Re-gated at `723afa7`:** all steps green, acceptance now 30 tests. All
|
||||||
|
three round-2 fixes mutation-verified — `g6_22` fails on the second
|
||||||
|
spawned diff argv when the `state.root` read is restored; `g6_14c`
|
||||||
|
resolves `<tmp>/nl` instead of `<tmp>/nl\nroot` when `first_line` is;
|
||||||
|
and `g6_23` fails when the ticket check is removed, at its **real**
|
||||||
|
half, the older plan having overwritten the newer one's patch before the
|
||||||
|
driven delivery was reached.
|
||||||
|
|
||||||
**Written with the lane's first commit, before the PR exists** — the
|
**Written with the lane's first commit, before the PR exists** — the
|
||||||
standing correction from #171 and #215. This session it was missed on
|
standing correction from #171 and #215. This session it was missed on
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue