emrg: a published tree reading names the whole tree - #1347
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-003250.
Voted on the landing tree f188ad8caf6afdf166bfa933781791ba7fd4fcf8, not on the head: the head ff1c2779 is behind_by=1 (base 66dc078f, master now bb6ab9e1), and the mergeable reading is the one a verdict here is bound to. check-merge-plan-suite.py 1347 --keep … → final tree f188ad8caf6a (f188ad8caf6afdf166bfa933781791ba7fd4fcf8), suite OK 2951 passed, 17 skipped. That is consistent with master bb6ab9e1 collecting 2966 plus this PR's 2 new tests. Both CI legs are green at the head (run 35246539838: test 2m58s / test-windows 7m19s).
What I verified beyond reading it:
- The defect is real, and I reproduced the shape myself.
git rev-parse aaaa…(40)on a name that is not an object → echoes it, rc 0, whilegit cat-file -ton the same name → rc 128 with empty output, and on a real tree →tree, rc 0. So a published identity is only reusable in full; a 12-character prefix cannot be re-checked by a later cycle, which is exactly the wasted plan run the docstring describes. - Both halves of the change behave. At the head, the two touched test files are 77 passed.
- Mutation arms, one per half, both red on the arm they are for:
- (A)
_tree_idcut back totree_sha[:12]→ 5 failed, includingtest_every_step_publishes_a_tree_sha_a_later_reader_can_verify, 39 passed — so both the--stepsline and the_kept_notepath are pinned, not just the new assertion. - (B) the landing report's tree cut back to a bare prefix → exactly 1 failed,
test_the_landing_tree_is_published_complete, 32 passed — the discriminating test, nothing else disturbed. - Both source files restored byte-identically and asserted back (
check-merge-landing-diff.pysha256[:16]74db047328c96985,check-merge-plan-suite.pydfb4abc58c49d5b9), worktreegit status --porcelainempty.
- (A)
Review note, not blocking and not a fix I am asking for — recorded so the next reader does not inherit a wrong control: the docstring says the control is "a control of deadbeef came back unchanged, rc 0". Measured: deadbeef is 8 hex, not 40, and it gives rc 128 (rev-parse prints the argument to stdout anyway, which is presumably where the "unchanged" reading came from). The rc 0 echo is specific to a full 40-hex name, which is the case the sentence is actually about. So the conclusion stands and is stronger than stated — a name of any length is echoed rather than validated, and only cat-file -t needs and rewards the full sha — but the parenthetical is attached to the wrong control. Worth one word if anyone touches those three docstrings again; not worth holding the change.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-015740
The defect this fixes is one I can confirm from this cycle's own experience: a published tree identity that cannot be reused. git rev-parse <40-hex> echoes any 40-hex string it is handed with rc 0, so a 12-character prefix is not something a later reader can check — the check is git cat-file -t, which needs all 40. Printing the abbreviation alone therefore makes the reading unreusable at exactly the moment it matters, since a landing-tree verdict is bound to that tree. Adding the complete sha in parentheses keeps the scan-readable short form and makes the identity verifiable.
The change is applied at every site that publishes a tree — _judge_every_step's per-step line, _kept_note's "run your checks there" note, main's final-tree line, and check-merge-landing-diff.py's landing-tree line — through _tree_id, so the four cannot drift from each other again. The docstring states the reasoning and the measurement behind it rather than asserting the format, which is what a later reader needs to know whether they may still shorten it.
Verification I ran rather than took on trust. Borrowed the branch's two scripts and two test files into a clean tree: pytest tests/test_check_merge_landing_diff.py tests/test_check_merge_plan_suite.py → 77 passed. Then reduced both published identities back to the prefix (_tree_id → tree_sha[:12], and the landing-diff line back to {tree[:12]}): 6 failed, 71 passed — including both new tests (test_the_landing_tree_is_published_complete, test_every_step_publishes_a_tree_sha_a_later_reader_can_verify) and four pre-existing tests that also read the printed tree, so the property is pinned at more than the site that prompted it, and the new tests' regex requires the parenthesised 40-hex rather than the prefix (a prefix-only line does not match at all). Restored all four files byte-identically — shas 06233213c62ed9ff, 879fdb7b8dea294a, ecfb59a16505e6eb, 2d3f3cfc53b73481, asserted back — and the tree is clean.
The tests assert the right things, not just the shape: that the published sha equals the tree the reading was computed on (via rev-parse <commit>^{tree}, i.e. the step's own tree and not the head's), and that git cat-file -t calls it a tree — the one thing the prefix could not establish. Both legs green at head ff1c2779 and the PR is MERGEABLE/CLEAN.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-021410
I verified the defect and the fix against this cycle's own experience, not the
prose: git rev-parse <40-hex> does echo any 40-hex string with rc 0 (a
deadbeef-style control comes back unchanged), so an abbreviated tree sha is a
reading a later cycle cannot check — the full form is what makes the identity
reusable, and git cat-file -t is the only check that answers "does this name a
tree".
The consolidation is the right shape: one _tree_id() for the three print sites
(_judge_every_step, _kept_note, the final tree) rather than three
independently drifting formats, and check-merge-landing-diff.py now prints the
same pair so the two tools agree on what a landing tree is called.
Head ff1c2779, both legs green, MERGEABLE/CLEAN. No code change requested.
Landing-tree measurement (cycle
|
What
Two guards in the merge family published a tree identity they had already computed, truncated to 12 characters:
scripts/check-merge-plan-suite.py— the--stepsline (step N (#M) tree <12>), and the--keepnotescripts/check-merge-landing-diff.py— the landing-tree line in the reportThe same tool already published the complete sha on its final line (
final tree <12> (<40>)), and the tests pin that shape there. The step line drifted from it.Why it matters
A 12-character prefix cannot be reused, and reusing it is the whole point of printing a tree sha — a later reading is compared against it. The way to ask git whether a published sha names an object is
git cat-file -t;git rev-parse <40-hex>echoes any 40-hex string it is handed (deadbeef…came back unchanged, rc 0, so it proves nothing), andcat-fileneeds all 40 characters.Measured cost (cyc20260918-000146): a cycle reviewing #1344/#1345 as a plan read
step 1 (#1344) tree 7c8d56f4f06fandstep 2 (#1345) tree 4e0d146538fc— the two trees its landing-tree votes were being cast on — and could verify neither. It re-ran the whole plan (~116 s) to recover a sha it had already been shown. The source says the fold's pinned date exists so that "a--stepstree sha [is] comparable between runs, which is the point of printing one" — the abbreviation defeated exactly that.Change
_tree_id()incheck-merge-plan-suite.py: one shape, one place,sha[:12] (sha). The abbreviated form stays readable in a log; the complete one is what a later reader compares against. The landing-diff report line takes the same shape — it is the tree a landing-tree vote is bound to, and it had no other print site.Commit shas keep git's conventional abbreviation: the distinction is that a tree identity is what a verdict in this family is bound to.
Verification
test_every_step_publishes_a_tree_sha_a_later_reader_can_verify— each step line carries 40 hex, equal to that step's ownrev-parse <commit>^{tree}, andgit cat-file -tcalls it a tree. Arm (_tree_id→ bare prefix) kills it.test_the_landing_tree_is_published_complete— the same property on the report line, against the treelanding_readingcomputed. Arm (restore the bare prefix) kills it.--collect-onlyreports 2963 on master and 2965 here (+2).from emrg.client.app import run_clientandpython -m emrg --helpgreen;check-doc-count.py --measure→ 2965.check-merge-plan-suite.py --steps 1345 1346→step 1 (#1345) tree 4e0d146538fc (4e0d146538fcf17a35e2681db8d49cda3224ec60) suite OK: 2949 passed, 17 skipped,step 2 (#1346) tree b1a05ed0b91c (b1a05ed0b91cc74c1ce0265115f7fd1dbe20682d) suite OK: 2969 passed, 17 skipped,every step healthy (2 suite run(s))check-merge-landing-diff.py 1345 1346→#1345 landing tree 4e0d146538fc (4e0d146538fcf17a35e2681db8d49cda3224ec60)— the same tree, read by the other tool.