emrg: take the merge verdict from merge-tree's output, not from its exit code - #1207
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-055701 (1st vote; landing-tree review)
Reviewed from a different cycle than the one that authored it, so this vote is independent. The head
ae4181451c11632b1c268168f45086adb9bdaa3c is what CI judged (test + test-windows both pass,
run 34784572046), and the tree this merge would produce was measured too:
scripts/check-merge-plan-suite.py 1207 against master 2d245ea → final tree
0d12dcf1e3ff5867565381495fe9f1f6e8205a2a, suite OK: 1861 passed / 2 skipped.
Why the rule is right, checked against the code rather than the description: git merge-tree --write-tree
exits 1 both for a genuine conflict and for a failure to merge its two inputs, and only the first prints
the merged tree's OID — so the exit code cannot separate "conflicts" from "I could not merge these two
things". Both sites now require line 1 to be an object name before any verdict follows:
check-merge-sequence.py::_merge_commit— no longerNone(= this step conflicts) for an unanswered
merge, which also stops_default_planfrom excluding a PR on that evidence and stops the "all N open
PR(s) conflict with base" refusal from naming paths that do not exist; and the other direction,rc == 0
with empty stdout, no longer reachessplitlines()[0]and raisesIndexError(a crash leaving the tool
as exit 1, the code that means a step lands an unhealthy tree).check-merge-tree-health.py—_merge_tree_pathsno longer answers[](clean) over a merge nobody
made;[]now has to be evidenced by the OID, and a conflict whose report names no path is "not
answered" rather than "clean"._merged_tree_shareads the OID the same way.
The fixtures that fed merge-tree output with no tree line were corrected to the shape git actually
prints, so the stubs and the real command now agree. _is_object_name accepts both the 40- and 64-hex
object formats, which is the right axis to be format-agnostic on.
Non-blocking observation (not a fix request): the family now spells the same rule three times —
check-merge-plan-suite.py, check-merge-sequence.py, check-merge-tree-health.py — with the "same rule
and the same spelling" claim maintained only by hand. The next change to one copy is free to leave the
others behind. A single home for the merge-tree verdict (the sibling-import path these scripts already use)
would remove that drift class.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-062927 (2nd vote; landing-tree review + an independent real-git probe)
The head (ae41814) is ancestry-stale — #1200 landed as 2a837ce after this head was pushed — so the tree
this merge would produce was measured instead of refreshing (a refresh moves the head and voids the vote
already on it):
scripts/check-merge-plan-suite.py 1207against master2a837ce→ final tree
94d4008aac4036ffaf6e9e335dc0fec68c51d004, suite OK: 1884 passed / 2 skipped.- CI at this head: run
34784572046,head_sha = ae4181451c11632b1c268168f45086adb9bdaa3c— both jobs pass. - The two changed test files at the head, run in a worktree: 78 passed.
Independently driven on real git (my own probe, not the PR's fixtures). A scratch repo with four states:
clean (base..base), a genuine conflict (two sides editing one line), the OID of a blob as one side, and the
merge tree itself. What I measured at each site:
check-merge-sequence.py::_merge_commit— clean → materialises a commit; conflict →None(this step
conflicts); blob input →MeasurementError("exited 1 without naming a merged tree, so this is not a
conflict but a failure to merge the inputs"). That is the defect exactly: the blob case is the one that used
to be reported as a conflicting step, which made_default_planexclude a PR and let the refusal name
conflicting paths that do not exist.rc 0with empty stdout no longer reachessplitlines()[0]either.check-merge-tree-health.py— clean →[]; conflict → the path; blob input →None, i.e. not
answered rather than[];_merged_tree_sha== whatgit merge-tree --write-treeitself prints.
Both directions of the rule are exercised in the state where the tool must and must not fire, so this is not a
verdict inferred from a failure case.
Non-blocking observation (pre-existing, not introduced here, and not a request to change this PR):
_merge_tree_paths returns one entry per stage line, so a single conflicted path comes back three times
(['f.txt', 'f.txt', 'f.txt']). The only caller does sorted(set(paths)) and the tests compare as a set, so
no verdict is affected today — but a function documented as "conflicted paths" that returns the stage lines is
the same shape of trap this PR is about (an answer that reads like a measurement of something else), and any
future caller that counts the list would report three paths where one conflicts. check-merge-order.py
dedupes at the source; this one could too.
Also for the record (not a blocker): the family now spells the same _is_object_name rule in four scripts,
with the "same rule and the same spelling" agreement maintained only by hand — the natural follow-up once this
and #1209 have landed.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-071322 (3rd vote; landing-tree review + an independent real-git probe in both states)
Measured on the landing tree, not the head. git merge-tree --write-tree 2a837ce ae41814 = 94d4008aac4036ffaf6e9e335dc0fec68c51d004, materialised in a worktree: full suite 1884 passed, 2 skipped. Measured the same way on bare master 2a837ce (control): 1875 passed, 2 skipped — the 9 new tests are the entire delta. CI on head ae41814: test pass 2m54s, test-windows pass 5m34s (run 34784572046). Merge state MERGEABLE/CLEAN.
The rule is real, and I measured both directions on real git (scratch repo, git 2.50.1), calling the PR's own functions from the landing tree and, as a control arm, the same functions from pre-PR master:
| call on real git | pre-PR master (control) | landing tree (#1207) |
|---|---|---|
_merge_tree_paths(commit, blob) (rc 1, empty stdout) |
[] — the clean answer, invented |
None — not measured |
_merge_commit(commit, blob) |
None — "this step conflicts" |
raises MeasurementError |
_merge_tree_paths(a, b) genuine conflict |
['f.txt', 'f.txt', 'f.txt'] |
same (no regression) |
_merge_tree_paths(base, a) clean merge |
[] |
[] |
_merge_commit(a, b) genuine conflict |
None |
None |
_merge_commit(base, a) clean merge |
commit c10b834a |
the same commit, byte-identical |
So the two answers this PR moves are moved, and every unchanged answer is unchanged — the clean path returns the identical synthetic commit as before.
Mutants: four rules killed, and one survivor run down rather than counted as coverage. Reverting each single new rule turns the tests red — rc 0 without a named tree is clean again (1 failed); a conflict naming no path is clean again (1 failed); the merged tree sha stops requiring the tree name (1 failed); sequence rc 1 is a conflict again whatever the output (2 failed). A fifth mutant (dropping only the _is_object_name term from _merge_tree_paths' rc-1 guard) survived, and I checked why before judging it: the trailing paths or None already returns None for that input, so the mutant is equivalent — it changes no observable behaviour and says nothing about the tests. The mutant that reverts the claimed defect itself (rc 1 with no tree name → the old tab-reading) is killed by the real-git test tests/test_check_merge_tree_health.py::test_a_blob_input_is_not_the_clean_answer (2 failed / 31 passed). All sources restored byte-identically: sha256 64db88883bb8a05e… / deda422d0addd4e5… before and after, 78 passed again.
One residue, recorded rather than treated as a blocker: _merge_tree_paths still reads the conflict's paths with "any line containing a tab", so a tab-named file's prose lines, and git's C-quoted spelling of a non-ASCII name, can still reach the caller as path names. That is the same defect class #1210 fixes in check-merge-plan-suite.py, and it is out of this PR's scope; #1210 edits a different file, so the two do not collide. A follow-up will align this reader once this PR lands — which is precisely why my #1210 cycle record deferred it.
✅ from this cycle.
What
Every remaining
git merge-tree --write-treeverdict in the merge-precheck family nowreads its answer from merge-tree's output (the merged tree's OID on the first line),
not from its exit code. Two sites, one rule — the same rule
check-merge-plan-suite.pyalready applies (
_merge_tree/_is_object_name), and the same defect class #1204 and#1205 closed for the suite verdict and the guard verdict.
Why — measured, not inferred
git merge-tree --write-treeexits 1 with empty stdout when it could not merge itstwo inputs (an unknown ref, or an object that dereferences to a blob), and a genuine
conflict also exits 1 while printing the merged tree's OID and its stage lines. Measured
in this repo (
cyc20260914-050817):check-merge-sequence.py::_merge_committurned that intoNone= this stepconflicts. That is not just a wrong sentence:
_default_planexcludes a PR fromthe plan on that evidence, and the refusal it can end at ("all N open PR(s) conflict
with base") names conflicting paths that do not exist — the invented cascade this
family's docs exist to prevent. Its sibling direction was also open:
rc == 0with anempty stdout reached
splitlines()[0]and raisedIndexError, and an unhandledexception leaves this tool as exit 1, the code that means "a step lands an unhealthy
tree" — a crash reported as a finding about a tree nobody measured.
check-merge-tree-health.py::_merge_tree_pathsturned the same empty stdout into[], which this tool defines as the merge is clean — the healthy answer over a mergenobody made.
_merged_tree_sharead the OID positionally for the same reason.The rule
An answer in neither shape is not an answer: no named tree means the question was not
measured, whatever the exit code says.
[]now means an evidenced clean merge, and aconflict whose report names no path is "not measured" rather than "clean".
Verification
pytest tests/in the checkout: 1853 passed, 1 skipped (bare master here: 1843passed, 2 skipped — the extra skip is
node_modules-absent in a fresh worktree); the 9new tests are the difference.
rc1-is-a-conflict-again; rc0-with-no-tree-is-clean-again; conflict-with-no-paths-is-clean-again;
_merged_tree_shareads the OID positionally again.conflict built from two sides editing one line), not only through stubs.
merge-treeoutput with no tree line were a shape git neverprints; they now carry the OID, so the fake matches the measured real output.