Report a duplicate-id once, not once per check that found it - #371
Merged
Conversation
`validate` runs two independent detectors, and one finding sits under both. A `Record-Id` repeated across a message's own blocks is caught by the shape check, which reads it off `labelRecordBlocks` from the message alone, and again by `checkReferences`, where those same blocks enter `findIdCollisions` as sibling records. The two lists are concatenated with nothing between them. A two-problem message therefore prints four lines, and the summary says four. That number is what a person reads at the moment their commit was just rejected, and it is how they judge how much work they are in for. `--json` carries the same objects byte-identical in pairs, so the repair loop is handed two identical instructions for one edit. Pre-existing, not a regression from #352: the same input against builds at c9a83a2 and at ca538a6 produces the identical four lines. It survived because the reference half is invisible until the `unfetched` gate is past -- a repository with no notes refspec short-circuits `references` to `not-checked` and prints two, which is the common local case and reads correct. Deduped at the seam between the two lists rather than by deciding which check owns the rule, because neither can be the one that goes silent. The shape half is the only half that survives the `unfetched` and shallow gates and the stdin mode that identifies no repository at all -- the commit-msg hook's own path. The reference half is the only half that answers on a resolved `sha`, where the shape half deliberately stands down, and the only half that can see a collision against a note or against an earlier commit at all. The overlap between them is real; only the second copy is the defect. The key is every field that reaches the output -- sha, line, rule, key, value, got, want -- so the multiplicity each check produces on its own is untouched: still one `duplicate-id` per colliding block, still one `cardinality` per extra occurrence. Neither check's status is softened. `shape failed · references failed` stays true, because both of them did fail; what changes is that they no longer say the same thing twice. Record-Id: r-dedupviol Limit: the key includes `line`, so two detectors that locate one finding differently -- one with a line, one without -- would still print it twice; today both resolve the line through the same `locateTrailerLines`/`lineForViolation` path Ruled-out: Letting checkReferences stay silent on duplicate-id | it is the only reporter once an sha is resolved, so --commit and --range would lose the finding outright, and the bug-issue-92 case in validate.test.ts asserts `references failed` on exactly that input Ruled-out: Letting the shape check stay silent on duplicate-id | it is the only half that runs when the notes mirror is unfetched, the clone is shallow, or the message arrives on stdin, so the check would disappear precisely where a commit is being written Ruled-out: Deduping the merged list globally instead of across the seam | it would also fold two byte-identical unknown-key trailers into one, and those are two edits rather than one instruction printed twice Certainty: firm Blast: local Undo: easy Verified: the issue's reproduction, in a clone with the notes refspec configured, printed 4 violations and 4 --json objects before and prints 2 and 2 after, exit 1 either way; the new red case in validate.test.ts fails with [4, 8, 4, 8] against the old code; validate, dogfood, squash, hooks, schema and cli pass at 220, and stale, trailers, query, notes, source-guards, gate-a-e2e, index-db and doctor at 457; typecheck clean and dist rebuilt from source Unverified: whether any rule other than duplicate-id is found by both check classes on one input -- only this one was traced, and the key is content-based rather than rule-specific, so a second overlap would fold without anyone noticing
CommitLore — record lintTrailers: clean — 1 commit in Active constraints for the paths this PR touchesLimits (77)
Ruled out (174)
Warnings (47)
Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #365.
validatereported every same-messageduplicate-idtwice and counted it twice. The shape check finds aRecord-Idrepeated across a message's own blocks from the message alone;checkReferencesfinds the same collision independently, becausefindIdCollisionssees those blocks as sibling records. The two lists were concatenated with nothing between them, so a two-problem message printed four lines, the summary said four, and--jsoncarried the objects byte-identical in pairs — two identical instructions for one edit.Pre-existing, not a regression from #352: the same input against builds at
c9a83a2andca538a6produces the identical four lines. It survived because the reference half is invisible until theunfetchedgate is past — a repository with no notes refspec short-circuitsreferencestonot-checkedand prints two.Before
After
--jsonnow carries two objects rather than four; the exit code is unchanged.Which fix, and why
The issue offered two. This takes the dedup, at the seam where the two lists meet, keyed on every field that reaches the output (
sha,line,rule,key,value,got,want).Deciding which check class owns
duplicate-idwas rejected because neither class can be the one that goes silent:shais resolved — the shape half deliberately stands down there — and the only half that can see a collision against a note or against an earlier commit at all. Silencing it would lose the finding for--commitand--range, andvalidate.test.ts's bug-issue-92 case assertsreferences failedon exactly that input.The overlap between the two detectors is real and worth keeping; only the second copy is the defect. Deduping at the seam rather than over the merged list also leaves the multiplicity each check produces on its own untouched: still one
duplicate-idper colliding block, still onecardinalityper extra occurrence, and two byte-identicalunknown-keytrailers still count as two edits. Neither check's status is softened —shape failed · references failedstays true, because both did fail.Tests
A red case in
test/validate.test.tsasserts the violation count, the per-block lines, the summary count and the--jsonarray against the number of distinct problems. Against the old code it fails with[4, 8, 4, 8]. It runs against a repository so the reference half is genuinely live, and asserts both check statuses first so it cannot pass for the wrong reason.No existing test was changed.
validate,dogfood,squash,hooks,schema,cli: 220 passedstale,trailers,query,notes,source-guards,gate-a-e2e,index-db,doctor: 457 passednpm run typecheckclean;npm run buildrun anddist/committed