fix(git): reserve the refresh generation at the command, not on arrival
PR #227 review, P1. Two `git.status` invocations against different repositories were ordered by which `rev-parse` returned first, not by which the user asked for last. `git.status` started an UNVERSIONED `rev-parse`, and the generation was minted later, inside `start_status`, which runs from that lookup's completion callback. So invoke status in repo A, then repo B: if B's root resolved first and A's resolved second, A claimed the NEWER generation and replaced B. The counter that exists to make the newest INVOCATION win instead made the slowest SUBPROCESS win --- and it did it silently, since both requests looked well-formed. The fix is the ordering, not a new check: * `reserve_generation()` is called at the point the user ASKS --- `git.status` after its early returns and before the spawn, `g` at the keypress. An invocation that starts no work reserves nothing, so it cannot invalidate one already in flight. * `start_status` takes the reserved generation as a PARAMETER instead of minting its own, so the value survives the round trip through the root lookup. * The root-lookup completion is now `pmacs.git._deliver_root`, and it drops a superseded result before any effect: no status spawn, no `state.root` write, and no status-line message. A message from an invocation the user has already replaced is as wrong as a panel from one, and the previous shape would have written both. Exposed for the same reason `_deliver_status` is exposed: the contract is about completions arriving in an order the caller did not choose, and no arrangement of real subprocess timing can guarantee two `rev-parse` runs finish in a chosen order. Witness: `g6_21_a_superseded_root_lookup_does_not_spawn_its_status` drives two real invocations, then completes their ROOT LOOKUPS out of order --- newer first, older second --- and asserts the superseded one spawns nothing at all, comparing the status-spawn count before and after. `g6_17` cannot see this: it drives the STATUS completions out of order, by which point the generation each carries is already fixed. Bite: restoring the old ordering (mint on arrival, no staleness check) fails `g6_21` and nothing else. A test that merely hoped for the bad subprocess order would have passed on the broken code about half the time, which is why this one drives the completion directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai
This commit is contained in:
parent
0aee97b725
commit
ffe5ae2d8d
|
|
@ -646,10 +646,31 @@ local function selected_path()
|
|||
return row and row.path or nil
|
||||
end
|
||||
|
||||
local function start_status(root, expect_buffer, want_selection)
|
||||
--- 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,
|
||||
--- because this runs from the root-resolution callback: two `git.status`
|
||||
--- invocations against different repositories resolve their roots
|
||||
--- concurrently, and if the generation were minted on arrival then the
|
||||
--- invocation whose `rev-parse` returned LAST would claim the newest
|
||||
--- generation and replace the newer request. Reserving at the command
|
||||
--- and carrying it through is what makes the ordering the user's, not
|
||||
--- the filesystem's.
|
||||
local function start_status(root, expect_buffer, want_selection, generation)
|
||||
local request = {
|
||||
generation = state.generation,
|
||||
generation = generation,
|
||||
expect_buffer = expect_buffer,
|
||||
selected_path = want_selection and selected_path() or nil,
|
||||
}
|
||||
|
|
@ -671,7 +692,10 @@ function pmacs.git._on_refresh()
|
|||
if not state.root then
|
||||
return listview_rows("(no repository --- run M-x git.status)")
|
||||
end
|
||||
start_status(state.root, state.buffer, true)
|
||||
-- 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())
|
||||
return listview_rows("(refreshing...)")
|
||||
end
|
||||
|
||||
|
|
@ -706,6 +730,41 @@ local function active_directory()
|
|||
return nil
|
||||
end
|
||||
|
||||
--- Deliver a completed root lookup for the invocation in `request`.
|
||||
---
|
||||
--- Exposed for the same reason `_deliver_status` is: 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
|
||||
--- `rev-parse` runs finish in a chosen order.
|
||||
function pmacs.git._deliver_root(request, res)
|
||||
-- A root lookup that returns after a newer invocation has superseded
|
||||
-- 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 (res.ok and res.code == 0) then
|
||||
if res.spawn_error then
|
||||
pmacs.editor.set_status("git: " .. failure_reason(res))
|
||||
else
|
||||
pmacs.editor.set_status(
|
||||
string.format("git: %s is not inside a repository", request.dir))
|
||||
end
|
||||
return
|
||||
end
|
||||
local root = first_line(res.stdout)
|
||||
if root == "" then
|
||||
pmacs.editor.set_status("git: rev-parse returned no worktree root")
|
||||
return
|
||||
end
|
||||
state.root = root
|
||||
-- A fresh open carries no buffer expectation, so a panel the user
|
||||
-- killed earlier does not make this run drop its own first result.
|
||||
state.buffer = nil
|
||||
-- The generation reserved at the command, NOT a fresh one.
|
||||
start_status(root, nil, false, request.generation)
|
||||
end
|
||||
|
||||
--- Open (or re-open) `*git-status*` for the repository containing the
|
||||
--- active file.
|
||||
function pmacs.git.status()
|
||||
|
|
@ -718,29 +777,15 @@ function pmacs.git.status()
|
|||
pmacs.editor.set_status("git: no directory to resolve a repository from")
|
||||
return
|
||||
end
|
||||
-- Reserved AFTER the early returns and BEFORE the spawn: an
|
||||
-- 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 }
|
||||
-- The root rule (Q#G-2): ask git, and let a non-zero exit BE the
|
||||
-- "not a repository" answer. `-C <dir>` with no root of our own.
|
||||
run_git("git rev-parse", nil, { "-C", dir, "rev-parse", "--show-toplevel" },
|
||||
function(res)
|
||||
if not (res.ok and res.code == 0) then
|
||||
if res.spawn_error then
|
||||
pmacs.editor.set_status("git: " .. failure_reason(res))
|
||||
else
|
||||
pmacs.editor.set_status(string.format("git: %s is not inside a repository", dir))
|
||||
end
|
||||
return
|
||||
end
|
||||
local root = first_line(res.stdout)
|
||||
if root == "" then
|
||||
pmacs.editor.set_status("git: rev-parse returned no worktree root")
|
||||
return
|
||||
end
|
||||
state.root = root
|
||||
-- A fresh open carries no buffer expectation, so a panel the user
|
||||
-- killed earlier does not make this run drop its own first result.
|
||||
state.buffer = nil
|
||||
start_status(root, nil, false)
|
||||
end)
|
||||
function(res) pmacs.git._deliver_root(request, res) end)
|
||||
end
|
||||
|
||||
pmacs.command.define {
|
||||
|
|
|
|||
|
|
@ -1108,6 +1108,136 @@ fn g6_17_a_stale_completion_discards_its_rows() {
|
|||
);
|
||||
}
|
||||
|
||||
/// Two `git.status` invocations against DIFFERENT repositories, where
|
||||
/// the **first** invocation's root lookup completes **second**. The
|
||||
/// second invocation must win.
|
||||
///
|
||||
/// This is the ordering `g6_17` cannot see. That test drives the STATUS
|
||||
/// completions out of order, and the generation each of those carries
|
||||
/// was already fixed; this one drives the **root lookups** out of order,
|
||||
/// which is where the generation used to be minted. `git.status` started
|
||||
/// an unversioned `rev-parse` and the generation was claimed later, from
|
||||
/// the callback — so whichever `rev-parse` returned last claimed the
|
||||
/// newest generation and replaced the newer request. The counter that is
|
||||
/// supposed to make the newest INVOCATION win instead made the slowest
|
||||
/// SUBPROCESS win.
|
||||
///
|
||||
/// Driven through `_deliver_root` for the same reason `g6_17` uses
|
||||
/// `_deliver_status`: no arrangement of real subprocess timing can
|
||||
/// guarantee that two `rev-parse` runs finish in a chosen order, and a
|
||||
/// test that merely hoped for the bad order would pass on the broken
|
||||
/// code roughly half the time.
|
||||
///
|
||||
/// The assertion is on the argv of the last spawn, because the contract
|
||||
/// is precisely that the superseded lookup "must not proceed to spawn a
|
||||
/// status" — and that is observable without pumping, so the real
|
||||
/// `rev-parse` children still in flight cannot muddy it.
|
||||
#[test]
|
||||
fn g6_21_a_superseded_root_lookup_does_not_spawn_its_status() {
|
||||
let (_dir_a, root_a) = tempdir();
|
||||
mixed_repo(&root_a);
|
||||
let (_dir_b, root_b) = tempdir();
|
||||
mixed_repo(&root_b);
|
||||
|
||||
let mut s = editor();
|
||||
// Invocation 1 (repo A), then invocation 2 (repo B). Neither is
|
||||
// pumped, so both root lookups are genuinely in flight and each has
|
||||
// reserved its generation at the command.
|
||||
for root in [&root_a, &root_b] {
|
||||
let root_str = root.display().to_string();
|
||||
let seed = root.join("staged.txt").display().to_string();
|
||||
exec(
|
||||
&s,
|
||||
&format!(
|
||||
"pmacs.project.set_search_boundary({root_str:?})\n\
|
||||
pmacs.buffer.find_or_open({seed:?})"
|
||||
),
|
||||
);
|
||||
exec(&s, "pmacs.git.status()");
|
||||
}
|
||||
|
||||
let gen_b: i64 = eval(&s, "return pmacs.git._generation()");
|
||||
let gen_a = gen_b - 1;
|
||||
assert!(
|
||||
gen_a >= 1,
|
||||
"premise: each invocation reserved its own generation at the \
|
||||
command, so the two differ"
|
||||
);
|
||||
|
||||
let a = root_a.display().to_string();
|
||||
let b = root_b.display().to_string();
|
||||
|
||||
// The NEWER invocation's root lands first…
|
||||
exec(
|
||||
&s,
|
||||
&format!(
|
||||
"pmacs.git._deliver_root(\n\
|
||||
{{ generation = {gen_b}, dir = {b:?} }},\n\
|
||||
{{ ok = true, code = 0, stdout = {b:?}, stderr = '' }})"
|
||||
),
|
||||
);
|
||||
let after_b: Vec<String> = eval(&s, "return pmacs.git._last_spawn.args");
|
||||
assert!(
|
||||
after_b.contains(&b),
|
||||
"premise: the newer invocation spawned its status against B: {after_b:?}"
|
||||
);
|
||||
let statuses_after_b: i64 = eval(
|
||||
&s,
|
||||
"local n = 0\n\
|
||||
for _, args in ipairs(pmacs.git._spawn_log) do\n\
|
||||
for _, a in ipairs(args) do if a == 'status' then n = n + 1 end end\n\
|
||||
end\n\
|
||||
return n",
|
||||
);
|
||||
|
||||
// …and the OLDER invocation's root lands second, superseded.
|
||||
exec(
|
||||
&s,
|
||||
&format!(
|
||||
"pmacs.git._deliver_root(\n\
|
||||
{{ generation = {gen_a}, dir = {a:?} }},\n\
|
||||
{{ ok = true, code = 0, stdout = {a:?}, stderr = '' }})"
|
||||
),
|
||||
);
|
||||
|
||||
let after_a: Vec<String> = eval(&s, "return pmacs.git._last_spawn.args");
|
||||
assert!(
|
||||
!after_a.contains(&a),
|
||||
"the superseded root lookup must NOT spawn a status against A; \
|
||||
last argv was {after_a:?}"
|
||||
);
|
||||
assert_eq!(
|
||||
after_a, after_b,
|
||||
"…so the last spawn is still the newer invocation's"
|
||||
);
|
||||
let statuses_after_a: i64 = eval(
|
||||
&s,
|
||||
"local n = 0\n\
|
||||
for _, args in ipairs(pmacs.git._spawn_log) do\n\
|
||||
for _, a in ipairs(args) do if a == 'status' then n = n + 1 end end\n\
|
||||
end\n\
|
||||
return n",
|
||||
);
|
||||
assert_eq!(
|
||||
statuses_after_a, statuses_after_b,
|
||||
"and it spawned nothing at all — one status invocation, not two"
|
||||
);
|
||||
|
||||
// The user-visible half: pumping settles on B's repository, whatever
|
||||
// order the two real `rev-parse` children happen to finish in.
|
||||
assert!(
|
||||
pump_until(&mut s, 15_000, |s| !panel_text(s).is_empty()
|
||||
&& !panel_text(s).contains("refreshing")),
|
||||
"the winning invocation's panel must render; status was {:?}",
|
||||
status(&s)
|
||||
);
|
||||
let last: Vec<String> = eval(&s, "return pmacs.git._last_spawn.args");
|
||||
assert!(
|
||||
last.contains(&b) && !last.contains(&a),
|
||||
"the settled panel belongs to the second invocation: {last:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// Selection is re-seated **by the completion handler**, across a
|
||||
/// refresh that reorders rows.
|
||||
///
|
||||
|
|
|
|||
Loading…
Reference in New Issue