Stop serving a record from a commit the history no longer reaches - #360
Merged
Conversation
Record a decision, then `git reset --hard` before pushing, and the record you abandoned goes on answering. `git notes` is keyed by object name and knows nothing about refs, so the mirror outlives the commit it annotates: the object stays addressable until gc prunes it, and `git notes list` keeps naming it. `readNoteRecords` enumerated that list with no reachability filter at all. Both failure directions land at once, which is why this is a bug and not a stale extra row. The abandoned record is served as active, and its `Supersedes:` retires the record that survived in history -- the audit of v0.5.1 saw `context` answer argon2id out of a commit that no longer exists while withholding the bcrypt record `git log` still shows. `stale` answered correctly on the same repository the whole time, because `commands/stale.ts` has always intersected its notes with the commits its walk saw. The filter is that intersection, and the set is the `rev-list HEAD` the callers already made: `rebuildIndex` and `scanTrailers` hand over the walk they were going to pass to `readCommitRecords` anyway, so the fix costs no git process on either path. Only `indexNotes` pays for one, and only when the notes ref has moved -- it re-reads the mirror whole and holds no walk of its own. A non-fast-forward HEAD move is already a full rebuild, which is what keeps the incremental path from carrying a note whose commit it just orphaned. HEAD is deliberately the boundary rather than every ref. The commit source has only ever read `rev-list HEAD`, so anything wider would rebuild the same disagreement in the other direction and let a branch nobody is on retire a record that is live on the one you are. A note whose commit is already pruned was never the problem: `git gc` deletes the unreachable object and leaves the note behind, `cat-file --batch-check` answers `missing`, and the existing type filter drops it. What this closes is the window before gc runs -- two weeks to ninety days by default -- which is exactly the window in which "record a decision, then rebase before pushing" leaves a phantom behind. Record-Id: r-notereach Limit: reachability is decided against HEAD alone, so a record mirrored onto a live branch that is not checked out is not served until it is Ruled-out: Filtering against every ref instead of HEAD | the commit source reads rev-list HEAD, so a wider boundary for notes puts the two sources back out of step in the other direction and lets an abandoned branch supersede a live record Ruled-out: git merge-base --is-ancestor per annotated commit | it spends a process per record to answer what the HEAD walk the callers already hold answers for nothing Certainty: firm Blast: module Undo: easy Verified: the issue's sequence is now a test -- commit A carrying r-old001, commit B carrying r-new001 and Supersedes r-old001, B mirrored into refs/notes/commitlore, reset --hard A -- and it failed on "expected [ ...(5) ] to not include 'r-new001'" before the filter and passes after; index-db, notes, stale and query pass at 241 and the whole suite at 1939 passed and 1 skipped across 78 files; npm run typecheck is clean and dist is rebuilt from this source Unverified: how many repositories keep records on a branch other than the checked-out one, which is what the HEAD boundary narrows
CommitLore — record lintTrailers: clean — 1 commit in Active constraints for the paths this PR touchesLimits (72)
Ruled out (160)
Warnings (48)
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 #351
readNoteRecordsenumeratedgit notes --ref=refs/notes/commitlore listwith noreachability filter, so a record mirrored onto a commit that history later
abandoned —
git reset --hard, a dropped branch, a rebase — went on being servedas active, and its
Supersedes:retired the record that is actually live. Bothfailure directions at once, as the issue says.
commands/stale.ts:131-134has always intersected its notes with the commits itswalk saw, which is why
staleandcontextdisagreed on the same repository.This applies the same intersection to the index and to the
--no-indexpath.What changed
readNoteRecordsnow takes the reachable set and drops every note outside it.rebuildIndexandscanTrailerspass therev-list HEADwalk they alreadymade for
readCommitRecords, so neither path spends a new git process.indexNotesholds no walk of its own, so it makes one — and only when thenotes ref has moved. A non-fast-forward HEAD move is already a full rebuild,
which is what stops the incremental path carrying a note whose commit it just
orphaned.
HEAD is the boundary rather than every ref, on purpose: the commit source has
only ever read
rev-list HEAD, so a wider boundary for notes would rebuild thesame disagreement in the other direction.
Test
test/index-db.test.ts— "does not serve a note on a commit HEAD no longerreaches (bug-issue-351)" builds the issue's sequence exactly: commit A carrying
Record-Id: r-old001, commit B carryingr-new001+Supersedes: r-old001, B'srecord mirrored into the notes ref, then
git reset --hard A. Written first andconfirmed RED against unmodified source:
No existing test was weakened.
gc caveat
A note whose commit
git gc --prunehas already removed was never the problem:gc deletes the unreachable object and leaves the note,
cat-file --batch-checkanswers
missing, and the existing type filter drops it. This closes the windowbefore gc runs — two weeks to ninety days by default.
Verification
npx vitest run test/index-db.test.ts test/notes.test.ts test/stale.test.ts test/query.test.ts→ 241 passednpx vitest run(full suite) → 78 files, 1939 passed, 1 skippednpm run typecheckclean,npm run buildrun anddist/committed