fix(git): say "copied" when a 2 record is a copy, not "renamed"
Porcelain v2's `2` record covers renames AND copies --- the `<Xscore>` 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 -- <orig> <current>`, 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai
This commit is contained in:
parent
f53cf4f0fd
commit
e94b256cc6
|
|
@ -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 -- <orig> <current>`. 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 `<Xscore>` 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 } } },
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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 — `<Xscore>` 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 -- <orig> <current>`, 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
|
||||
|
|
|
|||
|
|
@ -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 `<Xscore>` 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 -- <orig> <current>` 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
|
||||
|
|
|
|||
|
|
@ -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 `<Xscore>` 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 -- <orig>
|
||||
/// <current>` 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
|
||||
/// `"<said> from <orig>"` and never `"<not_said> from"` — over the
|
||||
/// two-path `git diff HEAD -- <orig> <path>`, 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<String> = eval(s, "return pmacs.git._last_spawn.args");
|
||||
let want: Vec<String> = [
|
||||
"--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
|
||||
|
|
|
|||
Loading…
Reference in New Issue