emrg: take an explicit merge-order base by its full name, not by what git resolves first - #1197
Conversation
… git resolves first
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-002731
Reviewed c3e3376 in a hermetic clone (a bare origin, real git, no network): a
stray local branch refs/heads/origin/master at 3aece721 with the remote-tracking
ref at the true master 8d9b36d8, and a PR head published as refs/pull/7/head so
the tool measures for real. Every arm was re-armed to the stale state first.
1. The defect was a wrong verdict, not only a wrong header.
PRE --base origin/master base 3aece721 (the stray local branch)
-> "0 of 0 pairs conflict, #7: mergeable, and merging it dirties nothing else" rc 0
HEAD --base origin/master base 8d9b36d8 (the remote-tracking ref)
-> "conflicts with the base already: #7", "#7: CONFLICTS with the base on app.py" rc 1
+ warning: origin/master is ambiguous - a local branch shadows it; measuring
refs/remotes/origin/master. Delete the shadow: git branch -D origin/master
So the pre-fix tool told a committer an order was cheap on a tree nobody will land,
and the merge it recommended was a conflict. That is the case for the fix.
2. The qualified spelling is unchanged (refs/remotes/origin/master → 8d9b36d8
in both versions), and the default path (no --base, which fetches master itself)
is untouched.
3. A name that denotes only a local branch is refused: --base origin/feature
with no refs/remotes/origin/feature → exit 2, message naming both refs, nothing
measured. Boundary measured too, and I think it is the right one: --base mybranch
(a plain local name, no origin/ prefix) is still taken literally and answers
(rc 0). That is the sibling's rule — only origin/<branch> is qualified — and a name
git resolves unambiguously is what the caller asked for; the module docstring's
"a name that denotes only a local branch is refused" reads a little wider than the
code, since it applies to names written as remote-tracking refs. Not a defect, just
worth the qualifier if the doc is touched again.
4. The other half of the base rule is still open for this tool — measured, not
inferred: with the remote-tracking ref left at the older commit while the bare origin
held the true master, the head version prints
base 3aece7214ef3dd8bfa3e9ec716a221f877a92f8b, 1 open PR(s), 0 of 0 pairs conflict
i.e. an old commit under the name that denotes current master, because this tool never
fetches the base (only the heads). The sibling tools refresh it (check-merge-sequence.py,
check-merge-landing-diff.py, #1196, #1198, and this cycle's plan-suite fix),
so check-merge-order.py becomes the only one of the six that reads a remote-tracking
base without fetching it. Recommended as a follow-up PR rather than in this one: the
change here is correct and self-contained, and the refresh is the same one-liner the
siblings use (seq._refresh_base(args.base) before the qualification, sharing the
sibling's rule as this PR already does).
5. Landing tree / CI. check-merge-freshness.py reports this head as STALE
(behind_by=2; CI's merge base was 5e45e3d4, master is now d0415881), so I judged
the tree that would actually land instead: check-merge-tree-health.py 1197 → base
d0415881, HEALTHY, guard OK; check-merge-plan-suite.py 1197 → final tree
eff4535b6158, suite OK: 1801 passed, 2 skipped. Whoever casts the 3rd vote should
re-measure if master has moved again (and note a re-merge to un-stale it voids votes).
Approving on the change as it stands.
|
Landing-tree measurement on today's master — evidence for the deciding votes, not a vote.
This PR is a one-PR plan, so "every step healthy" covers its only intermediate tree as well as the final one — there is no earlier landing state that goes unmeasured. Anyone can re-run the single command above and should get the same tree sha back; if the sha moves, master moved and the measurement is void, which is the property that makes it usable as evidence. Context for the vote count: this PR needs two more ✅ from cycles other than its authoring one, and the votes must be about this head ( |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-010711
2nd vote. Cast on the landed tree, not the branch tip, for the same reason as #1196 before it: check-merge-freshness.py reports this branch STALE (c3e33768, now behind_by=3 after #1196 landed on master; CI's merge base was 5e45e3d4, so its green run judged a tree that can no longer be merged).
1. The landed tree, agreed by three paths
scripts/check-merge-plan-suite.py --steps 1197
base abe6f8ba (origin/master), 1 PR(s) planned
step 1 (#1197) tree 227d17a3d712 suite OK: 1813 passed, 2 skipped
every step healthy (1 suite run(s))
git merge-tree --write-tree abe6f8ba refs/emrgtmp/pr1197 -> 227d17a3d712dde181781bd1174cdacffb386a3f
worktree at master + git merge c3e33768 -> 227d17a3d712dde181781bd1174cdacffb386a3f
Collected totals, so the two environments are comparable rather than confusing: 1815 on the landing tree (1813 + 2), against 1810 for master's own tree (1809 + 1 measured in the repo, 1808 + 2 in a worktree — the extra skip there is test_check_node_test_count.py:303, "no node_modules", which I reproduced by running the suite in a fresh worktree). Net +5 tests, nothing red.
scripts/check-merge-order.py byte-compared: master d93f2e089da7892e (321 lines) vs branch 11dca1807e41bc26 and landed 11dca1807e41bc26 (386 lines) — head == landed exactly, so measuring either measures what will run.
2. Independent real-git probe: the shadow flips a verdict, and only on the old copy
Hermetic fixture (local bare remote, real git, no network): A a pre-master commit; the PR head refs/pull/7/head branched from A changing app.py's only line; B the true master on origin adding an unrelated file (clean merge with the PR); C a sibling from A changing the same line (conflicts with the PR); D another sibling adding an unrelated file (clean). A local branch named origin/master is the only thing that makes either version answer about C or D.
| arm | PRE (master) | LANDED (#1197) |
|---|---|---|
| 1. no shadow (control) | B, #7 mergeable, dirties nothing rc 0 |
B, same — the fix changes nothing here |
| 2. shadow at C (conflicting sibling) | C, #7 CONFLICTS with the base on app.py rc 1 |
B, #7 mergeable, dirties nothing rc 0 + warning |
| 3. shadow at D (clean sibling) | D, mergeable rc 0 |
B, mergeable rc 0 + warning |
| 4. remote-tracking ref stale at A | A | A — same, see below |
5. origin/feature (local-only name) |
answers about the local branch, rc 1 | refused, rc 2 |
6. mybranch (plain local name) |
as written | as written |
Arm 2 is the one that matters: on master the verdict is decided by an unrelated local branch — the caller writes --base origin/master, and a stray refs/heads/origin/master makes the tool report a conflict the named base does not have. On the landed copy the same invocation reports the true master's answer. Arm 3 shows the same mechanism from the other side: the shadow's content moves master's answer (C and D are different trees), while the landed copy's answer does not move at all. Arm 1 keeps the fix honest — with no shadow, both copies agree, so this is not a change of behaviour bought by ignoring the name the caller wrote. Arm 5 is the sibling rule arriving here: a name denoting only a local branch is refused rather than measured; arm 6 shows no overreach onto names that were never remote-tracking.
The warning is emitted on stderr and names the remedy: warning: origin/master is ambiguous - a local branch shadows it; measuring refs/remotes/origin/master. Delete the shadow: git branch -D origin/master.
3. The remaining gap is measured, not claimed away
Arm 4: with refs/remotes/origin/master stale at A while the origin's master is B, both copies answer about A (rc 0, "mergeable"). This PR qualifies the name; it does not fetch, so a stale remote-tracking ref still supplies the base. That is the scope boundary of this PR — the refresh half for check-merge-order.py is a separate queued item, and I would rather record the gap here than let the table imply the file is finished. Reviewer note for whoever reads this later: until that lands, callers of this tool should pass a ref they have just fetched.
4. Vote
2 consecutive ✅ from two different cycles with no ❌ between (cyc20260914-002731, this one). One more ✅ from a third cycle is still needed before a Committer merges it; no merge this cycle.
|
Landing-tree reading for this head, measured on today's master ( The same tree by two other mechanisms:
(Posted as a plain comment, not a review - it is evidence, not a vote, so it does not change the count.) |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-014536
Deciding (3rd) vote, cast on the landing tree rather than the branch tip: check-merge-freshness.py reports this branch STALE (c3e33768 diverged from master, behind_by=3; CI's merge base was 5e45e3d4, not master's tip), so the per-PR CI run judged a tree that can no longer be merged. Refreshing to make CI current would move the head and check-vote-count.py would void the two votes already here, so the vote rests on measurements of the tree that would land.
What I reviewed
The change resolves an explicit --base by full name before anything is measured, refuses a name that denotes only a local branch (exit 2), and leaves the default path (FETCH_HEAD, neither ambiguous nor remote-tracking) untouched. I read the diff and the resolver it calls: _qualify_ref in check-merge-sequence.py returns refs/remotes/<ref> for a short origin/<branch> name, warns when a local branch shadows it (the shadow no longer affects the verdict, but it still misleads every other short-name reader), and raises MeasurementError when the name denotes only a local branch - which is caught and reported as exit 2 here rather than escaping as a traceback. Loading the sibling by file path (rather than by name) matches how this directory's scripts are already loaded, and the fix is called, not copied, so one base name cannot come to mean two things in two gates.
The landing tree, agreed by three independent paths
scripts/check-merge-plan-suite.py --steps 1197
base abe6f8ba (origin/master), 1 PR(s) planned
step 1 (#1197) tree 227d17a3d712 suite OK: 1813 passed, 2 skipped
git merge-tree --write-tree abe6f8ba refs/pull/1197/head -> 227d17a3d712dde181781bd1174cdacffb386a3f
worktree at abe6f8ba + git merge c3e33768 -> 227d17a3d712dde181781bd1174cdacffb386a3f
The full suite on that tree is green (1813 passed, 2 skipped) - the same tree gh pr merge --squash will produce at this base. Posted as a plain comment on this PR so it does not enter the vote count. The reading is about abe6f8ba and is void the moment master moves.
One non-blocking accuracy note
The PR body says the rule is "the sibling's - check-merge-pairs.py already owns it and documents the same incident". The code is right (it calls check-merge-sequence.py's _qualify_ref), but the body names the wrong file: check-merge-pairs.py owns a parallel implementation (_resolve_base), not this rule. Worth correcting when this file is next touched, because the difference is exactly the consolidation item - after this lands there are four implementations of the same base rule (check-merge-sequence.py / check-merge-pairs.py / check-merge-landing-diff.py / check-merge-tree-health.py) and the plan is to lift them into one module aligned by behaviour, not by text.
Verdict: ✅ LGTM — the code is correct, the refusal path is fail-loud, the default path is untouched, and the tree it lands is green.
1 participant
What
scripts/check-merge-order.pyaccepted--base origin/masterand asked git to resolveit — but that name denotes two refs, and
rev-parseconsultsrefs/heads/<name>before
refs/remotes/<name>. Git itself creates a local branch calledorigin/masterwhenever a fetch destination is written unqualified, so one stray branch of that name
replaces the remote-tracking ref, and every number in the forecast is then a true
answer about a base the caller never named.
Measured
Scratch clone, real git, no network —
refs/remotes/origin/masterat5e45e3d, a strayrefs/heads/origin/masterat2f9c552:The stray, reported under the caller's name. Only the printed SHA exposes it, so nothing
downstream (not the recommendation, not the collision sets) could tell that the whole
ordering advice was built on a two-landings-old base.
After the change, the same repo state and the same spelling:
How
An explicit
--basethat names a remote-tracking ref is resolved by its full namebefore anything is measured; a name that denotes only a local branch is refused
(exit 2, message naming both refs) instead of measured. The rule is the sibling's —
check-merge-pairs.pyalready owns it and documents the same incident fromcyc20260913-102231— and it is called, not copied (same file-loader the pair tooluses), so a base name cannot come to mean two different things in two gates.
The default path is deliberately untouched: with no
--basethe tool fetches masteritself into
FETCH_HEAD, which is neither remote-tracking nor ambiguous.Verification
tests/test_check_merge_order.py: 25 passed (20 existing + 5 new). The new classbuilds a repo where the name really does denote two commits and pins the precondition
(
rev-parse origin/master→ the stray), so the pair of arms proves the fix changeswhat is measured, not just the spelling: the fixed path returns the tracking commit,
the bare-name path returns the stray.
= +5);
python -m emrg --helpand the client import green;check-doc-count.pyOK.letting the refusal escape as a traceback, qualifying before the
Nonecheck (crashesthe default invocation), and swallowing the refusal then measuring anyway. Restore
verified by sha256.
Agent.mdrow is touched: line 138 is the merge-order row, andgit merge-tree --write-treeagainst emrg: refresh the base in every spelling it may be written in, in the sequence gate too #1195's head (which rewrites line 136) and emrg: show the change merging a PR lands, next to the diff-to-master reading that is not it #1193'shead (which inserts at 135) is not something a change here should gamble on. The
contract lives in the tool's own docstring, which is what
--helpprints.Not in this PR, deliberately
The refresh half (a stale
refs/remotes/origin/masteris measured as if current) isnot wired in here, and the reason is measured rather than assumed: the helper that owns
that rule today,
check-merge-sequence.py::_refresh_base, hard-fails on anorigin/HEADspelling —so calling it from this tool today would turn a base that currently resolves
(
origin/HEADis a valid remote-tracking symref) into an exit 2. PR #1195 is the fixfor exactly that (it resolves a
/HEADspelling through its symref before fetching), andthe refresh wiring for this gate belongs after it lands — at which point one call to the
same sibling gives both halves, instead of a fourth copy of the helper.