From 9c0b4337a81895b37303b58ab82454cdf3c0dfc5 Mon Sep 17 00:00:00 2001 From: EMRG Evolution Date: Tue, 15 Sep 2026 23:11:13 +0800 Subject: [PATCH 1/2] emrg: catch a patch that silently dropped a file, which applying and green cannot A patch that loses a file passes every check a maintainer naturally runs: it applies cleanly and the suite is green, because the missing file is not there to fail. This repo did exactly that - a fix was published whose only test file had been dropped during a rebuild - and the cheap check that catches it is a set comparison of the two patches' file lists. scripts/check-patch-files.py reports any file an earlier patch carried that a later one does not, with its hunk count. A gain is reported but does not fail. Exit 0 = nothing dropped, 1 = a file dropped, 2 = unmeasurable (no patches, one patch, an unreadable path, or a patch that parsed to zero files). That last exit code is the one that keeps the tool honest: a parser that matched nothing agrees with every input, so 'no file disappeared' would then be a verdict about the parser rather than about the patches. Verified against the real artifacts whose verdicts are known: armG -> armH (no drop) rc=0, and the historic armD -> armF publication, where the test file really was lost, rc=1 naming it. tests/test_check_patch_files.py: 16 cases, both directions (a guard that fires on everything is as useless as one that never fires), including the output being ASCII and the exit codes surviving a console codec that cannot encode anything else. Mutation-tested: removing the drop detection, making a zero-file patch pass, splitting a quoted header on spaces, and counting gains as losses each turn a named test red (4/4 killed), with the script restored byte-for-byte. --- scripts/check-patch-files.py | 214 +++++++++++++++++++++++++++++ tests/test_check_patch_files.py | 231 ++++++++++++++++++++++++++++++++ 2 files changed, 445 insertions(+) create mode 100644 scripts/check-patch-files.py create mode 100644 tests/test_check_patch_files.py diff --git a/scripts/check-patch-files.py b/scripts/check-patch-files.py new file mode 100644 index 00000000..c8f5e13a --- /dev/null +++ b/scripts/check-patch-files.py @@ -0,0 +1,214 @@ +#!/usr/bin/env python3 +"""Check that a rebuilt patch did not silently drop a file the previous one had. + +Why this exists +--------------- +A patch that drops a test file still satisfies every check a maintainer would +naturally run: it *applies* cleanly, and the suite is *green* - because the +missing file is simply not there to fail. This repo hit exactly that: a fix was +published whose only test file had been dropped while the patch was rebuilt from +a working tree that contained just the two files being edited that cycle. The +code survived (it lives in the file that descended from the previous version), +nothing failed, and a reader comparing the two patches' file lists could see the +loss immediately. + +So the free check is a set comparison: list the files each patch touches, and +report any file present in a previous patch and absent from a later one. A +disappearance is either deliberate (and then it should be stated in the publish +text) or it is the defect. + +Usage +----- + python3 scripts/check-patch-files.py old.patch new.patch + python3 scripts/check-patch-files.py a.patch b.patch c.patch # each vs the prior + python3 scripts/check-patch-files.py --list a.patch b.patch + +Exit codes +---------- +0 every file a previous patch carried is still carried by the next one +1 a file disappeared from the patch (named, with its hunk count) +2 the question could not be answered - no patches given, a patch is unreadable, + a single patch without --list (there is nothing to compare it against), or a + patch that parsed to zero files. The last one matters: a parser that matched + nothing would report "nothing wrong" for every input, so it must never be + reported as a pass. + +The tool reads the patch files it is given, not the working tree; its first +output line names every patch it opened, so a reader can tell what was measured. +""" + +from __future__ import annotations + +import argparse +import re +import shlex +import sys +from dataclasses import dataclass +from pathlib import Path + +DIFF_HEADER = "diff --git " +HUNK_MARKER = "@@ " +NEW_FILE_MODE = "new file mode " +DELETED_FILE_MODE = "deleted file mode " + +DESCRIPTION = ( + "Compare the file lists of two or more patches and fail if a file carried by " + "an earlier patch is missing from a later one." +) + + +@dataclass(frozen=True) +class Entry: + """One file a patch touches.""" + + path: str + hunks: int + is_new: bool + is_deleted: bool + + def label(self) -> str: + kind = "new file" if self.is_new else ("deletes" if self.is_deleted else "edits") + return f"{kind} {self.path}" + (f" ({self.hunks} hunks)" if self.hunks else "") + + +def _path_from_diff_header(line: str) -> str | None: + """The post-image path of a `diff --git` header, or None if unparseable. + + ``shlex`` rather than a regex: git quotes a path containing whitespace, so + splitting on spaces silently truncates exactly the paths a regex is most + likely to mis-handle. + """ + rest = line[len(DIFF_HEADER):] + try: + parts = shlex.split(rest) + except ValueError: + parts = rest.split() + if len(parts) < 2: + return None + target = parts[1] + return target[2:] if target.startswith("b/") else target + + +def parse_patch(text: str) -> dict[str, Entry]: + """{path: Entry} for every file the patch touches, in the order git wrote them.""" + entries: dict[str, Entry] = {} + current: str | None = None + hunks = 0 + is_new = is_deleted = False + + def flush() -> None: + if current is not None: + entries[current] = Entry(current, hunks, is_new, is_deleted) + + for line in text.splitlines(): + if line.startswith(DIFF_HEADER): + flush() + current = _path_from_diff_header(line) + hunks, is_new, is_deleted = 0, False, False + continue + if current is None: + continue + if line.startswith(HUNK_MARKER): + hunks += 1 + elif line.startswith(NEW_FILE_MODE): + is_new = True + elif line.startswith(DELETED_FILE_MODE): + is_deleted = True + flush() + return entries + + +def _describe(label: str, entries: dict[str, Entry]) -> None: + print(f" {label}: {len(entries)} file(s)") + for entry in entries.values(): + print(f" {entry.label()}") + + +def _load(path: str) -> tuple[dict[str, Entry], str] | None: + target = Path(path) + if not target.is_file(): + print(f"unmeasurable: no such patch file: {path}") + return None + try: + text = target.read_text(encoding="utf-8", errors="replace") + except OSError as exc: # unreadable file (permissions, a directory, ...) + print(f"unmeasurable: cannot read {path}: {exc}") + return None + return parse_patch(text), text + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=DESCRIPTION) + parser.add_argument("patches", nargs="*", help="patch files, oldest first") + parser.add_argument( + "--list", + action="store_true", + help="print each patch's file set and stop (no comparison)", + ) + args = parser.parse_args(argv) + + if not args.patches: + parser.print_help() + return 2 + if len(args.patches) == 1 and not args.list: + print( + "unmeasurable: one patch given and nothing to compare it against; " + f"pass a second patch or use --list ({args.patches[0]})" + ) + return 2 + + print("patches read:") + for path in args.patches: + print(f" {path}") + + loaded: list[tuple[str, dict[str, Entry]]] = [] + for path in args.patches: + result = _load(path) + if result is None: + return 2 + entries = result[0] + if not entries: + # The failure this guard must not have: a parser that matched + # nothing agrees with every input, so "no file disappeared" would + # be a verdict about the parser, not about the patches. + print( + f"unmeasurable: {path} parsed to 0 files - it is empty, not a diff, " + "or in a format this tool does not read; reporting 'nothing wrong' " + "here would be a verdict about the parser" + ) + return 2 + loaded.append((path, entries)) + + if args.list: + for path, entries in loaded: + print() + _describe(Path(path).name, entries) + return 0 + + dropped_any = False + for (prev_path, prev), (cur_path, cur) in zip(loaded, loaded[1:]): + lost = [prev[f] for f in prev if f not in cur] + gained = [cur[f] for f in cur if f not in prev] + print() + print(f"{Path(prev_path).name} -> {Path(cur_path).name}") + _describe("previous", prev) + _describe("current ", cur) + if gained: + print(f" gained: {[e.path for e in gained]}") + if lost: + dropped_any = True + print(f" FAIL: {len(lost)} file(s) dropped") + for entry in lost: + print(f" {entry.label()}") + print( + " A dropped file is either deliberate (say so where the patch " + "is published) or a rebuild defect: the patch still applies and the " + "suite is still green, because the file is absent." + ) + else: + print(" OK: no file the previous patch carried is missing from this one") + return 1 if dropped_any else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_check_patch_files.py b/tests/test_check_patch_files.py new file mode 100644 index 00000000..113c82b2 --- /dev/null +++ b/tests/test_check_patch_files.py @@ -0,0 +1,231 @@ +"""Tests for scripts/check-patch-files.py - the file-set comparison. + +Background +---------- +A patch that drops a file is invisible to every check a maintainer naturally +runs: it applies cleanly, and the suite is green, because the file is not there +to fail. This repo hit exactly that - a fix was published whose only test file +had been dropped during a rebuild - and the free check that catches it is a set +comparison of the two patches' file lists. + +The tests below pin the properties that make the verdict trustworthy, in both +directions, because a guard that fires on everything is as useless as one that +never fires: + +* a drop is reported with the file named, and the exit code is 1; +* a gain is reported and the exit code stays 0, and a file the patch *deletes* + still counts as present (the tool compares file sets, not live files); +* the patches are compared pairwise, so a drop between the second and the third + is not missed while the second is compared against the first; +* a patch that parses to **zero** files exits 2 rather than 0. This is the + property that keeps the tool honest: a parser that matched nothing agrees with + every input, so "no file disappeared" would then be a statement about the + parser instead of about the patches; +* everything whose answer cannot be measured (no patches, one patch, an + unreadable path) exits 2, never 0; +* the output is ASCII and the exit codes survive a console codec that cannot + encode anything else. +""" + +from __future__ import annotations + +import importlib.util +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +SCRIPT = REPO_ROOT / "scripts" / "check-patch-files.py" + + +def _load_module(): + spec = importlib.util.spec_from_file_location("check_patch_files", SCRIPT) + mod = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = mod + spec.loader.exec_module(mod) + return mod + + +@pytest.fixture +def mod(): + return _load_module() + + +# ── synthetic patches ──────────────────────────────────────────────────────── + +def _quote(token: str) -> str: + """git quotes a *whole* header token - `"a/my file.md"`, never `a/"my file.md"`. + + It quotes when the token contains whitespace, a double quote or a backslash, + so a fixture that emits such a path unquoted is not a patch git would ever + write, and the parser is entitled to misread it. + """ + if any(ch in token for ch in ' \t"\\'): + return '"' + token.replace("\\", "\\\\").replace('"', '\\"') + '"' + return token + + +def _file_section(path: str, hunks: int = 1, *, new: bool = False, deleted: bool = False) -> str: + a, b = _quote(f"a/{path}"), _quote(f"b/{path}") + out = f"diff --git {a} {b}\n" + if new: + out += "new file mode 100644\n" + if deleted: + out += "deleted file mode 100644\n" + out += f"--- {a}\n+++ {b}\n" + out += "@@ -1,1 +1,1 @@\n" * hunks + return out + + +def _write(tmp_path: Path, name: str, *sections: str) -> str: + path = tmp_path / name + path.write_text("".join(sections), encoding="utf-8") + return str(path) + + +# ── the verdict ────────────────────────────────────────────────────────────── + +def test_no_drop_is_ok(tmp_path, mod, capsys): + old = _write(tmp_path, "old.patch", _file_section("a.py"), _file_section("b.py")) + new = _write(tmp_path, "new.patch", _file_section("a.py", 2), _file_section("b.py")) + + assert mod.main([old, new]) == 0 + out = capsys.readouterr().out + assert "OK: no file the previous patch carried is missing" in out + + +def test_a_dropped_file_is_named_and_fails(tmp_path, mod, capsys): + old = _write(tmp_path, "old.patch", _file_section("a.py"), _file_section("tests/t.py", 5)) + new = _write(tmp_path, "new.patch", _file_section("a.py")) + + assert mod.main([old, new]) == 1 + out = capsys.readouterr().out + assert "tests/t.py" in out + assert "5 hunks" in out, "the dropped file's hunk count is what makes the loss legible" + assert "FAIL" in out + + +def test_a_gained_file_is_reported_but_passes(tmp_path, mod, capsys): + old = _write(tmp_path, "old.patch", _file_section("a.py")) + new = _write(tmp_path, "new.patch", _file_section("a.py"), _file_section("c.py")) + + assert mod.main([old, new]) == 0 + out = capsys.readouterr().out + assert "gained" in out and "c.py" in out + + +def test_a_file_the_patch_deletes_is_still_present(tmp_path, mod): + """The tool compares file *sets*, not live files: a deletion is not a drop.""" + old = _write(tmp_path, "old.patch", _file_section("a.py"), _file_section("gone.py")) + new = _write(tmp_path, "new.patch", _file_section("a.py"), _file_section("gone.py", deleted=True)) + + assert mod.main([old, new]) == 0 + + +def test_a_new_file_is_marked_new(tmp_path, mod, capsys): + p = _write(tmp_path, "p.patch", _file_section("fresh.py", new=True)) + assert mod.main(["--list", p]) == 0 + assert "new file fresh.py" in capsys.readouterr().out + + +def test_three_patches_are_compared_pairwise(tmp_path, mod, capsys): + a = _write(tmp_path, "a.patch", _file_section("x.py"), _file_section("keep.py")) + b = _write(tmp_path, "b.patch", _file_section("x.py"), _file_section("keep.py")) + c = _write(tmp_path, "c.patch", _file_section("x.py")) + + # The drop is between b and c; comparing only a against b would miss it. + assert mod.main([a, b, c]) == 1 + assert "keep.py" in capsys.readouterr().out + + +def test_a_quoted_path_with_a_space_is_parsed(tmp_path, mod, capsys): + """git quotes a path containing whitespace; the header must survive it.""" + old = _write(tmp_path, "old.patch", _file_section("docs/my file.md")) + new = _write(tmp_path, "new.patch", _file_section("docs/my file.md", 3)) + assert '"a/docs/my file.md" "b/docs/my file.md"' in ( + tmp_path / "new.patch" + ).read_text(encoding="utf-8"), "the fixture must be a patch git would actually write" + + assert mod.main([old, new]) == 0 + out = capsys.readouterr().out + assert "docs/my file.md" in out + assert "file.md (3 hunks)" in out, "the whole quoted path is one file, not two" + + +# ── unmeasurable must never read as a pass ─────────────────────────────────── + +def test_a_patch_with_no_diff_headers_is_unmeasurable(tmp_path, mod, capsys): + """The property that keeps the tool honest. + + A patch this tool cannot read yields an empty file set, which would agree + with every other patch and print OK. Zero files is a statement about the + parser, so it must exit 2. + """ + old = _write(tmp_path, "old.patch", _file_section("a.py")) + empty = _write(tmp_path, "empty.patch", "this is not a diff\n") + + assert mod.main([old, empty]) == 2 + out = capsys.readouterr().out + assert "unmeasurable" in out and "parsed to 0 files" in out + assert "OK:" not in out, "an unreadable patch must not produce a pass verdict" + + +def test_a_missing_patch_file_is_unmeasurable(tmp_path, mod, capsys): + old = _write(tmp_path, "old.patch", _file_section("a.py")) + assert mod.main([old, str(tmp_path / "nope.patch")]) == 2 + assert "unmeasurable" in capsys.readouterr().out + + +def test_one_patch_without_list_is_unmeasurable(tmp_path, mod, capsys): + old = _write(tmp_path, "old.patch", _file_section("a.py")) + assert mod.main([old]) == 2 + assert "nothing to compare it against" in capsys.readouterr().out + + +def test_no_patches_is_unmeasurable(mod, capsys): + assert mod.main([]) == 2 + assert "usage" in capsys.readouterr().out.lower() + + +def test_the_measured_patches_are_named(tmp_path, mod, capsys): + """A reader must be able to tell what was read, not just what was concluded.""" + old = _write(tmp_path, "old.patch", _file_section("a.py")) + new = _write(tmp_path, "new.patch", _file_section("a.py")) + mod.main([old, new]) + out = capsys.readouterr().out + assert "patches read:" in out and "old.patch" in out and "new.patch" in out + + +# ── as a subprocess: exit codes and ASCII under a hostile codec ────────────── + +def _run(args: list[str], codec: str) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(SCRIPT), *args], + capture_output=True, + check=False, + env=dict(os.environ, PYTHONIOENCODING=codec), + ) + + +@pytest.mark.parametrize("codec", ["ascii", "gbk", "cp1252"]) +def test_verdict_reaches_a_console_that_cannot_encode_typography(tmp_path, codec): + old = _write(tmp_path, "old.patch", _file_section("a.py"), _file_section("t.py")) + new = _write(tmp_path, "new.patch", _file_section("a.py")) + + proc = _run([old, new], codec) + assert proc.returncode == 1, proc.stdout + proc.stderr + assert b"t.py" in proc.stdout + assert proc.stdout == proc.stdout.decode("ascii", errors="ignore").encode("ascii"), ( + "the verdict line must be ASCII: a caller piping the output to a file or a " + "captured pipe must not turn a verdict into a UnicodeEncodeError" + ) + + +def test_help_exits_zero_and_is_ascii(mod): + proc = _run(["--help"], "ascii") + assert proc.returncode == 0, proc.stdout + proc.stderr + assert b"check-patch-files.py" in proc.stdout + assert proc.stdout.decode("ascii") # raises if any byte is non-ASCII From 787a26cf99989c6e3c9de45eecc83a6fe4c09603 Mon Sep 17 00:00:00 2001 From: EMRG Evolution Date: Tue, 15 Sep 2026 23:54:20 +0800 Subject: [PATCH 2/2] emrg: name a patch section's file from the line git writes it on (#1246) The file-set guard split each `diff --git` header on whitespace, but git does not quote a path merely for containing a space: it writes `diff --git a/a b.txt b/a b.txt` bare. The parse then read `a b.txt` as `b.txt`, so a rebuilt patch that really dropped `a b.txt` parsed to the same file set as the patch before it and the tool printed OK - the silent pass its own exit codes were introduced to prevent (reported by a reviewer on #1246, reproduced here on real repositories). The name now comes from the line git writes it on alone: `+++ b/`, or `--- a/` for a deletion, or `rename to ` for a rename, with the header kept as the fallback for the two shapes that carry no name line (a mode-only change, a binary section). Those lines terminate a space-bearing name with a tab, which makes the reading exact, and git's C-quoted spellings are unquoted before they are reported. A section whose file cannot be named now makes the run unmeasurable instead of being skipped: a file the parser cannot see is absent from both patches, so skipping it would agree with itself and print OK from the other side. Measured against git's own `--name-only` answer on nine real patch shapes (space, nested space, unicode, quote, backslash, tab, binary, mode-only, pure rename): the old parse matched 1/9, this one 9/9. Four mutations of the fix (the old parse, skipping an unnameable section, dropping git's tab terminator, not undoing the quoting) each turn the new tests red. Agent.md names the command, so the guard is run rather than remembered; the file is 7939 of the 8000 chars the daemon keeps, pinned by test_agent_md_prompt_cap. --- Agent.md | 2 +- scripts/check-patch-files.py | 241 ++++++++++++++++++++++++++------ tests/test_check_patch_files.py | 217 +++++++++++++++++++++++++--- 3 files changed, 401 insertions(+), 59 deletions(-) diff --git a/Agent.md b/Agent.md index 76a4c84a..72be22ad 100644 --- a/Agent.md +++ b/Agent.md @@ -60,7 +60,7 @@ GUI: `cd emrg/gui && npm test` (100: 44 daemon_client + 20 conn-manager + 7 inte Renderer: `cd emrg/gui/renderer && npm run typecheck && npm test` (514: 5 snapshot-store + 9 utils + 3 ErrorBoundary + 2 App smoke + 11 commands + 4 copywriting + 11 i18n + 13 markdown + 21 transcript + 11 TranscriptView + 15 history + 31 composer + 41 Composer + 6 LinkDialog + 16 sidebar + 17 Sidebar + 9 fileTree + 9 FileTree + 16 resultPanel + 8 ResultPanel + 27 workspaceView + 10 WorkspaceView + 10 dialog + 6 Dialog + 9 ConfirmDialog + 9 RenameDialog + 10 dialogLists + 3 HelpDialog + 9 MemoryDialog + 6 SkillsDialog + 8 openSession + 6 WelcomeDialog + 9 OpenSessionDialog + 7 NewSessionDialog + 7 rewind + 8 RewindDialog + 7 GithubDeviceDialog + 18 daemonBridge + 7 DaemonBridgeProvider + 30 Shell + 15 DialogHost + 20 SettingsPanel + 6 TaskFormDialog + 5 RantDialog + 4 vendorMarkdown) - CI: pytest on ubuntu **and windows-2025**, the GUI suites, and an actionlint gate over `.github/workflows/`; re-trigger a dropped push event with `scripts/re-trigger-ci.sh `, never an empty commit - Merge gates, run before merging — each answers a question no other gate asks: `uv run --no-sync python3 scripts/check-vote-count.py` (approvals still valid) · `scripts/check-pr-base.py` (base reaches master) · `scripts/check-merge-freshness.py` (that green CI is about the landing tree) · `scripts/check-merge-order.py` (what merging dirties) · `scripts/check-merge-landing-diff.py` (what the landing changes) · `scripts/check-merge-pairs.py` / `scripts/check-merge-sequence.py` (two PRs together; every step of an order) · `scripts/check-merge-tree-health.py` (the merged tree passes the guards) · `scripts/check-merge-plan-suite.py` (the plan's final tree passes the suite) — all under `uv run --no-sync python3` -- Other tools: `uv run --no-sync python3 scripts/check-node-test-count.py --write` (syncs the two Node totals above), `bump-version.py`, `classify-conflict.py`, `sync-master-from-api.py`, `push-branch-from-api.py` +- Other tools: `uv run --no-sync python3 scripts/check-node-test-count.py --write` (syncs the two Node totals above), `scripts/check-patch-files.py` (a rebuilt patch still carries every file the previous one did), `bump-version.py`, `classify-conflict.py`, `sync-master-from-api.py`, `push-branch-from-api.py` Every guard reads the tree you are **standing in** and says so in its first line; a question it cannot answer is reported unmeasurable, never as a pass. diff --git a/scripts/check-patch-files.py b/scripts/check-patch-files.py index c8f5e13a..8cecee17 100644 --- a/scripts/check-patch-files.py +++ b/scripts/check-patch-files.py @@ -28,10 +28,12 @@ 0 every file a previous patch carried is still carried by the next one 1 a file disappeared from the patch (named, with its hunk count) 2 the question could not be answered - no patches given, a patch is unreadable, - a single patch without --list (there is nothing to compare it against), or a - patch that parsed to zero files. The last one matters: a parser that matched - nothing would report "nothing wrong" for every input, so it must never be - reported as a pass. + a single patch without --list (there is nothing to compare it against), a + patch that parsed to zero files, or a section whose file cannot be named. The + last two matter for the same reason: a file the parser cannot see is a file + that is missing from *both* patches, so it would make "nothing disappeared" + a statement about the parser. An unnameable section is therefore never + skipped - it makes the whole comparison unmeasurable. The tool reads the patch files it is given, not the working tree; its first output line names every patch it opened, so a reader can tell what was measured. @@ -41,15 +43,26 @@ import argparse import re -import shlex import sys from dataclasses import dataclass from pathlib import Path DIFF_HEADER = "diff --git " HUNK_MARKER = "@@ " +PLUS_NAME = "+++ " +MINUS_NAME = "--- " +RENAME_TO = "rename to " NEW_FILE_MODE = "new file mode " DELETED_FILE_MODE = "deleted file mode " +NULL_PATH = "/dev/null" + +# The escapes git's C-style quoting uses, besides octal byte escapes. Measured +# on a real repository: a path is quoted exactly when it holds one of these, a +# non-ASCII byte, or a control character - a space alone does NOT quote it. +_C_ESCAPES = { + "a": "\a", "b": "\b", "f": "\f", "n": "\n", "r": "\r", "t": "\t", "v": "\v", + '"': '"', "\\": "\\", +} DESCRIPTION = ( "Compare the file lists of two or more patches and fail if a file carried by " @@ -71,50 +84,191 @@ def label(self) -> str: return f"{kind} {self.path}" + (f" ({self.hunks} hunks)" if self.hunks else "") -def _path_from_diff_header(line: str) -> str | None: - """The post-image path of a `diff --git` header, or None if unparseable. +class UnparseableSection(Exception): + """A `diff --git` section whose file cannot be named. + + Raised rather than skipped: an unnamed file is absent from the parsed set of + every patch, so it would agree with itself and turn "nothing disappeared" + into a verdict about this parser (see the exit codes). + """ + + +def _closing_quote(text: str) -> int | None: + """Index of the quote closing the C-quoted name `text` starts with.""" + i = 1 + while i < len(text): + if text[i] == "\\": + i += 2 + continue + if text[i] == '"': + return i + i += 1 + return None + - ``shlex`` rather than a regex: git quotes a path containing whitespace, so - splitting on spaces silently truncates exactly the paths a regex is most - likely to mis-handle. +def _unquote(token: str) -> str: + """git's C-style quoting undone: ``"b/w\\303\\255th.txt"`` -> ``b/wíth.txt``. + + git spells a non-ASCII byte as octal, so the escapes decode to bytes and the + bytes to UTF-8. Anything that does not round-trip comes back untouched + rather than guessed at. """ - rest = line[len(DIFF_HEADER):] + if not token.startswith('"'): + return token + end = _closing_quote(token) + if end is None: + return token + body = token[1:end] + out = bytearray() + i = 0 try: - parts = shlex.split(rest) - except ValueError: - parts = rest.split() - if len(parts) < 2: - return None - target = parts[1] - return target[2:] if target.startswith("b/") else target + while i < len(body): + char = body[i] + if char != "\\": + out += char.encode("utf-8") + i += 1 + continue + i += 1 + if i >= len(body): + return token + escape = body[i] + if escape in "01234567": + j = i + while j < len(body) and j - i < 3 and body[j] in "01234567": + j += 1 + out.append(int(body[i:j], 8)) + i = j + elif escape in _C_ESCAPES: + out += _C_ESCAPES[escape].encode("utf-8") + i += 1 + else: + return token + return out.decode("utf-8") + except (ValueError, UnicodeDecodeError): + return token + + +def _section_token(rest: str) -> str | None: + """The one path on a `---`/`+++`/`rename to` line, or None if malformed. + + git terminates an *unquoted* name containing a space with a tab - measured: + ``+++ b/a b.txt`` - and quotes a name it cannot print plainly, so the + first tab after an unquoted name is always git's terminator and never part of + the name. + """ + rest = rest.rstrip("\r\n") + if rest.startswith('"'): + end = _closing_quote(rest) + return None if end is None else _unquote(rest[: end + 1]) + return rest.split("\t", 1)[0] or None -def parse_patch(text: str) -> dict[str, Entry]: - """{path: Entry} for every file the patch touches, in the order git wrote them.""" - entries: dict[str, Entry] = {} - current: str | None = None - hunks = 0 - is_new = is_deleted = False +def _strip_side(path: str, side: str) -> str: + """`b/` -> ``; git writes its side prefix on every one of these lines.""" + return path[len(side):] if path.startswith(side) else path + + +def _head(section: list[str]) -> list[str]: + """A section's lines up to its first hunk - where git writes the names.""" + for i, line in enumerate(section): + if line.startswith(HUNK_MARKER): + return section[:i] + return section - def flush() -> None: - if current is not None: - entries[current] = Entry(current, hunks, is_new, is_deleted) +def _path_from_diff_header(line: str) -> str | None: + """The post-image path in a `diff --git` header, or None when it is ambiguous. + + git writes `a/ b/`, quoting a whole token only when that token + needs C-escaping. It does *not* quote a path merely for containing a space + (measured: ``diff --git a/a b.txt b/a b.txt``), so an unquoted header cannot + be split on whitespace. The split is instead chosen by plausibility: the two + sides agreeing is the reading of every section that is not a rename, which + resolves the ambiguous-looking case uniquely. A header with more than one + plausible reading and no agreeing one is reported as unknown rather than + guessed - a wrong name is invisible in both patches, which is the failure + this tool exists to prevent. + """ + rest = line[len(DIFF_HEADER):].rstrip("\r\n") + if rest.startswith('"'): + end = _closing_quote(rest) + if end is None: + return None + remainder = rest[end + 1:].strip() + token = _section_token(remainder) if remainder else None + return None if token is None else _strip_side(token, "b/") + + candidates: list[tuple[str, str]] = [] + for match in re.finditer(" b/", rest): + left, right = rest[: match.start()], rest[match.start() + 1:] + if left.startswith("a/") and right.startswith("b/"): + candidates.append((left[2:], right[2:])) + for left, right in candidates: + if left == right: + return right + if len(candidates) == 1: + return candidates[0][1] + return None + + +def _path_from_section(section: list[str]) -> str | None: + """The one file a section touches, or None if it cannot be named. + + Each source is a line git writes the name on alone, in the order that makes + the post-image path the answer for every shape measured: `+++ b/` (an + edit or an addition), `--- a/` (a deletion), `rename to ` (a + rename). The header is the fallback for the two shapes that carry no name + line of their own - a mode-only change and a binary section. + """ + head = _head(section) + for line in head: + if line.startswith(PLUS_NAME): + token = _section_token(line[len(PLUS_NAME):]) + if token and token != NULL_PATH: + return _strip_side(token, "b/") + for line in head: + if line.startswith(MINUS_NAME): + token = _section_token(line[len(MINUS_NAME):]) + if token and token != NULL_PATH: + return _strip_side(token, "a/") + for line in head: + if line.startswith(RENAME_TO): + token = _section_token(line[len(RENAME_TO):]) + if token: + return _strip_side(token, "b/") + return _path_from_diff_header(section[0]) + + +def _split_sections(text: str) -> list[list[str]]: + """The patch cut at every `diff --git` header; text before the first is ignored.""" + sections: list[list[str]] = [] for line in text.splitlines(): if line.startswith(DIFF_HEADER): - flush() - current = _path_from_diff_header(line) - hunks, is_new, is_deleted = 0, False, False - continue - if current is None: - continue - if line.startswith(HUNK_MARKER): - hunks += 1 - elif line.startswith(NEW_FILE_MODE): - is_new = True - elif line.startswith(DELETED_FILE_MODE): - is_deleted = True - flush() + sections.append([line]) + elif sections: + sections[-1].append(line) + return sections + + +def parse_patch(text: str) -> dict[str, Entry]: + """{path: Entry} for every file the patch touches, in the order git wrote them. + + Raises UnparseableSection when a section's file cannot be named - see the + exit codes: an unreadable section must make the comparison unmeasurable + instead of quietly shrinking both file sets. + """ + entries: dict[str, Entry] = {} + for section in _split_sections(text): + path = _path_from_section(section) + if path is None: + raise UnparseableSection(section[0]) + head = _head(section) + entries[path] = Entry( + path, + sum(1 for line in section if line.startswith(HUNK_MARKER)), + any(line.startswith(NEW_FILE_MODE) for line in head), + any(line.startswith(DELETED_FILE_MODE) for line in head), + ) return entries @@ -134,7 +288,12 @@ def _load(path: str) -> tuple[dict[str, Entry], str] | None: except OSError as exc: # unreadable file (permissions, a directory, ...) print(f"unmeasurable: cannot read {path}: {exc}") return None - return parse_patch(text), text + try: + entries = parse_patch(text) + except UnparseableSection as exc: + print(f"unmeasurable: cannot name the file of this section: {exc}") + return None + return entries, text def main(argv: list[str] | None = None) -> int: diff --git a/tests/test_check_patch_files.py b/tests/test_check_patch_files.py index 113c82b2..57733c9c 100644 --- a/tests/test_check_patch_files.py +++ b/tests/test_check_patch_files.py @@ -56,26 +56,66 @@ def mod(): # ── synthetic patches ──────────────────────────────────────────────────────── -def _quote(token: str) -> str: - """git quotes a *whole* header token - `"a/my file.md"`, never `a/"my file.md"`. +def _needs_quoting(name: str) -> bool: + """Does git have to C-quote this name? Measured, not assumed. + + git quotes when the name holds a byte it cannot print plainly - a non-ASCII + byte, a control character, `"` or `\\`. A **space does not quote it**, which + is the whole reason this file's fixtures were rebuilt: the previous version + of this suite emitted `"a/my file.md" "b/my file.md"`, a header git never + writes, and so tested the belief instead of the format. + """ + return any( + chr(b) == '"' or chr(b) == "\\" or not (0x20 <= b < 0x7F) for b in name.encode("utf-8") + ) + - It quotes when the token contains whitespace, a double quote or a backslash, - so a fixture that emits such a path unquoted is not a patch git would ever - write, and the parser is entitled to misread it. +def _c_quote(name: str) -> str: + """git's quoting of a whole name: escapes for `"` `\\` tab, octal for other bytes.""" + out: list[str] = [] + for byte in name.encode("utf-8"): + char = chr(byte) + if char == '"': + out.append('\\"') + elif char == "\\": + out.append("\\\\") + elif char == "\t": + out.append("\\t") + elif 0x20 <= byte < 0x7F: + out.append(char) + else: + out.append("\\%03o" % byte) + return '"' + "".join(out) + '"' + + +def _header_token(prefix: str, name: str) -> str: + token = prefix + name + return _c_quote(token) if _needs_quoting(token) else token + + +def _name_line(marker: str, name: str) -> str: + """A `--- a/` / `+++ b/` line, spelled the way git spells it. + + Measured on a real repository: a name git can print plainly is written bare, + **with a tab appended when it contains a space** (`+++ b/a b.txt`), and a + name it cannot print plainly is C-quoted instead. That terminator is why the + path can be read off one of these lines unambiguously. """ - if any(ch in token for ch in ' \t"\\'): - return '"' + token.replace("\\", "\\\\").replace('"', '\\"') + '"' - return token + token = marker + name + if _needs_quoting(token): + return _c_quote(token) + return token + ("\t" if " " in token else "") def _file_section(path: str, hunks: int = 1, *, new: bool = False, deleted: bool = False) -> str: - a, b = _quote(f"a/{path}"), _quote(f"b/{path}") - out = f"diff --git {a} {b}\n" + out = f"diff --git {_header_token('a/', path)} {_header_token('b/', path)}\n" if new: out += "new file mode 100644\n" if deleted: out += "deleted file mode 100644\n" - out += f"--- {a}\n+++ {b}\n" + # git writes /dev/null on the side the file does not exist on. + out += f"{'--- /dev/null' if new else _name_line('--- a/', path)}\n" + out += f"{'+++ /dev/null' if deleted else _name_line('+++ b/', path)}\n" out += "@@ -1,1 +1,1 @@\n" * hunks return out @@ -141,18 +181,161 @@ def test_three_patches_are_compared_pairwise(tmp_path, mod, capsys): assert "keep.py" in capsys.readouterr().out -def test_a_quoted_path_with_a_space_is_parsed(tmp_path, mod, capsys): - """git quotes a path containing whitespace; the header must survive it.""" +def test_a_space_in_a_path_is_written_unquoted_and_still_parsed(tmp_path, mod, capsys): + """The shape git actually writes for a space, and the header must survive it. + + Measured on a real repository: a path whose only unusual character is a space + is written **unquoted** (`diff --git a/a b.txt b/a b.txt`) with a tab + terminator on the `---`/`+++` lines. The previous version of this test + asserted the *quoted* form and passed, because its fixture and the parser + shared the same wrong belief about the format. + """ old = _write(tmp_path, "old.patch", _file_section("docs/my file.md")) new = _write(tmp_path, "new.patch", _file_section("docs/my file.md", 3)) - assert '"a/docs/my file.md" "b/docs/my file.md"' in ( - tmp_path / "new.patch" - ).read_text(encoding="utf-8"), "the fixture must be a patch git would actually write" + written = (tmp_path / "new.patch").read_text(encoding="utf-8") + assert written.startswith("diff --git a/docs/my file.md b/docs/my file.md\n"), written + assert "+++ b/docs/my file.md\t\n" in written, "git terminates a space-bearing name with a tab" assert mod.main([old, new]) == 0 out = capsys.readouterr().out assert "docs/my file.md" in out - assert "file.md (3 hunks)" in out, "the whole quoted path is one file, not two" + assert "file.md (3 hunks)" in out, "the whole path is one file, not two" + + +def test_a_truncated_name_that_collides_is_no_longer_a_false_pass(tmp_path, mod, capsys): + """The verdict this tool must never get wrong (reported on PR #1246). + + Splitting the header on whitespace read `a b.txt` as `b.txt`, so a previous + patch carrying {a b.txt, b.txt, keep.txt} and a rebuilt one carrying only + {b.txt, keep.txt} had the *same* parsed set: the loss of `a b.txt` was + invisible and the tool printed OK. A wrong name is invisible in both patches, + which is why this case is a false pass and not merely a cosmetic mistake. + """ + old = _write( + tmp_path, "old.patch", + _file_section("a b.txt"), _file_section("b.txt"), _file_section("keep.txt"), + ) + new = _write(tmp_path, "new.patch", _file_section("b.txt"), _file_section("keep.txt")) + + assert mod.main([old, new]) == 1, "the drop must be caught, not silently agreed with" + out = capsys.readouterr().out + assert "a b.txt" in out + assert "OK: no file" not in out + + +def test_a_c_quoted_name_is_reported_as_its_real_name(tmp_path, mod, capsys): + """git escapes a name it cannot print plainly; the report shows the name, not the escapes.""" + name = "docs/w\u00edth-\u00fcn\u00efcode\tq.txt" + patch = _write(tmp_path, "p.patch", _file_section(name, 2)) + written = (tmp_path / "p.patch").read_text(encoding="utf-8") + assert '\\303\\255' in written, "the fixture must carry git's octal escape, not the character" + + assert mod.main(["--list", patch]) == 0 + out = capsys.readouterr().out + assert f"edits {name} (2 hunks)" in out + assert "\\303" not in out, "the escaped spelling must not reach the report" + + +def test_a_mode_only_change_is_named_from_the_header(tmp_path, mod, capsys): + """A mode-only section has no `---`/`+++` line at all: the header is the only source.""" + section = "diff --git a/a b.txt b/a b.txt\nold mode 100644\nnew mode 100755\n" + patch = _write(tmp_path, "p.patch", section) + assert "+++ " not in section and "--- " not in section + + assert mod.main(["--list", patch]) == 0 + assert "edits a b.txt" in capsys.readouterr().out + + +def test_a_binary_section_with_no_name_lines_is_named(tmp_path, mod, capsys): + """A binary change carries no hunks and no name line either.""" + section = ( + "diff --git a/blob.bin b/blob.bin\nindex c94be36..e38375f 100644\n" + "Binary files a/blob.bin and b/blob.bin differ\n" + ) + patch = _write(tmp_path, "p.patch", section) + assert mod.main(["--list", patch]) == 0 + assert "edits blob.bin" in capsys.readouterr().out + + +def test_a_pure_rename_is_named_by_its_new_path(tmp_path, mod, capsys): + """A 100%-similarity rename has no name lines; `rename to` is the unambiguous source.""" + section = ( + "diff --git a/a b.txt b/c d.txt\nsimilarity index 100%\n" + "rename from a b.txt\nrename to c d.txt\n" + ) + patch = _write(tmp_path, "p.patch", section) + assert mod.main(["--list", patch]) == 0 + out = capsys.readouterr().out + assert "edits c d.txt" in out, "the post-image path is the one compared" + assert "a b.txt" not in out + + +def test_a_section_whose_file_cannot_be_named_makes_the_run_unmeasurable(tmp_path, mod, capsys): + """A file the parser cannot name must stop the comparison, not shrink it. + + Skipping the section would leave the *other* file in both patches, so the + comparison would agree with itself and print OK - the same false pass as a + wrong name, arrived at from the other side. The question is unanswerable, and + an unanswerable question is exit 2 here, never a pass. + """ + old = _write(tmp_path, "old.patch", _file_section("keep.py"), "diff --git unreadable\n") + new = _write(tmp_path, "new.patch", _file_section("keep.py")) + + assert mod.main([old, new]) == 2 + out = capsys.readouterr().out + assert "unmeasurable" in out and "cannot name the file" in out + assert "OK:" not in out + + +def test_a_real_git_patch_agrees_with_gits_own_file_list(tmp_path, mod, capsys): + """The ground truth is git's own answer, not a fixture's idea of it. + + Two staged trees, the second rebuilt without the space-bearing file: the + parsed names must equal `git diff --cached --name-only`, and the drop must be + the tool's verdict. + """ + import shutil + import subprocess + + if shutil.which("git") is None: + pytest.skip("git is not reachable") + + def run(*args): + return subprocess.run( + args, cwd=tmp_path, capture_output=True, text=True, + encoding="utf-8", errors="replace", + ) + + def stage(names): + run("git", "rm", "-r", "--cached", "-q", ".") + for path in tmp_path.rglob("*"): + if path.is_file() and ".git" not in path.parts: + path.unlink() + for name in names: + target = tmp_path / name + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text("one\ntwo\n", encoding="utf-8") + run("git", "add", "-A") + names_git = [n for n in run("git", "diff", "--cached", "--name-only").stdout.splitlines() if n] + return run("git", "diff", "--cached").stdout, sorted(names_git) + + run("git", "init", "-q") + run("git", "config", "user.email", "t@example.invalid") + run("git", "config", "user.name", "probe") + + prev_text, prev_names = stage(["keep.txt", "b.txt", "a b.txt"]) + cur_text, cur_names = stage(["keep.txt", "b.txt"]) + assert prev_names == ["a b.txt", "b.txt", "keep.txt"], "git's own list is the ground truth" + + assert sorted(mod.parse_patch(prev_text).keys()) == prev_names + assert sorted(mod.parse_patch(cur_text).keys()) == cur_names + + old = tmp_path / "old.patch" + new = tmp_path / "new.patch" + old.write_text(prev_text, encoding="utf-8") + new.write_text(cur_text, encoding="utf-8") + assert mod.main([str(old), str(new)]) == 1 + assert "a b.txt" in capsys.readouterr().out # ── unmeasurable must never read as a pass ───────────────────────────────────