perf: scale readable ref diffs with changed notes#176
Conversation
quick-ricon
left a comment
There was a problem hiding this comment.
Requesting changes for one ref-diff correctness issue.
In lib/diff.sh:145-152, the manifest comparison stores only one readable path per ID (base[$1] = $2). Repeated IDs are accepted by the current materializer and can arise from the merge driver's documented pure-rename limitation. If the base maps one ID to both alpha.md and alias-alpha.md, and the head removes only alpha.md, the retained alias is the last path in both arrays. No candidate ID is emitted, so notes diff reports “No readable note changes” instead of the readable deletion.
There is a second form of the same selection invariant: when multiple IDs map to one readable path, selecting only the changed ID can materialize content that an unchanged colliding ID would overwrite in the full readable tree. The changed-note subset must include the full affected ID/path component, or reject these manifests explicitly rather than emit a misleading patch.
I reproduced the empty-patch case against exact head 0638992bbf9110484a9964872559381e77106393. Fix-it PR #177 derives candidates from exact changed rows and expands the affected alias component, with regression coverage for both shapes.
Apart from this finding, the batched full-tree mapping validation, missing-blob warnings, manifest-only rename path, changed-only --out contract, spaces/nested path quoting, and bounded Git process count held up in review. The mechanical test split preserves all 58 moved test names, and focused diff tests plus all eight lints passed locally.
…st-aliases fix: preserve aliased manifest diff semantics
johnson-ricon
left a comment
There was a problem hiding this comment.
Requesting changes for three ref-diff contract issues found at exact head 3de63a20c69f48378d7aa2171b3d86d41511e36d against exact base 7e19f5f623d4fc4895599f6afad9e36599d6b21f.
-
lib/diff.sh:145-162reduces each manifest to exact row membership. That loses row order, butlib/diff.sh:197-208materializes colliding IDs sequentially and the last row wins. ReorderingA -> shared.md, B -> shared.mdtoB -> shared.md, A -> shared.mdchanges the full readable tree from B's content to A's content while the reducer selects no IDs and reports no change. This is an edge left by #177's exact-row repair. Fix-it #179 records and compares each path's ordered ID sequence before component expansion. -
lib/diff.sh:150-158connects only exactly equal readable paths. A ref mapping one ID tosharedand another toshared/child.mdcannot be represented by full-tree materialization, which always fails atlib/diff.sh:205-208. If only the child ID changes, the reduced path omits the parent and emits a plausible patch instead of preserving that failure. Fix-it #178 validates every existing-blob mapping and rejects file/directory prefix collisions while retaining exact-path aliases. -
lib/diff.sh:167-181rescans every edge to a fixed point. Reverse edge order can advance a long alias component only one hop per pass, making the new component repair quadratic in manifest size. A synthetic 3,001-ID component took 7.258s at this head; fix-it #180's union-find selects the same 3,001 IDs in 0.096s without additional subprocesses or blob reads.
I formed this view before reading the existing review, then checked Quick's review and #177. Quick correctly repaired duplicate-ID aliases and exact multiple-ID path collisions, but the repair still misses ordered overwrite semantics and adds the quadratic fixed-point walk.
The broader reduction is sound for ordinary one-ID/one-path manifests: changed blob IDs, manifest-only renames, missing-blob omission/warnings, NUL-delimited Git path discovery, complete-ref validation, and changed-only --out artifacts all held up. The mechanical BATS split was not part of my verdict.
Validation on the reviewed head: complete suite passed (404 BATS, 9 Python), focused ref-diff suite passed (14/14), all eight Codebase lints passed, mise run doctor passed with only the documented optional local-hook warning, readme build --check, bash -n, and git diff --check passed. Each fix-it separately passed the complete and focused suites plus the same lint/document/syntax checks.
Summary
--outbase/head trees as changed-note review artifactsValidation
mise run test test/diff.bats— 12/12 passedmise run test test-task— 9/9 passed after correcting parallelism textmise run test changes— 15/15 passedgit diff --checkand Fold pre-commit/pre-push checks passedThe full local suite ran once. It passed 400/402 before failing only on the two old parallelism-message assertions; those assertions were then corrected and their focused 9-test suite passed. The 29-minute full suite was not repeated for that wording-only correction. Hosted CI remains the complete post-push gate.