From e94b256cc6edbf316caa92c6a22dd4767d08ce23 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 22:12:56 +0200 Subject: [PATCH] fix(git): say "copied" when a 2 record is a copy, not "renamed" Porcelain v2's `2` record covers renames AND copies --- the `` field leads with `R` or `C` --- and the diff header said "renamed from" for either. A copied file was therefore reported to the user as a rename, which is a different fact about their tree. The information was already retained: `parse_status` captures `score`. Nothing new is parsed; the header reads the byte it already has. `kind` stays `"rename"` for both, deliberately. Every BEHAVIOUR keyed on it is identical --- including the two-path `git diff HEAD -- `, which is right for a copy as much as for a rename. Splitting the kind would oblige every consumer present and future to spell `kind == "rename" or kind == "copy"`, and an arm forgotten anywhere silently drops copies back to the one-path diff: the exact regression this fix exists to avoid. The consumers are few and all were checked --- `diff_plan` is the tree's only `kind == "rename"` branch, `status_line_text` keys off `row.orig`, and the two tests that name the kind are `g6_1`'s corpus and `g6_8`'s unborn-unreachability assertion. `score` has no other reader anywhere. Read from `score` rather than from `row.x`: the score names rename-vs-copy whichever side detected the change, while `X` carries the letter only for an index-side one. The status ROW is UNCHANGED, and that is a decision rather than an omission. Its `XY` prefix already reads `R.` against `C.`, out of the same byte, in the porcelain vocabulary every other row in the panel is read in --- so the distinction is already on screen, and a second vocabulary beside it would be a wider surface for no new fact. `g6_4b` asserts both prefixes so the claim is checked. Unborn `HEAD` needs nothing, confirmed rather than assumed: `diff_plan`'s rename branch sits inside `if not unborn`, and `g6_8` already pins that no `2` record can occur there. `g6_4b` is a parser/presentation test and says so. Real `git` emits no `2 C` record --- the test MEASURES that under `-c status.renames=copies` rather than recalling it --- so the copy row is supplied through `_deliver_status`, the seam `g6_2b`/`g6_17`/`g6_21` already use. Everything downstream is real: repository, panel, `d` dispatch, spawned `git diff`, rendered buffer. Both crafted rows name paths that exist in the fixture, so each drives a real two-path diff. Both classes are asserted, and so is the argv --- a fix to what the user is TOLD must not reach what the module DOES. Mutations, each caught: header always "renamed" fails only the copy half; header always "copied" fails only the rename half; dropping `row.orig` from the steps fails the argv equality. Gate: `scripts/gate --acceptance git_status_stage1_acceptance --acceptance listview_acceptance --acceptance config_registry_acceptance` --- all eleven steps green, acceptance 34/34. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- builtin/runtime/git.lua | 34 ++++- docs/active-work.md | 55 ++++++++ docs/git-integration-framing.md | 14 ++ tests/git_status_stage1_acceptance.rs | 193 ++++++++++++++++++++++++++ 4 files changed, 294 insertions(+), 2 deletions(-) diff --git a/builtin/runtime/git.lua b/builtin/runtime/git.lua index 12e6e51..4b99f6f 100644 --- a/builtin/runtime/git.lua +++ b/builtin/runtime/git.lua @@ -409,6 +409,14 @@ end --- `path` is the CURRENT path --- what the panel shows and what RET --- visits --- and `orig` remembers where a rename or copy came from. --- Both are raw bytes; nothing here assumes they are text. +--- +--- **`kind = "rename"` covers copies too, deliberately.** A `2` record +--- is porcelain-v2's ONE two-path record and every behaviour keyed on +--- it is the same for both classes --- notably the two-path +--- `git diff HEAD -- `. What differs is only what the +--- user is TOLD, and that fact is already carried: `score` leads with +--- `R` or `C` (and `xy` carries the same letter on whichever side +--- detected it). See `is_copy` for where the two are told apart. function pmacs.git.parse_status(text) local branch = { unborn = false } local rows = {} @@ -543,6 +551,12 @@ local state = { failure = nil, } +-- A copy and a rename are already TOLD APART here, and by the field +-- that tells every other row class apart: the `XY` prefix reads `R.` +-- for one and `C.` for the other, out of the same byte `score` leads +-- with. So this renders both the same way on purpose --- `<-` reads +-- "came from", which is true of a copy --- rather than growing a second +-- vocabulary beside the porcelain codes the whole panel is built on. local function status_line_text(row) local shown = pmacs.git.display_path(row.path) if row.orig then @@ -906,6 +920,18 @@ pmacs.command.define { local SPLIT_HEADER = "no commits yet --- split view: staged (index) above, unstaged (worktree) below" +-- True when a `2` record is a COPY rather than a rename. +-- +-- Read from `score`, not from `row.x`. The `` field names +-- rename-vs-copy whichever side detected the change, while `X` carries +-- the letter only for an index-side one --- a worktree-side detection +-- puts it in `Y` and leaves `X` a `.`. Absent or malformed, this says +-- "not a copy", so the header falls back to the commoner of the two +-- rather than to a claim it cannot support. +local function is_copy(row) + return (row.score or ""):sub(1, 1) == "C" +end + -- The invocations `d` runs for `row`, as -- `{ { label = string|nil, args = {...}, no_index = bool }, ... }`, -- plus the header describing what the result shows. @@ -924,9 +950,13 @@ local function diff_plan(row, unborn) if not unborn then if row.kind == "rename" and row.orig then -- Both paths, which is what lets rename detection render this as - -- a rename rather than an unrelated add plus delete. + -- a rename rather than an unrelated add plus delete. Identical for + -- a copy, which is why the two share a `kind` --- only the WORD + -- differs, because a copy left the origin where it was and saying + -- "renamed" of it states a different fact about the user's tree. return { - header = string.format("against HEAD (renamed from %s)", + header = string.format("against HEAD (%s from %s)", + is_copy(row) and "copied" or "renamed", pmacs.git.display_path(row.orig)), steps = { { args = { "diff", "--no-color", "HEAD", "--", row.orig, path } } }, } diff --git a/docs/active-work.md b/docs/active-work.md index c8eff9c..bdb398e 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -534,6 +534,61 @@ Linux stayed green. are valid UTF-8 and legal on APFS, which is why they were green on macOS all along. +**Review round 4: a COPY was being reported as a RENAME.** + +- **Fixed at presentation, not in `kind`.** Porcelain v2's `2` record + covers renames **and** copies — `` leads with `R` or `C` — + and the parser already retained `score`. The diff header now reads + that byte and says `copied from` or `renamed from`; nothing new is + parsed. + + **`kind` stays `"rename"` for both, deliberately.** Every *behaviour* + keyed on it is identical, including the two-path + `git diff HEAD -- `, which is correct for a copy as + much as for a rename. Splitting the kind would force every consumer + present and future to spell `kind == "rename" or kind == "copy"`, and + an arm forgotten anywhere silently drops copies back to the one-path + diff — the exact regression the fix exists to avoid. Consumers + checked, and there are few: `diff_plan` (the only `kind == "rename"` + branch in the tree), `status_line_text` (keys off `row.orig`, not + `kind`), `g6_1`'s corpus assertion and `g6_8`'s unborn-unreachability + assertion. `score` has no other reader anywhere. + +- **Read from `score`, not from `row.x`.** The score field names + rename-vs-copy whichever side detected the change; `X` carries the + letter only for an index-side one, a worktree-side detection leaving + `X` a `.`. + +- **The status ROW is unchanged, and that is a decision.** Its `XY` + prefix already reads `R.` against `C.`, out of the same byte, in the + porcelain vocabulary every other row is read in — so the distinction + is already on screen and a second vocabulary beside it would be the + wider surface for no new fact. `g6_4b` asserts both prefixes, so the + claim is checked rather than asserted here. + +- **Parser-level coverage, stated plainly rather than implied.** Real + `git` emits no `2 C` record — the test MEASURES that, under + `-c status.renames=copies`, rather than recalling it — so the copy + ROW is supplied through `_deliver_status`, the seam `g6_2b`/`g6_17`/ + `g6_21` already use. Everything downstream is real: repository, + panel, `d` dispatch, spawned `git diff`, rendered buffer. Both + crafted rows name paths that exist in the fixture, so each drives a + real two-path diff. + +- **Unborn `HEAD` needed nothing, confirmed rather than assumed.** + `diff_plan`'s rename branch is inside `if not unborn`, and `g6_8` + already pins that no `2` record can occur there — over + `kind == 'rename'`, which under this choice covers copies too. + +**Re-gated:** all steps green, acceptance now **34 tests**. Three +mutations, each caught: header always `renamed` fails only the copy +half; header always `copied` fails only the rename half; dropping +`row.orig` from the steps fails the argv equality. The two `--lib` +failures seen on an earlier run (`composition_overhead_under_ten_percent`, +`setsid_escapee_…`, plus two perf tests) were **machine load from +sibling worktrees** — load average 15–27 — and pass in isolation and on +a re-run; nothing in this change touches `src/`. + **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 diff --git a/docs/git-integration-framing.md b/docs/git-integration-framing.md index 0878c64..b9c4d15 100644 --- a/docs/git-integration-framing.md +++ b/docs/git-integration-framing.md @@ -599,6 +599,20 @@ So, stated plainly rather than dressed up: both, deleted, renamed, and **untracked** — the last because a normal `git diff` shows nothing there, so a missing `--no-index` case makes `d` silently dead exactly where it is most used. +- **A copy is reported as a COPY, not a rename** (Q#G-7). Porcelain v2 + folds both into the one `2` record, so `kind` stays `"rename"` for + both — every *behaviour* keyed on it is the same — and the + distinction is made where it is a distinction: the diff header reads + the `` field's leading `R`/`C` and says which one happened. + The status row is left alone, because its `XY` prefix already reads + `R.` against `C.`. Both classes are asserted, and so is the **argv**: + the two-path `git diff HEAD -- ` is right for a copy + and a rename alike, so a fix to what the user is *told* must not + reach what runs. **Parser-level, deliberately** — see the corpus + bullet above: real `git` emits no `2 C` record even under + `status.renames=copies`, so the copy ROW is supplied through + `_deliver_status` while the repository, the panel, the `d` dispatch + and the spawned diff around it are real. - **The untracked diff renders on exit 1**, not a failure row (Q#G-7a) — the case `--exit-code` semantics would otherwise break, and the one most likely to be "fixed" later by someone who reads exit 1 as an diff --git a/tests/git_status_stage1_acceptance.rs b/tests/git_status_stage1_acceptance.rs index 8f35bdf..6a66706 100644 --- a/tests/git_status_stage1_acceptance.rs +++ b/tests/git_status_stage1_acceptance.rs @@ -200,6 +200,35 @@ fn mixed_repo(root: &Path) { write(root, "untracked.txt", "untracked body\n"); } +/// A repository holding **a real rename and a real copy**: `orig.txt` +/// is `git mv`d to `moved.txt`, and `copy_src.txt` is copied to a +/// staged `copy_dst.txt`. +/// +/// Both are real on disk, and that is what the copy fixture is FOR: +/// real `git` classifies the copy as an ordinary `1 A.` add (see +/// `g6_4b`), so the `2 C.` row has to be supplied — but because both of +/// its paths exist here, the two-path diff that row drives is a real +/// invocation rendering a real patch. +fn rename_and_copy_repo(root: &Path) { + init_repo(root); + write(root, "Cargo.toml", "[package]\nname = \"fixture\"\n"); + write( + root, + "orig.txt", + "a line of content long enough for rename detection to score it\n", + ); + write( + root, + "copy_src.txt", + "a line of content long enough for copy detection to score it\n", + ); + git(root, &["add", "-A"]); + git(root, &["commit", "-qm", "init"]); + git(root, &["mv", "orig.txt", "moved.txt"]); + std::fs::copy(root.join("copy_src.txt"), root.join("copy_dst.txt")).expect("cp"); + git(root, &["add", "copy_dst.txt"]); +} + /// The unborn fixture, enumerated from a real unborn repository rather /// than reasoned about: `git init`, stage three files, then edit one /// (`AM`), delete one (`AD`), and `git mv` one — which produces an @@ -911,6 +940,170 @@ fn refocus_panel(s: &mut EditorState) { ); } +/// A **copy** row's diff header says *copied*, a **rename** row's says +/// *renamed*, and both run the SAME two-path invocation. +/// +/// **This is a parser/presentation test, not end-to-end copy coverage, +/// and no test here could be.** Porcelain v2 folds renames and copies +/// into one `2` record whose `` field leads with `R` or `C`, but +/// real `git` will not emit a `2 C` record for a plain copy — not even +/// under `status.renames=copies`, which the premise below MEASURES +/// rather than recalls. The framing scopes "copied" to the parser level +/// for exactly that reason (§6's witness corpus), so the copy ROW is +/// supplied as payload bytes through `_deliver_status` — the seam +/// `g6_2b`, `g6_17` and `g6_21` already use for rows no fixture can +/// produce. +/// +/// Everything downstream of the row is real: the repository, the panel, +/// the `d` dispatch, the spawned `git diff`, and the rendered buffer. +/// Both crafted rows name paths that EXIST in the fixture, so each one's +/// two-path diff really runs and really renders. +/// +/// BOTH classes are asserted. A header that said "copied" for every `2` +/// record would satisfy the copy half on its own, so the rename half is +/// what makes this a distinction rather than a relabelling. The argv is +/// asserted for both as well: the two-path `git diff HEAD -- +/// ` is correct for a copy and a rename alike, so a fix to what +/// the user is TOLD must not reach what the module DOES. +#[test] +fn g6_4b_a_copy_says_copied_and_a_rename_says_renamed() { + let (_dir, root) = tempdir(); + rename_and_copy_repo(&root); + + let mut s = editor(); + open_panel(&mut s, &root, "copy_src.txt"); + + // The premise, measured: even asked for copy detection explicitly, + // real `git` reports the copy as `1 A.` and emits no `2 C` record. + // Pinned here so a future reader can see WHY the row below is + // crafted, instead of taking it on trust. + let raw = git( + &root, + &[ + "--no-optional-locks", + "-c", + "status.renames=copies", + "status", + "--porcelain=v2", + "--branch", + "-z", + ], + ); + assert!( + !raw.contains("2 C"), + "fixture premise: real git emits no `2 C` record for a plain copy, \ + even under status.renames=copies; it emitted:\n{raw:?}" + ); + assert!( + panel_text(&s).contains("A. copy_dst.txt"), + "…and the panel shows the real run's ordinary ADD: {}", + panel_text(&s) + ); + + // Now the crafted pair, delivered at the current generation. + let fields = [ + "# branch.oid 16fa4d708a09af0c96212f66395c3e204049534a".to_string(), + "# branch.head main".to_string(), + format!("2 R. N... 100644 100644 100644 {H} {H} R100 moved.txt"), + "orig.txt".to_string(), + format!("2 C. N... 100644 100644 100644 {H} {H} C100 copy_dst.txt"), + "copy_src.txt".to_string(), + ]; + let refs: Vec<&str> = fields.iter().map(String::as_str).collect(); + exec( + &s, + &format!( + "pmacs.git._deliver_status(\n\ + {{ generation = pmacs.git._generation() }},\n\ + {{ ok = true, code = 0, stdout = {}, stderr = '' }})", + z_payload(&refs) + ), + ); + + // The ROW rendering is deliberately the same for both, because the + // `XY` prefix ALREADY tells them apart — `R.` against `C.`, out of + // the same byte the score leads with, in the porcelain vocabulary + // every other row in the panel is read in. Pinned so the decision + // not to widen row rendering is a checked claim rather than a note. + let text = panel_text(&s); + assert!( + text.contains("R. moved.txt <- orig.txt"), + "the rename row keeps its `R.` prefix and both paths: {text}" + ); + assert!( + text.contains("C. copy_dst.txt <- copy_src.txt"), + "and the copy row is distinguished by its `C.` prefix: {text}" + ); + + assert_two_path_diff(&mut s, &root, "moved.txt", "orig.txt", "renamed", "copied"); + refocus_panel(&mut s); + assert_two_path_diff( + &mut s, + &root, + "copy_dst.txt", + "copy_src.txt", + "copied", + "renamed", + ); +} + +/// Press `d` on the `path` row and assert its header says +/// `" from "` and never `" from"` — over the +/// two-path `git diff HEAD -- `, asserted argv and all. +/// +/// One helper for both classes on purpose: a copy and a rename differ +/// in exactly one word, and everything else about them — the +/// invocation, the patch, the buffer — has to stay identical, which is +/// easiest to keep honest when the same code asserts it twice. +fn assert_two_path_diff( + s: &mut EditorState, + root: &Path, + path: &str, + orig: &str, + said: &str, + not_said: &str, +) { + seat_on(s, path); + let diff = press_d_and_wait(s, path); + assert!( + diff.contains(&format!("against HEAD ({said} from {orig})")), + "the {path:?} row's header must say {said:?} of {orig:?} — a copy \ + left its origin where it was, and saying \"renamed\" of it states \ + a different fact about the user's tree: {diff}" + ); + assert!( + !diff.contains(&format!("{not_said} from")), + "…and must never say {not_said:?}: {diff}" + ); + assert!( + !diff.contains("exited with code"), + "…over a diff that really ran: {diff}" + ); + + let last: Vec = eval(s, "return pmacs.git._last_spawn.args"); + let want: Vec = [ + "--no-optional-locks", + "-C", + &root.display().to_string(), + "diff", + "--no-color", + "HEAD", + "--", + orig, + path, + ] + .iter() + .map(|a| (*a).to_string()) + .collect(); + assert_eq!( + last, want, + "and the two-path invocation is the SAME for both classes — \ + passing both paths is what makes git render the relationship \ + rather than an unrelated add, so a fix to what the user is TOLD \ + must not reach what the module DOES" + ); +} + /// The untracked diff renders **on exit 1**, not a failure row. /// /// `git diff --no-index` implies `--exit-code`: it exits 1 when it