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:94RepoObj.parse_meta, repoobj.py:168RepoObj.parse, repoobj.py:197PackReader._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.
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 inSUPPORTED_OBJ_VERSIONS, sizes that fit) is written out five times:RepoObj.extract_crypted_data,repoobj.py:94RepoObj.parse_meta,repoobj.py:168RepoObj.parse,repoobj.py:197PackReader._parse_header,repository.py:381superseded_gap_ranges,repository.py:556, which unpacks the header inlineThe 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_SIZEcheck. So it acceptsheaders
_parse_headerrejects.The wording has already drifted: a wrong magic is "invalid object magic" in
repoobj.pyand "no object header" in_parse_header. A sixth copy is one new caller away.Proposal: one
RepoObj.parse_header(buf)classmethod that returns either theObjHeaderor a name for the problem it found. TheRepoObjmethods raiseIntegrityErrorfrom that name,_parse_headeradds only what is specific to a pack (does the object fit into this pack, is it withinMAX_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 timeUnder
--repairwithchunks_modifiedset,ArchiveChecker.finish()(archive.py:2747) rebuilds the chunks index again using the same validatorcheck()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 inself.chunksrather 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.