From 723afa717f7abbd03bf08039970af1af16bd1741 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 16:48:54 +0200 Subject: [PATCH] fix(git): give diff requests a ticket, and share one channel mechanism `git.diff-file` started a plan with no request generation while every plan writes the SINGLETON `*git-diff*` buffer through `show_diff_buffer`. Press `d` on A, then `d` on B before A finishes: if A completes last, A's diff replaces B's. The newest invocation loses to the slowest subprocess --- the same defect the status channel had before `ffe5ae2`, on the one surface that had never been given the fix. Every finding on this lane has now been one shape: module-level mutable state read or written at CONTINUATION time without an invocation-time ticket. So the rule gets ONE implementation rather than 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, and that is a design decision rather than an oversight: 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 "newest wins" 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`, and a diff is one or two `git diff` runs --- so `_deliver_root` and `_deliver_status` correctly share one ticket while the diff plan gets its own. `g6_23` asserts the separation directly: 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 exactly the reason `_deliver_status` and `_deliver_root` are: no arrangement of real subprocess timing can make two `git diff` runs finish in a chosen order, and the contract is about the order the caller did NOT choose. The ticket check sits at the single point a plan re-enters from a continuation, so one check covers everything downstream --- no further spawn, no buffer write, and no status message, since a status line from a replaced invocation is as wrong as a buffer from one. Witnessed by `g6_23` in two halves. The real half presses `d` twice with nothing pumped between, so two plans are genuinely in flight and each really reserved its own ticket. The driven half then completes the OLDER request after the newer one has rendered --- the ordering that is the whole contract, and the one real timing will not produce on demand, since the first plan spawned normally finishes first and that order passes on the broken code. A superseded FAILURE is asserted too, since a buffer-only check would miss the status-message half. The positive control at the current ticket makes the discards attributable to the ticket rather than the payload. Mutation-verified: removing the ticket check fails `g6_23` --- and in that run it failed at the REAL half, the older plan having overwritten the newer one's patch before the fabricated delivery was ever reached. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- builtin/runtime/git.lua | 208 +++++++++++++++++--------- tests/git_status_stage1_acceptance.rs | 126 ++++++++++++++++ 2 files changed, 261 insertions(+), 73 deletions(-) diff --git a/builtin/runtime/git.lua b/builtin/runtime/git.lua index bfac410..9ef69c1 100644 --- a/builtin/runtime/git.lua +++ b/builtin/runtime/git.lua @@ -458,6 +458,51 @@ function pmacs.git.parse_status(text) return { branch = branch, rows = rows } end +-- --------------------------------------------------------------------- +-- Request channels: "the newest INVOCATION wins" +-- --------------------------------------------------------------------- +-- +-- EVERY defect review found on this module was one shape: module-level +-- mutable state read or written at CONTINUATION time without an +-- invocation-time ticket. So the rule gets one implementation instead of +-- a bespoke counter per call site. +-- +-- A channel hands out a ticket when the user ASKS for something and +-- answers "is this still the request in force?" when a subprocess +-- finally replies. A continuation holding a stale ticket must discard +-- BEFORE ANY EFFECT --- no spawn, no shared-state write, and no status +-- message either, since a message from a replaced invocation is as wrong +-- as a panel from one. +-- +-- There are TWO channels, and that is deliberate rather than an +-- oversight: a single module-wide counter would make pressing `d` cancel +-- an in-flight `g`, and vice versa. The status panel and the diff view +-- are independent things a user can ask for, so each gets its own "newest +-- wins" ordering. What is shared is the MECHANISM, not the counter. +-- +-- A channel spans a whole request, not one process: a status open is +-- `rev-parse` then `status`, and a diff is one or two `git diff` runs. +-- Every stage of one request carries the ticket reserved at the command. +local function new_channel() + local ch = { current = 0 } + --- Claim the newest ticket, and return it. Called at the point a user + --- ASKS for something, never at the point some subprocess answers --- + --- minting on arrival makes the SLOWEST subprocess win instead of the + --- newest invocation, which is exactly the bug this exists to stop. + function ch.reserve() + ch.current = ch.current + 1 + return ch.current + end + --- True while `ticket` is still the request in force. + function ch.is_current(ticket) + return ticket == ch.current + end + return ch +end + +local status_requests = new_channel() +local diff_requests = new_channel() + -- --------------------------------------------------------------------- -- Panel state -- --------------------------------------------------------------------- @@ -474,7 +519,6 @@ local state = { buffer = nil, diff_buffer = nil, failure = nil, - generation = 0, } local function status_line_text(row) @@ -590,14 +634,20 @@ local function open_status_panel(rows) state.buffer = pmacs.window.buffer() end ---- The refresh generation currently in force. +--- The status-refresh ticket currently in force. --- --- Exposed alongside `_deliver_status` below, and for the same reason: --- the discard rule is about a completion arriving LATE, and a caller --- cannot construct a stale request without knowing what "current" --- means. function pmacs.git._generation() - return state.generation + return status_requests.current +end + +--- The diff ticket currently in force. `_generation`'s counterpart, on +--- the other channel; see `new_channel` for why they are two. +function pmacs.git._diff_generation() + return diff_requests.current end --- Deliver a completed `git status`. @@ -607,11 +657,10 @@ end --- real subprocess timing can guarantee. function pmacs.git._deliver_status(request, res) -- Generation (Q#G-1 item 3): a second `g` while one is in flight - -- bumps the generation, and the older completion DISCARDS its rows - -- rather than racing. It does not terminate the first process --- - -- reaping is `process.forget`'s job and killing git mid-read buys - -- nothing. - if request.generation ~= state.generation then return end + -- bumps the ticket, and the older completion DISCARDS its rows rather + -- than racing. It does not terminate the first process --- reaping is + -- `process.forget`'s job and killing git mid-read buys nothing. + if not status_requests.is_current(request.generation) then return end -- Panel lifetime (Q#G-1 item 5): if the buffer this refresh belongs -- to is gone, drop the result. A FIRST open carries no expectation. if request.expect_buffer ~= nil then @@ -672,18 +721,6 @@ local function selected_path() return row and row.path or nil end ---- Claim the newest generation, and return it. ---- ---- Called at the point a user ASKS for something, never at the point ---- some subprocess happens to answer. That distinction is the whole ---- rule: the generation counter exists to make the newest INVOCATION ---- win, and minting it from a completion callback instead makes the ---- slowest subprocess win. -local function reserve_generation() - state.generation = state.generation + 1 - return state.generation -end - --- Spawn `git status` under an ALREADY-RESERVED generation. --- --- The generation is a parameter rather than something minted here, @@ -721,7 +758,7 @@ function pmacs.git._on_refresh() -- Reserved HERE, at the keypress, for the same reason `git.status` -- reserves at the command: `g` needs no root lookup, so this is -- already the moment of invocation. - start_status(state.root, state.buffer, true, reserve_generation()) + start_status(state.root, state.buffer, true, status_requests.reserve()) return listview_rows("(refreshing...)") end @@ -767,7 +804,7 @@ function pmacs.git._deliver_root(request, res) -- it must not proceed: not to a status spawn, not to `state.root`, and -- not even to a status-line message. Everything below this line is an -- effect belonging to an invocation the user has already replaced. - if request.generation ~= state.generation then return end + if not status_requests.is_current(request.generation) then return end if not (res.ok and res.code == 0) then if res.spawn_error then @@ -810,7 +847,7 @@ function pmacs.git.status() -- invocation that starts no work must not invalidate one that is -- already in flight, and an invocation that does start work must own -- the newest generation from that moment on. - local request = { generation = reserve_generation(), dir = dir } + local request = { generation = status_requests.reserve(), dir = dir } -- The root rule (Q#G-2): ask git, and let a non-zero exit BE the -- "not a repository" answer. `-C ` with no root of our own. run_git("git rev-parse", nil, { "-C", dir, "rev-parse", "--show-toplevel" }, @@ -928,54 +965,77 @@ local function show_diff_buffer(title, body) pcall(pmacs.window.display, buf, { select = true }) end --- Run the plan's steps in order, then render. +-- Start the request's next step, or render what the finished ones +-- produced. -- --- `root` is a PARAMETER, captured at the keypress, and `state.root` is --- deliberately not read anywhere below. An unborn `AM`/`AD` row produces --- a TWO-STEP plan, and `state.root` is module-level mutable state that a --- concurrent `git.status` against another repository reassigns from its --- own root-resolution callback. Reading it per step would let one plan's --- second step run in a different repository than its first --- with the --- first repository's path --- so every step of one plan runs against the --- repository the user was looking at when they pressed `d`. Same shape --- as the generation counter: capture at the INVOCATION, never at the --- continuation. -local function run_diff_plan(row, plan, root) - local pieces = {} - local index = 0 - local step_done - local function next_step() - index = index + 1 - local step = plan.steps[index] - if not step then - local body = table.concat(pieces, "\n") - if body:gsub("%s", "") == "" then - body = "(no differences)" - end - show_diff_buffer(string.format("git diff --- %s\n%s", - pmacs.git.display_path(row.path), plan.header), body) - return +-- Everything this needs lives on `request`, captured at the keypress: +-- the diff ticket, the row, the plan, and the ROOT. `state.root` is +-- deliberately not read anywhere in here. An unborn `AM`/`AD` row +-- produces a TWO-STEP plan, and `state.root` is module-level mutable +-- state that a concurrent `git.status` against another repository +-- reassigns from its own root-resolution callback --- so reading it per +-- step would let one plan's second step run in a different repository +-- than its first, carrying the first repository's path. +local function advance_diff(request) + request.index = request.index + 1 + local step = request.plan.steps[request.index] + if not step then + local body = table.concat(request.pieces, "\n") + if body:gsub("%s", "") == "" then + body = "(no differences)" end - run_git("git diff", root, step.args, function(res) step_done(step, res) end) + show_diff_buffer(string.format("git diff --- %s\n%s", + pmacs.git.display_path(request.row.path), request.plan.header), body) + return end - step_done = function(step, res) - if not diff_step_ok(step, res) then - local reason = failure_reason(res) - show_diff_buffer(string.format("git diff --- %s", - pmacs.git.display_path(row.path)), reason) - pmacs.editor.set_status("git diff: " .. reason) - return - end - local text = utf8_clean(res.stdout) - if step.label then - pieces[#pieces + 1] = string.format("=== %s ===\n%s", step.label, - text ~= "" and text or "(no changes)\n") - else - pieces[#pieces + 1] = text - end - next_step() + run_git("git diff", request.root, step.args, + function(res) pmacs.git._deliver_diff(request, step, res) end) +end + +--- Deliver a completed diff STEP for the plan in `request`. +--- +--- Exposed for the same reason `_deliver_status` and `_deliver_root` +--- are: the contract is about completions arriving in an order the +--- CALLER did not choose, and no arrangement of real subprocess timing +--- can guarantee that two `git diff` runs finish in a chosen order. +function pmacs.git._deliver_diff(request, step, res) + -- Superseded by a newer `d`: discard BEFORE ANY EFFECT. `*git-diff*` + -- is a singleton buffer, so without this a slow first request + -- overwrites a fast second one --- the newest invocation loses to the + -- slowest subprocess, which is the same defect the status channel had. + -- + -- This is the ONE place a diff plan re-enters from a continuation, so + -- one check covers all of it: no further spawn (`advance_diff` is + -- never reached), no buffer write, and no status message either, since + -- a status line from a replaced invocation is as wrong as a buffer + -- from one. The in-flight process is not terminated --- reaping is + -- `process.forget`'s job, exactly as on the status channel. + if not diff_requests.is_current(request.generation) then return end + if not diff_step_ok(step, res) then + local reason = failure_reason(res) + show_diff_buffer(string.format("git diff --- %s", + pmacs.git.display_path(request.row.path)), reason) + pmacs.editor.set_status("git diff: " .. reason) + return end - next_step() + local text = utf8_clean(res.stdout) + if step.label then + request.pieces[#request.pieces + 1] = string.format("=== %s ===\n%s", step.label, + text ~= "" and text or "(no changes)\n") + else + request.pieces[#request.pieces + 1] = text + end + advance_diff(request) +end + +-- Run `plan`'s steps in order under an ALREADY-RESERVED diff ticket, +-- then render. `generation` is a parameter for the same reason +-- `start_status`'s is: it belongs to the keypress, not to this call. +local function run_diff_plan(row, plan, root, generation) + advance_diff { + generation = generation, row = row, plan = plan, root = root, + pieces = {}, index = 0, + } end pmacs.command.define { @@ -1005,11 +1065,13 @@ pmacs.command.define { pmacs.editor.set_status("git: disabled by the `git.enabled` setting") return end - -- The root is captured HERE, at the keypress, and threaded through - -- every step of the plan. `state.branch` is read here for the same - -- reason: both describe the repository the user is looking at right - -- now, and both are replaced wholesale by a `git.status` against - -- another repository. - run_diff_plan(row, diff_plan(row, (state.branch or {}).unborn == true), state.root) + -- Everything the plan runs on is captured HERE, at the keypress, and + -- threaded through every step: the ticket, reserved AFTER the early + -- returns above so a `d` that starts no work cannot supersede one + -- that is already in flight; the root; and the unborn flag. All three + -- describe the repository the user is looking at right now, and all + -- three are replaced wholesale by a `git.status` against another one. + run_diff_plan(row, diff_plan(row, (state.branch or {}).unborn == true), + state.root, diff_requests.reserve()) end, } diff --git a/tests/git_status_stage1_acceptance.rs b/tests/git_status_stage1_acceptance.rs index 1dbb5a6..9099cac 100644 --- a/tests/git_status_stage1_acceptance.rs +++ b/tests/git_status_stage1_acceptance.rs @@ -1394,6 +1394,132 @@ fn g6_21_a_superseded_root_lookup_does_not_spawn_its_status() { ); } +/// Two `d` requests in flight: the **newer** one wins, even when the +/// older one completes **last**. +/// +/// `*git-diff*` is a singleton buffer, so a diff plan that finishes +/// after a newer one would otherwise overwrite it — the newest +/// invocation losing to the slowest subprocess, which is exactly the +/// defect the status channel was fixed for. The diff channel now +/// reserves its own ticket at the keypress and discards a superseded +/// completion **before any effect**. +/// +/// Two halves, and both are needed: +/// +/// * the **real** half presses `d` twice with nothing pumped between, +/// so two plans are genuinely in flight and each really did reserve +/// its own ticket at the command; +/// * the **driven** half then completes the OLDER request, after the +/// newer one has already rendered. That ordering is the whole +/// contract and no arrangement of real subprocess timing can produce +/// it on demand — both diffs take milliseconds, and the first one +/// spawned normally finishes first, which is the order that passes on +/// the broken code. Same reason `g6_17` and `g6_21` drive their own +/// completions. +/// +/// The positive control at the end is what makes the discard +/// attributable to the ticket rather than to the payload. +#[test] +fn g6_23_a_superseded_diff_does_not_replace_the_newer_one() { + let (_dir, root) = tempdir(); + mixed_repo(&root); + let mut s = editor(); + open_panel(&mut s, &root, "staged.txt"); + + // Request A, then request B, with no frame pumped between them. `d` + // renders into the DOCUMENT window only at completion, so the panel + // is still focused for the second press. + let status_gen_before: i64 = eval(&s, "return pmacs.git._generation()"); + seat_on(&mut s, "staged.txt"); + press(&mut s, KeyCode::Char('d')); + let gen_a: i64 = eval(&s, "return pmacs.git._diff_generation()"); + seat_on(&mut s, "unstaged.txt"); + press(&mut s, KeyCode::Char('d')); + let gen_b: i64 = eval(&s, "return pmacs.git._diff_generation()"); + assert_eq!( + gen_b, + gen_a + 1, + "premise: each `d` reserves its own ticket at the keypress" + ); + // …and the diff channel is its OWN: two `d` presses must not have + // touched the status channel, which a single module-wide counter + // would have done — making `d` cancel an in-flight `g`. + let status_gen_after: i64 = eval(&s, "return pmacs.git._generation()"); + assert_eq!( + status_gen_before, status_gen_after, + "a diff must not consume the status channel's ticket" + ); + + assert!( + pump_until(&mut s, 15_000, |s| diff_text(s).contains("+worktree edit")), + "the newest request must render; diff was:\n{}\nstatus: {:?}", + diff_text(&s), + status(&s) + ); + let settled = diff_text(&s); + assert!( + settled.contains("git diff --- unstaged.txt"), + "…and it is the row `d` was last pressed on: {settled}" + ); + + // The older request finally answers. It must change nothing. + let stale = format!( + "pmacs.git._deliver_diff(\n\ + {{ generation = {gen_a}, row = {{ path = 'staged.txt' }},\n\ + plan = {{ header = 'against HEAD', steps = {{}} }},\n\ + root = '/', pieces = {{}}, index = 0 }},\n\ + {{}},\n\ + {{ ok = true, code = 0, stdout = 'STALE-DIFF-SENTINEL\\n', stderr = '' }})" + ); + exec(&s, &stale); + assert_eq!( + diff_text(&s), + settled, + "a superseded diff must not replace the newer one" + ); + + // …and it must not reach the status band either, which is the half a + // buffer-only check would miss. + exec(&s, "pmacs.editor.set_status('')"); + exec( + &s, + &format!( + "pmacs.git._deliver_diff(\n\ + {{ generation = {gen_a}, row = {{ path = 'staged.txt' }},\n\ + plan = {{ header = 'against HEAD', steps = {{}} }},\n\ + root = '/', pieces = {{}}, index = 0 }},\n\ + {{}},\n\ + {{ ok = true, code = 128, stdout = '',\n\ + stderr = 'fatal: STALE-FAILURE' }})" + ), + ); + assert_eq!( + status(&s), + "", + "a superseded FAILURE is as wrong as a superseded patch" + ); + assert_eq!( + diff_text(&s), + settled, + "and it wrote no failure body either" + ); + + // The positive control: the same delivery at the CURRENT ticket does + // land, so the two discards above were about the ticket. + exec( + &s, + &stale.replace( + &format!("generation = {gen_a}"), + &format!("generation = {gen_b}"), + ), + ); + let now = diff_text(&s); + assert!( + now.contains("STALE-DIFF-SENTINEL"), + "the current ticket must be delivered: {now}" + ); +} + /// Selection is re-seated **by the completion handler**, across a /// refresh that reorders rows. ///