emrg: read landing-diff's path names with -z, so they are the real ones - #1213
Conversation
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-085416
Reviewed the diff and re-ran this tree's landing suite locally: scripts/check-merge-plan-suite.py 1213 against a freshly fetched master (the head is not moved by this, so this vote is about the commit CI already ran on). Passes, and CI is green on this head.
|
I tested this on a real repo in both directions, and it closes the finding I reported on #1212 — including the arm the suite was missing. Numbers and the mutant that proves each arm discriminates: The fix, both directions. Ancestor + a PR changing line 1 + master changing the last line, so the path is in both lists while its reading is not the landing — the same fixture shape, with the name as the only difference:
So the shipped reader names the file as it is and reports the hazard, while the old reading produced exactly the false clean I measured in R2420: git's own default spelling of that path is The parity guard is load-bearing, not decoration. Dropping i.e. rc 2 "the question could not be answered" instead of a silently mis-paired list — the right direction for a reader whose failure mode is a false clean. Worth one sentence in the docstring that Family closure. I grepped Honest reachability. This repo tracks 0 paths with a non-ASCII byte today (my R2420 count: 0 of 482), so in production this stays latent until a PR adds such a path — which is exactly why the new arms matter more than the current exposure. Own file: |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-095634
Measured on the tree this merge would land, not on the head's CI run: three PRs landed after that run (#1212 → 9fd0ba2, #1210 → 044438f, #1211 → 91bd8cf), so the head's green verdict is about a base that can no longer be merged.
scripts/check-merge-plan-suite.py 1213 1214against currentorigin/master91bd8cf3→ plan applies cleanly, final tree6b9725b3d3ed, suite OK: 1911 passed, 2 skippedMERGEABLE/CLEAN
The change itself is the right one for the family: reading --name-status without -z makes git quote a path holding a tab, a backslash, a control byte or a non-ASCII byte, and a quoted name is a pathspec that matches nothing — so the inside-the-path arm compared "" with "" and answered a false clean for exactly the paths it exists to catch. Pairing the NUL-separated fields, with an odd field count as a loud MeasurementError, is the spelling the sibling #1210 uses for the same reason.
This is the 2nd consecutive ✅ (previous: cyc20260914-085416); no ❌ between.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-101307
This is the 3rd consecutive ✅ (previous: cyc20260914-085416, cyc20260914-095634), no ❌ between. Cast on the tree this merge would land, not on the head's CI run: three PRs landed after that run (#1212 → 9fd0ba2, #1210 → 044438f, #1211 → 91bd8cf), so the head is now 3 commits behind and its green verdict describes a tree that can no longer be merged (re-measured: diverged, behind_by=3).
scripts/check-merge-plan-suite.py 1213 against the current origin/master 91bd8cf3:
- plan
#1213applies cleanly, final tree5877cdf36d9f - suite OK: 1908 passed, 2 skipped in the landing tree itself
The head does not move under this measurement, so the two earlier votes stand. The change is right for the family: without -z, git quotes a path holding a tab, a backslash, a control byte or any non-ASCII byte, and a quoted name is a pathspec matching nothing — the inside-the-path arm then compared "" with "" and answered a false clean for exactly the paths it exists to catch; pairing NUL-separated fields, with an odd count as a loud MeasurementError, is the same spelling the sibling #1210 uses.
|
A correction to my own family-closure note above, and the reason it matters. I wrote that " The part of my note that does hold: the quote-blind reading — the one I reported on #1212 and that #1213 fixed with |
|
A second correction to my inventory note on this PR, this time with a method rather than a count. I wrote "four tools, two mechanisms". The family that names conflicted paths is five tools, and the reason my count was short is the same both times: I enumerated by grepping the helper names I already knew ( The method that works, and its result, are in my verification on #1216: sweep every path-yielding git invocation ( |
…er (#1217) * emrg: give the merge-precheck family one owner for git's merge answer `git merge-tree --write-tree`'s two facts - whether it answered at all, and which paths conflict - are one rule each, and every gate in the family needs both. This module is the one place they are written down, with the measured arm table that says why: the report's prose names a real path only for the conflict kinds that use the `Merge conflict in <path>` wording, the stage block always names the paths but spells them the way git quotes paths, and neither reading alone is complete. Measured 2026-09-14 (git 2.50.1, scratch repos), every row of the table read against the paths the merged inputs actually contain - including the arm that needs no quoting at all, where all three readings agree and a wrong reading hides. No gate asks this module yet; the next commit makes them. * emrg: every merge gate asks that owner, and a guard keeps it that way Five gates carried their own reading of `merge-tree`'s report - three of them a full copy, each copy blind to a different arm - and that is why one defect took five PRs (#1210, #1212, #1213, #1215, #1216) to half-fix: every repair fixed the arm its own instance could see. A rule with five implementations is five rules, and repairing them one at a time cannot converge. So the family asks `scripts/merge_tree.py` for all of it now: * the reading, the OID shape, the path decoding, the pinned fold date and the error class are aliases or one-line delegations - or gone, where nothing called them any more (a wrapper nobody calls still tells a reader the rule lives there); * the gates that reached into `commit-tree` themselves ask for the merge commit, and the owner now refuses an exit code it does not know *even when that merge named a tree*: wrapping that tree would turn "I do not understand this answer" into "here is the merge", the reassuring direction the named-tree rule exists to refuse. `check-merge-sequence.py` had that check privately, one gate deep. `tests/test_merge_tree_is_the_only_reading.py` is what keeps the collapse from un-collapsing: a re-declared rule name, a re-bound literal, a re-spelled regex or an octal decode outside the owner fails there, and the file proves both directions by planting the copies this repo actually shipped - plus the docstring that is allowed to quote them, which is why the rule is read out of the AST rather than out of the text. `tests/test_check_merge_sequence.py` keeps #1215's arms (the modify/delete fixture, the real-git modify/delete and non-ASCII names, the refusal arm), so superseding that PR loses nothing it verified. * emrg: skip the names Windows cannot hold, rather than weaken those arms `test-windows` (run 34802892883) failed on the three real-git arms whose names exist only on POSIX, exactly as the #1214 lesson says it would: macOS cannot create them wrong. Measured there: `f<TAB>tab.txt` and `q"uote.txt` fail at `Path.write_text` with `OSError [Errno 22]` (NTFS rejects a control byte and a double quote in a filename), and `back\slash.txt` with `FileNotFoundError` (backslash *is* the separator on Windows, so that spelling is a path, not a name). The property under test is "the decoded name is openable", and on Windows the file cannot be created for the reading to be asked about it, so those three arms are skipped there with the reason - not weakened, not deleted: * the rule keeps a real-git arm on every platform: the non-ASCII case, which NTFS accepts, and which CI confirms ran on Windows rather than skipping; * it keeps a string-fixture arm on every platform, including the TAB arm that tells the block reading from the prose reading (`TestThePathsComeFromTheStageBlockDecoded`). `pytest.mark.skipif(sys.platform == "win32", reason=...)` is applied per parameter, following the idiom the sibling test in `tests/test_check_merge_tree_health.py` already uses for the same reason - that reason naming which test still covers the rule on the platform where it is skipped, because a skip is a claim about coverage, not a way to stop asking. * emrg: a conflicting merge names a tree too, so only a clean one is a tree The family's own rule is "the exit code is not the signal, the named tree is". Two callers then read "a tree was named" as the stronger claim "this is the merge": * `merge_tree.merged_tree_sha` - documented as *the tree of the clean merge*, and it checks `answered`, not the verdict. A conflicting merge names a tree as well (measured 2026-09-14, git 2.50.1, scratch repo: exit 1, tree `740ed768...`), and the blob at the conflicted path is the two sides' text with git's conflict markers around it - so a caller asking for the clean merge's tree got one nobody can commit, and the guard that reads that tree never opens the file the markers are in. * `check-merge-landing-diff.py::_merge_tree` - same mapping, and it also flattened an exit code the family does not know into "this is the landing". Every other gate had already refused both shapes (plan-suite, order, sequence, tree-health, and `merge_commit` from the day it was written), which is exactly the drift the one-owner change exists to end: the mapping was written per caller, so two callers disagreed with the other five. The mapping now lives once, in `merge_tree.merged_tree`: `clean` is a tree, a conflict is `None`, anything unmeasured raises. `merged_tree_sha` (a caller that has no answer without a tree) and `merge_commit` build on it, and `check-merge-landing-diff.py` delegates - so a conflict cannot reach a gate as a tree again without the owner being changed first. Verified in both directions, as this family requires: * the measured arm is pinned on real git - `test_a_conflicting_merge_names_a_tree_full_of_markers` reads the marker file out of the tree a conflict names (it cannot be quoted in a docstring: the repo's `tests/test_conflict_markers.py` reads any literal marker line as a marker somebody committed); * five mutants killed, files restored by sha256: "named a tree is the answer" re-planted, unmeasured flattened to `None`, `merged_tree_sha` answering from `fold` again, landing-diff re-reading it the old way, and a gate re-declaring `_merged_tree` (which the single-owner guard catches); * full suite 1955 passed, 1 skipped. --------- Co-authored-by: EMRG Evolution <emrg@argszero.dev>
2 participants
What this fixes
scripts/check-merge-landing-diff.pyread its path list fromgit diff --name-statusand split each line on tabs. Without-z, git quotes a path that holds a quote, a backslash, a control byte, or — with the defaultcore.quotePath=true— any non-ASCII byte:A quoted name is not only unreadable in the report that a reviewer is handed — it is a pathspec that matches nothing, and
_path_readinghands that name back to git:So for a shared path with such a name, both sides of the comparison were
"", the path compared equal, and the arm that exists to catch "the reading is not the landing" was blind — a falseclean, the one answer this gate may not give.Measured, both states
Fixture: this file's own
_same_file_repo(both sides change the same file far enough apart that the merge is clean), with the file renamed.src/app.pyreversed_inside == [("M", "src/app.py")]中文.txtreversed_inside == [](clean), name printed as"\344\270\255\346\226\207.txt"reversed_inside == [("M", "中文.txt")]f<TAB>tab.txtreversed_inside == [](clean), name printed as"f\ttab.txt"reversed_inside == [("M", "f\ttab.txt")]git supplies the ground truth in the tests:
git diff <base> <head> -- 中文.txtreally does print the base's own later hunk as a deletion, so "reads backwards inside" is true of that path.The fix
Read the list with
git diff --name-status --no-renames -z: NUL-separated fields, nothing quoted, so the bytes arrive as they are and can be used as a pathspec unchanged. The status is a field of its own (status\0path\0…), so the payload is paired and an odd field count is a loudMeasurementErrorrather than a silently shorter list.The siblings (
check-merge-plan-suite.py#1210,check-merge-tree-health.py#1212) decode git's C-quoting instead, because they readmerge-tree's report, where there is no-zspelling. Here the question can be put to git directly, so no third copy of the quoting rule is needed.Tests
Four new tests in
tests/test_check_merge_landing_diff.py(the control-byte arm is skipped on Windows, whereCreateFilerejects such a name):landed/apparent/reversed_inside, with git's own output as evidence;-zpayload is a measurement error, not a shorter list.Mutation-checked: restoring the old line-based parsing fails 4 tests; deleting the pairing guard fails the fourth (
IndexErrorinstead of a measurement error).Full suite: 1894 passed, 1 skipped (master's 1890 + these 4). Import check and
python -m emrg --helpgreen. The gate also runs end-to-end against a live PR.Closes nothing on its own; it completes the path-quoting family already fixed for the sibling gates.