Let a record be re-declared, which is what the spec always said - #435
Merged
Conversation
A commit carrying a record could not be amended. Not to fix a typo, not to add a file left out, not with `--no-edit`. During an amend HEAD is still the commit being replaced, so the id group held its record and the incoming one -- byte-identical -- and `findIdCollisions` fired on the count of commit-sourced records without ever comparing them. That is stricter than SPEC §3.2, which says a `Record-Id` must resolve to exactly one logical record and that "re-declaring that record in later commits is a lifecycle update", reserving identity collision for a note that adds or replaces content under a commit-declared id. §6's own example of the violation is a note adding *different content*. The comparison the spec asks for was already in the function, one branch up: `hasAmbiguousGroup` measures payload divergence, and did so only when a note was involved. The rule was serving half the cases it governs. The commit-sourced branch now makes the same comparison. No hook needs to know an amend is happening, which was the expensive shape this avoids. `commit-msg` receives no signal that would tell it -- measured: only `prepare-commit-msg` gets `commit HEAD`, and `ORIG_HEAD`, `MERGE_MSG` and the `GIT_*` environment are identical on both paths -- so the alternative was cross-hook state written by one hook and read by another. That is the shape of change that shipped an infinite loop earlier today. Byte-identical is narrower than §3.2 allows. A lifecycle update may legitimately carry changes, and deciding which ones remain updates is a wider question; requiring identity cannot overshoot the spec while that stays open. The suite narrowed it twice more, and both are worth keeping. An id that was ever superseded keeps the count rule: two identical declarations straddling a supersession are still two answers to "is this in force", which is a lifecycle ambiguity that content cannot resolve. And the payload has to exist -- two blocks carrying nothing but an id are not one record re-declared, they are an id with no record attached, and reading them as identical would be true only vacuously. A third correction went the other way, into the test rather than the rule. The supersession guard was first asserted end to end and passed alone while failing in the full suite. Fixing the commit instants did not settle it, because `--date` sets only the author date and the ordering reads the committer one -- and setting that did not settle it either. The incoming record has no instant at all at `commit-msg` time, so where it sorts against the superseding commit is undetermined and `hasDeclaredSuccession` can forgive on that alone. The assertion was demanding a tie-break the product cannot decide, and now names the instants explicitly against `findIdCollisions`. Limit: `commit-msg` gets no argument, environment variable or ref that distinguishes an amend from an ordinary commit Ruled-out: recording the amend in `prepare-commit-msg` for `commit-msg` to read | it is cross-hook state in the hooks, needing a marker keyed to HEAD so a stale one cannot suppress a real collision, and the last hook change made at speed hung every push Ruled-out: excluding HEAD from the duplicate walk | it would pass a genuine divergent duplicate whenever the colliding record happened to sit on HEAD Ruled-out: widening the rule to whatever §3.2 might mean by a lifecycle update | that needs deciding which content changes stay updates, and amend needs none of it Warn: this relaxes identity, not content -- a re-declaration differing in any trailer, carrying no payload at all, or following a supersession of the same id is still a collision Blast: module Undo: easy Certainty: firm Verified: six cases drive real git -- amend with --no-edit, a subject fix and a forgotten file all accepted; a divergent record under the same id and a bare id declared twice still refused with `duplicate-id`; the supersession guard is asserted on `findIdCollisions` directly, with instants given, because at commit-msg time the incoming record has none and its sort position is not determined; four of them fail before this change Provenance: authored Record-Id: r-amendid430
CommitLore — record lintTrailers: clean — 1 commit in Active constraints for the paths this PR touchesLimits (96)
Ruled out (224)
Warnings (55)
Truncated: 56 lines omitted — the comment hit GitHub's 65000 character limit. 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 #430.
What was wrong
A commit carrying a record could not be amended. Not to fix a typo, not to add a file left out, not with
--no-edit:During an amend HEAD is still the commit being replaced, so the id group held its record and the incoming one — byte-identical — and
findIdCollisionsfired on the count of commit-sourced records without ever comparing them.The implementation was stricter than its own spec
SPEC §3.2:
And §6's example of the violation: "A note adds different content under a
Record-Idalready declared by a commit."The comparison the spec asks for was already in the function, one branch up —
hasAmbiguousGroupmeasures payload divergence, and did so only when a note was involved. The rule was serving half the cases it governs.Why this shape, and not cross-hook state
commit-msgreceives nothing that identifies an amend. Measured: onlyprepare-commit-msggetscommit HEAD;ORIG_HEAD,.git/MERGE_MSGand theGIT_*environment are identical on both paths. So the alternative was a marker written by one hook and read by another, keyed to HEAD so a stale one could not suppress a real collision — cross-hook state in the hooks, which is the shape of change that shipped an infinite loop earlier today (#422).With this rule no hook needs to know.
The suite narrowed it three times
1. An id that was ever superseded keeps the count rule.
does not let an earlier succession forgive a later duplicatewent red. Two identical declarations straddling a supersession are still two answers to "is this in force" — a lifecycle ambiguity content cannot resolve.2. The payload has to exist. The dogfood alignment test went red on two blocks carrying nothing but an id. Those are not one record re-declared; they are an id with no record attached, and calling them identical is true only vacuously. That shape is a copy-paste and stays a violation.
3. One correction went into the test, not the rule. The supersession guard was first asserted end to end, and passed alone while failing in the full suite. Fixing the commit instants did not settle it —
--datesets only the author date and the ordering reads the committer one — and settingGIT_COMMITTER_DATEdid not settle it either. The incoming record has no instant at all atcommit-msgtime, so where it sorts against the superseding commit is undetermined andhasDeclaredSuccessioncan forgive on that alone. The assertion was demanding a tie-break the product cannot decide; it now names the instants explicitly againstfindIdCollisions.Scope
Byte-identical is narrower than §3.2 allows. A lifecycle update may legitimately carry changes, and deciding which ones stay updates is a wider question — requiring identity cannot overshoot the spec while that is open.
This relaxes identity, not content. A re-declaration that differs in any trailer, carries no payload, or follows a supersession of the same id is still a collision.
Verification
test/amend-recorded-commit.test.ts— 7 cases: three relaxations driven through realgit commit --amend, four guarding what must not weaken. Four are red before this change. Run three times for stability after the ordering fix.test/dogfood.test.tsre-run after committing: 9 passed.devafter Say when the agent's hook is running a different build than you are #434; thedist/conflict was resolved by rebuilding from the merged source.