diff --git a/scripts/check-merge-landing-diff.py b/scripts/check-merge-landing-diff.py index e87e4d4b..86747be7 100755 --- a/scripts/check-merge-landing-diff.py +++ b/scripts/check-merge-landing-diff.py @@ -74,6 +74,16 @@ arm that fires on a shape the two-list rule cannot see, not a repair of an observed false "clean". +The path names here are the real ones +------------------------------------- +Both halves of the measurement need the *name git means*, so the path list is read +with `-z` rather than from the default line-per-path spelling, which quotes a path +holding a quote, a backslash, a control byte, or any non-ASCII byte. The quoted +form is a pathspec matching nothing: measured (`cyc20260914-082347`) on this file's +own fixture with the file named `中文.txt`, the tool answered *clean* where the +same repo names `src/app.py` as reading backwards inside, and printed a name no +resolver can open. See `_changed_paths`. + Exit codes ---------- 0 nothing reads backwards: every path `diff(base, head)` lists is both a path the @@ -422,18 +432,49 @@ def _commit_tree(tree: str, parents: list[str], message: str) -> str: def _changed_paths(a: str, b: str) -> list[tuple[str, str]]: - """`git diff --name-status a b`, as (status, path) pairs, in git's order.""" - proc = _run(["git", "diff", "--name-status", "--no-renames", a, b]) + """`git diff --name-status -z a b`, as (status, path) pairs, in git's order. + + `-z` because the paths have to be **the real ones**, and both halves of this + tool need that: the report prints them, and `_path_reading` hands them back to + git as a pathspec. + + Without `-z` git *quotes* a path that holds a quote, a backslash, a control + byte, or - with the default `core.quotePath=true` - any non-ASCII byte: + `中文.txt` arrives as the text `"\\344\\270\\255\\346\\226\\207.txt"` and + `ftab.txt` as `"f\\ttab.txt"` (measured 2026-09-14, `cyc20260914-082347`, + git 2.50.1, in a scratch repo, both spellings). Such a quoted path is not only + unreadable in the report - it is a pathspec that matches **nothing**, so + `_path_reading` answered "" for both sides of a shared path and the + "reads backwards inside" arm went blind: measured on the fixture this file's + tests use, the same repo reported the hazard for `src/app.py` and reported + *clean* for `中文.txt`, whose reading does print the base's later hunk as a + deletion. A false "clean" is the one answer this tool may not give. + + The siblings read merge-tree's *report*, where there is no `-z` spelling, so + they decode git's quoting instead (`check-merge-tree-health.py::_unquote_path`, + `check-merge-plan-suite.py`, #1212/#1210). Here the question can be put to git + directly: `-z` separates the fields with NUL and quotes nothing, so the bytes + arrive as they are and can be used as a pathspec unchanged. + """ + proc = _run(["git", "diff", "--name-status", "--no-renames", "-z", a, b]) if proc.returncode != 0: raise MeasurementError( f"could not diff {a[:8]}..{b[:8]}: {_diagnosis(proc)}" ) - changed: list[tuple[str, str]] = [] - for line in proc.stdout.splitlines(): - fields = [field.strip() for field in line.split("\t")] - if len(fields) >= 2: - changed.append((fields[0], fields[-1])) - return changed + # `-z` writes NUL after every field, and the status is a field of its own, so + # the payload is `status\0path\0status\0path\0...` (and "no changes" is empty). + fields = proc.stdout.split("\0") + if fields and fields[-1] == "": + fields.pop() + if len(fields) % 2: + # A shape this tool does not know is a measurement failure, never a short + # list: silently dropping the odd field would answer about a change that + # was not read. + raise MeasurementError( + f"diff {a[:8]}..{b[:8]} named {len(fields)} field(s), not status/path " + f"pairs: {proc.stdout!r}" + ) + return [(fields[i], fields[i + 1]) for i in range(0, len(fields), 2)] def _behind_by(base: str, head: str) -> int: diff --git a/tests/test_check_merge_landing_diff.py b/tests/test_check_merge_landing_diff.py index f794c979..a33b880f 100644 --- a/tests/test_check_merge_landing_diff.py +++ b/tests/test_check_merge_landing_diff.py @@ -29,6 +29,7 @@ import importlib.util import re import subprocess +import sys from pathlib import Path import pytest @@ -1035,3 +1036,129 @@ def test_the_refresh_is_effective_even_with_a_shadowing_stray(mod, tmp_path, mon # …and it is the same ref the header names, so the printed spelling and the measured # commit agree - the property the whole file exists for. assert mod._qualify_ref("origin/master") == "refs/remotes/origin/master" + + +# --- the names are git's, not git's spelling of git's --------------------------- +# +# Both halves of the measurement need the path git *means*: the report prints it, and +# `_path_reading` hands it back to git as a pathspec. Without `-z` git quotes a path +# holding a quote, a backslash, a control byte, or any non-ASCII byte, and a quoted +# path is a pathspec that matches nothing - so the inside-the-path arm answered "" for +# both sides and reported *clean* (measured `cyc20260914-082347`). +# +# Pinned in both directions (#455): the non-ASCII arm and the control-byte arm are +# shown to name the real file *and* to report the hazard, with git itself supplying +# the evidence; the plain-name arm next door keeps passing unchanged for the control. + + +def _named_same_file_repo(tmp_path: Path, name: str, tag: str) -> tuple[Path, str, str]: + """`_same_file_repo` with the shared file carrying `name`. + + Same fixture as `_same_file_repo` - both sides change the file far enough apart + that the merge is clean, so the path is in both lists while its reading is not the + landing - with the name as the only difference. The name is what the tool used to + get wrong. + """ + repo = tmp_path / tag + _init_repo(repo) + lines = [f"line {i}" for i in range(1, 31)] + _write(repo, name, "\n".join(lines) + "\n") + _commit(repo, "base") + + _git(repo, "checkout", "-q", "-b", "feature") + pr_lines = list(lines) + pr_lines[0] = "CHANGED BY THE PR" + _write(repo, name, "\n".join(pr_lines) + "\n") + head = _commit(repo, "the PR changes line 1") + + _git(repo, "checkout", "-q", "master") + master_lines = list(lines) + master_lines[-1] = "CHANGED BY MASTER LATER" + _write(repo, name, "\n".join(master_lines) + "\n") + base = _commit(repo, "master moves on inside the same file") + return repo, base, head + + +def test_a_non_ascii_path_is_named_as_it_is(mod, tmp_path, monkeypatch) -> None: + """The report must print the file's name, not git's octal spelling of it. + + Measured on this fixture with the file named 中文.txt: the default + `--name-status` spelling is `"\\344\\270\\255\\346\\226\\207.txt"`. That names + nothing a resolver can open, and as a pathspec it matches nothing - so the + inside-the-path arm compared "" with "" and this repo reported *clean* where the + same fixture named `src/app.py` as reading backwards inside. + """ + name = "中文.txt" + repo, base, head = _named_same_file_repo(tmp_path, name, "cjk") + monkeypatch.chdir(repo) + + _, landed, apparent, backwards, reversed_inside = mod.landing_reading(base, head) + + assert landed == apparent == [("M", name)] + assert backwards == [] # the name-set rule is blind here: the path *is* landed + assert reversed_inside == [("M", name)] + + # The evidence, from git rather than from the tool: the reading of that path does + # print the base's own later hunk as a deletion (so "reads backwards inside" is + # true of it), while the unseparated spelling really is quoted - which is why the + # old reading could never have matched it. + assert "CHANGED BY MASTER LATER" in _git(repo, "diff", base, head, "--", name) + quoted = _git(repo, "diff", "--name-status", "--no-renames", base, head) + assert quoted.split("\t", 1)[1].startswith('"') + assert "\\344" in quoted + + +@pytest.mark.skipif( + sys.platform == "win32", + reason="Windows cannot create a file whose name holds a control byte (CreateFile)", +) +def test_a_tab_in_a_path_is_named_as_it_is(mod, tmp_path, monkeypatch) -> None: + """The control-byte arm: a real tab arrives quoted as `"f\\ttab.txt"`. + + Without `-z` the old reading split the line on *tabs*, so this name was the one + that produced a second, non-existent path in the sibling tools' reports; here it + made the path unopenable and the inside-the-path arm blind. + """ + name = "f\ttab.txt" + repo, base, head = _named_same_file_repo(tmp_path, name, "tab") + monkeypatch.chdir(repo) + + _, landed, apparent, _, reversed_inside = mod.landing_reading(base, head) + + assert landed == apparent == [("M", name)] + assert reversed_inside == [("M", name)] + assert "CHANGED BY MASTER LATER" in _git(repo, "diff", base, head, "--", name) + + +def test_the_report_names_a_non_ascii_path_as_it_is(mod, tmp_path, monkeypatch) -> None: + """What the reviewer is handed: the real name, in the hazard line.""" + name = "中文.txt" + repo, base, head = _named_same_file_repo(tmp_path, name, "cjk-report") + monkeypatch.chdir(repo) + monkeypatch.setattr(mod, "_fetch_head", lambda number: head) + + state, report = mod.check_pr(1, base) + + assert state == "backwards" + assert "reads backwards inside: 1 of the 1 path(s)" in report + assert f"M\t{name}" in report + # The quoted spelling is not a name a resolver can use, so it must not be printed. + assert "\\344" not in report + + +def test_a_payload_that_is_not_status_path_pairs_is_a_measurement_error(mod, monkeypatch) -> None: + """A shape this tool does not know is unmeasurable, never a shorter list. + + `-z` writes `status\\0path\\0`; a reader that skipped an unpaired trailing field + would answer about a change it never read, so the pairing is asserted and the + failure is loud (the fake stands in for a payload git cannot be made to produce). + """ + fake = subprocess.CompletedProcess( + args=["git", "diff"], returncode=0, stdout="M\0only-a-path\0M\0", stderr="" + ) + monkeypatch.setattr(mod, "_run", lambda *args, **kwargs: fake) + + with pytest.raises(mod.MeasurementError) as excinfo: + mod._changed_paths("a" * 40, "b" * 40) + + assert "not status/path pairs" in str(excinfo.value)