diff --git a/builtin/runtime/git.lua b/builtin/runtime/git.lua index c19b9a7..e4f44af 100644 --- a/builtin/runtime/git.lua +++ b/builtin/runtime/git.lua @@ -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 ` 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 { diff --git a/tests/git_status_stage1_acceptance.rs b/tests/git_status_stage1_acceptance.rs index 26d861b..a793495 100644 --- a/tests/git_status_stage1_acceptance.rs +++ b/tests/git_status_stage1_acceptance.rs @@ -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 = 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 = 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 = 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. ///