check --repair: share one object header parser, stop re-validating unchanged packs
- Dominant language
- Python
- Stars
- 13.7k
- Forks
- 875
- Avg merge
- 11h 15m
- Merged PRs (30d)
- 192
Description
Two follow-ups from the review of #10094 (see #8476). Both are about object headers in pack files and touch the same code, so they are filed together, but they can be done independently.
## 1. Object header parsing exists in five copies
The check "does this buffer start with a valid object header" (`OBJ_MAGIC`, a version in `SUPPORTED_OBJ_VERSIONS`, sizes that fit) is written out five times:
- `RepoObj.extract_crypted_data`, `repoobj.py:94`
- `RepoObj.parse_meta`, `repoobj.py:168`
- `RepoObj.parse`, `repoobj.py:197`
- `PackReader._parse_header`, `repository.py:381`
- the gap walk in `superseded_gap_ranges`, `repository.py:556`, which unpacks the header inline
The gap walk is not just a duplicate, it is a weaker one: it checks the magic and that the object
ends inside the gap, and nothing else. No version check, no `MAX_DATA_SIZE` check. So it accepts
headers `_parse_header` rejects.
The wording has already drifted: a wrong magic is "invalid object magic" in `repoobj.py` and "no object header" in `_parse_header`. A sixth copy is one new caller away.
Proposal: one `RepoObj.parse_header(buf)` classmethod that returns either the `ObjHeader` or a name for the problem it found. The `RepoObj` methods raise `IntegrityError` from that name, `_parse_header` adds only what is specific to a pack (does the object fit into this pack, is it within `MAX_DATA_SIZE`), and the gap walk calls it instead of unpacking by hand.
Refactoring only. The one visible change is that the error strings stop disagreeing.
## 2. `finish()` validates every pack a second time
Under `--repair` with `chunks_modified` set, `ArchiveChecker.finish()` (`archive.py:2747`) rebuilds the chunks index again using the same validator `check()` already ran. Per object that costs a metadata slot read of up to 1 KiB plus one decryption, where the second pass used to read the 49 byte header and nothing else. It runs over every pack, although the only packs whose contents changed are those that repair rewrote while deleting a defect chunk.
Proposal: remember which packs the first walk validated cleanly, or which packs repair rewrote (the repointed index entries name them), and re-walk only those with `validate`. For the untouched packs, reuse the entries already in `self.chunks` rather than reading them off the store again.
This one deserves a measurement on a repository with many packs, before and after, so the improvement is a number rather than a guess.
Contributor guide
Research direction
Start with the five object-header parsing sites in repoobj.py and repository.py, then read ArchiveChecker.finish() in archive.py alongside the existing check() walk. Confirm how shared parsing can preserve the pack-specific checks, and how chunks records identify packs that need revalidation. Done means the duplicate validation paths are consolidated, unchanged packs are reused under --repair, and the repository-scale measurement shows the effect.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, performance
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100