fix(git): capture the diff plan's root at `d`, not per step

`run_diff_plan`'s `next_step` read the module-level `state.root` each
time it started a step, so a multi-step plan could change repositories
halfway through.

An unborn `AM`/`AD` row produces a TWO-STEP plan --- staged patch, then
unstaged patch --- and the second step is spawned from the first one's
completion callback. `state.root` is reassigned by `_deliver_root`,
which runs whenever a concurrent `git.status` for another repository
finishes resolving its worktree. Start a diff in A, run `git.status` in
B before the first patch lands, and the plan's second step runs with B
as its cwd and A's path: git there matches nothing, so the unstaged half
silently renders "(no changes)" instead of the worktree delta it exists
to show.

The root is now captured at the keypress and threaded through the plan
as a parameter; `state.root` is not read inside the plan at all. Same
shape as the generation counter fixed in `ffe5ae2` --- capture at the
INVOCATION, never at the continuation --- and the third instance of it,
`state.branch`, is already read at the keypress on the same line.

Witnessed by `g6_22`: an unborn `AM` row's two-step plan with
`state.root` reassigned between the keypress and the first step's
completion, asserting BOTH spawned diff argvs carry the originally
captured root and neither carries the other repository's. Driven through
`_deliver_root` because no arrangement of real subprocess timing can
guarantee the interleaving, and nothing is pumped between the keypress
and the reassignment, so step 1 is genuinely in flight.

The argv assertion is the load-bearing half. A test that checked only
the first step, or only that a diff rendered, passes on the broken code:
step 1 is spawned synchronously from the keypress, and step 2 against
the wrong repository exits 0 with empty output rather than failing.
Mutation-verified --- restoring the `state.root` read fails the test on
the second argv.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai
This commit is contained in:
Levi Neuwirth 2026-08-09 16:16:52 +02:00
parent a70ee5fdc0
commit 3eca5e8f60
No known key found for this signature in database
2 changed files with 116 additions and 3 deletions

View File

@ -900,7 +900,18 @@ local function show_diff_buffer(title, body)
end
-- Run the plan's steps in order, then render.
local function run_diff_plan(row, plan)
--
-- `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
@ -916,7 +927,7 @@ local function run_diff_plan(row, plan)
pmacs.git.display_path(row.path), plan.header), body)
return
end
run_git("git diff", state.root, step.args, function(res) step_done(step, res) end)
run_git("git diff", root, step.args, function(res) step_done(step, res) end)
end
step_done = function(step, res)
if not diff_step_ok(step, res) then
@ -965,6 +976,11 @@ pmacs.command.define {
pmacs.editor.set_status("git: disabled by the `git.enabled` setting")
return
end
run_diff_plan(row, diff_plan(row, (state.branch or {}).unborn == true))
-- 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)
end,
}

View File

@ -786,6 +786,103 @@ fn g6_6c_a_single_state_unborn_row_takes_one_patch_and_says_so() {
);
}
/// A two-step plan runs **every** step against the repository the user
/// was looking at when they pressed `d`.
///
/// An unborn `AM` row is the only shape that makes this observable: it
/// produces a **two-step** plan (staged patch, then unstaged patch), and
/// the second step used to be spawned from the first one's completion
/// callback against whatever `state.root` held **by then**. So a
/// `git.status` for another repository, whose root lookup lands while
/// the first patch is still in flight, moved the plan's second step into
/// a different repository — carrying the first repository's path, which
/// git there resolves to nothing at all.
///
/// Driven through `_deliver_root` for the same reason `g6_21` is: no
/// arrangement of real subprocess timing can guarantee that the
/// interleaving happens, and a test that merely hoped for it would pass
/// on the broken code most of the time. Nothing is pumped between the
/// keypress and the reassignment, so step 1 is genuinely in flight.
///
/// The argv assertion is the load-bearing one. A test that checked only
/// the FIRST step — or only that a diff rendered — passes on the broken
/// code, since step 1 is spawned synchronously from the keypress and
/// step 2 against the wrong repository merely produces an empty patch.
#[test]
fn g6_22_a_two_step_plan_keeps_the_root_it_started_with() {
let (_dir_a, root_a) = tempdir();
unborn_repo(&root_a);
// An unrelated repository, with no `am.txt` in it: a step that
// escaped into B would find nothing and render "(no changes)".
let (_dir_b, root_b) = tempdir();
mixed_repo(&root_b);
let mut s = editor();
open_panel(&mut s, &root_a, "am.txt");
seat_on(&mut s, "am.txt");
let a = root_a.display().to_string();
let b = root_b.display().to_string();
// `d` spawns step 1 synchronously against A…
press(&mut s, KeyCode::Char('d'));
// …and now, before a single frame is pumped, a `git.status` for B
// resolves its root and reassigns `state.root`.
exec(
&s,
&format!(
"pmacs.git._deliver_root(\n\
{{ generation = pmacs.git._generation(), dir = {b:?} }},\n\
{{ ok = true, code = 0, stdout = {b:?}, stderr = '' }})"
),
);
assert!(
pump_until(&mut s, 15_000, |s| diff_text(s)
.contains("=== unstaged (worktree) ===")),
"the plan must run to completion; diff was:\n{}\nstatus: {:?}",
diff_text(&s),
status(&s)
);
let diffs: Vec<String> = eval(
&s,
"local out = {}\n\
for _, args in ipairs(pmacs.git._spawn_log) do\n\
for _, a in ipairs(args) do\n\
if a == 'diff' then\n\
out[#out + 1] = table.concat(args, ' ')\n\
break\n\
end\n\
end\n\
end\n\
return out",
);
assert_eq!(
diffs.len(),
2,
"premise: the AM row's plan really is two steps: {diffs:?}"
);
for argv in &diffs {
assert!(
argv.contains(&format!("-C {a}")),
"every step of one plan runs against the captured root: {argv:?}"
);
assert!(
!argv.contains(&b),
"and none of them may follow `state.root` into another \
repository: {argv:?}"
);
}
// The user-visible half: the second patch is still A's worktree
// delta, not the empty answer B would have given.
let diff = diff_text(&s);
assert!(
diff.contains("+worktree edit"),
"the unstaged half must still carry A's worktree delta: {diff}"
);
}
/// Rename/copy under an unborn `HEAD` is **unreachable**.
///
/// The fixture `git mv`s a staged-but-uncommitted file and the parser