From afe79bd7dcecb2c55238424aa6700369da68e40a Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 16:35:38 +0200 Subject: [PATCH] docs: record PR #227's second review round on the git Stage 1 lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more P2s, both the same shape as round 1's P1 --- module-level mutable state read at continuation time instead of captured at invocation time --- plus the third instance of that shape, which is still open and which no review finding covers: the diff path has no generation counter at all, so two `d` presses in flight together are last-writer-wins on the single `*git-diff*` buffer. Also corrects a citation round 1 got wrong. The P1a block names two lines that must not be touched, and its second one (`:854`) pointed at `local unstaged = …` inside `diff_plan`, not at a display call. The site was always `show_diff_buffer`'s `pmacs.window.display`. A stale pointer in a block whose entire purpose is "leave these alone" is worse than none, so it is corrected rather than silently re-numbered. Section-local: nothing outside this lane's entry is touched or reflowed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 63 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 8 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index f3da0a3..2f58ea9 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -269,7 +269,7 @@ census. **PR #227** — https://github.com/levineuwirth/pmacs/pull/227. Opened 2026-08-09 at `4002734`, after the framing was approved at revision 5 -and the full gate suite went green. **Now at `6c1631e`, MERGE-BLOCKED.** +and the full gate suite went green. **Now at `842ec61`, MERGE-BLOCKED.** **Review round 1 found three blockers. Two are fixed; the third is why this lane is blocked.** @@ -301,17 +301,64 @@ this lane is blocked.** already. Verified; no binding was added for this. - **P1a NOT fixed, and deliberately — it blocks this PR.** The async completions display UI without capturing the initiating frontend - (`builtin/runtime/git.lua:609`, `:854`), so a result surfaces in - whichever frontend is active when git exits. `commit_to` is the right - mechanism and is **not Lua-reachable** outside a directory open, so - the fix lives in the **`destination-capture`** lane. This lane adopts - it after that lands. Verified untouched: no diff line in `ffe5ae2` or - `6c1631e` reaches `open_status_panel`, `show_diff_buffer`, - `pmacs.window.display` or `commit_to`. + (`builtin/runtime/git.lua:635` and `:928` at `842ec61`), so a result + surfaces in whichever frontend is active when git exits. `commit_to` + is the right mechanism and is **not Lua-reachable** outside a + directory open, so the fix lives in the **`destination-capture`** + lane. This lane adopts it after that lands. Verified untouched: no + diff line in `ffe5ae2`, `6c1631e`, `3eca5e8` or `842ec61` reaches + `open_status_panel`, `show_diff_buffer`, `pmacs.window.display` or + `commit_to`. + + **The second citation written here in round 1 was wrong** — `:854` + pointed at `local unstaged = …` inside `diff_plan`, not at a display + call. The site was always `show_diff_buffer`'s `pmacs.window.display`. + Corrected rather than silently re-numbered, because a stale pointer in + a block whose whole purpose is "do not touch these two lines" is worse + than none. **Re-gated at `6c1631e`:** all 11 steps green, acceptance now 27 tests. Both fixes mutation-verified. +**Review round 2 found two more P2s, both the SAME SHAPE as the round-1 +P1: module-level mutable state read at CONTINUATION time instead of +captured at INVOCATION time.** Both fixed here; the P1a block above is +unchanged and still the reason this lane cannot merge. + +- **P2 fixed (`3eca5e8`) — an unborn-repository diff could switch + repositories mid-plan.** `run_diff_plan`'s `next_step` read + `state.root` each time it started a step, and an unborn `AM`/`AD` row + produces a **two-step** plan. A concurrent `git.status` for another + repository reassigns `state.root` from its own root-resolution + callback, so a diff started in A ran its second step with B as cwd and + A's path — git there matches nothing, so the unstaged half silently + rendered `(no changes)` instead of the worktree delta it exists to + show. The root is now captured at the keypress and threaded through as + a parameter; `state.root` is not read inside the plan at all. +- **P2 fixed (`842ec61`) — a repository root containing a newline was + truncated.** `rev-parse --show-toplevel` was parsed with `first_line`, + so a root at `/tmp/a\nb` became `/tmp/a` and every command after it ran + with a nonexistent cwd. Fixed with a **separate** helper, + `strip_output_terminator`, at that one call site. `first_line` is + deliberately untouched: its other three callers all feed the + **single-line status band**, where truncating is right, so folding the + two together would fix one caller and break three. + +**A THIRD instance of the shape is still open and was NOT fixed** (not +in this round's scope, and no review finding covers it): **the diff path +has no generation counter at all.** `run_diff_plan` has no in-flight +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. +Both round-2 fixes mutation-verified — `g6_22` fails on the second +spawned diff argv when the `state.root` read is restored, and `g6_14c` +resolves `/nl` instead of `/nl\nroot` when `first_line` is. + **Written with the lane's first commit, before the PR exists** — the standing correction from #171 and #215. This session it was missed on #224 and again on #225, both caught by review; writing it now is the