fix(receive-pack): resolve thin-pack REF_DELTA bases from packs, not just loose objects #50
fix/78-thin-pack-packed-base
into main
Closes #78
The defect
git push sends a thin pack by default: its REF_DELTAs are cut against objects the server advertised, which the pack therefore omits. ReceivePack supplies those bases through build_external_resolver/1, which went straight to:
Repo.storage_call(repo, :get_object, [sha])
That is Storage.Filesystem.get_object/3 — a single File.read of objects/ab/cdef…. It sees loose objects and nothing else. Once the bases live in a packfile, every lookup misses, Reader.finalize_deferred/1 exhausts its passes, and the push is rejected outright:
error: remote unpack failed: ref_delta_base_not_found: unresolvable REF_DELTA at offset 1110
! [remote rejected] main -> main
Packed is the steady state of any real repository, and it is exactly what Maintenance.repack/1 produces on purpose — so this blocks #228. Client-side workaround is git push --no-thin; server-side there is none, because the server cannot ask a client to have sent a different pack.
The fix
The resolver now goes through ObjectResolver, the pack-first read every other caller already uses.
Raw bytes, not parse-and-re-encode
The issue proposes ObjectResolver.read/2 followed by Object.encode_content_only/1. I evaluated that first and it does work — but I took the raw-bytes route instead, and the reason is worth stating because it changes the failure mode rather than the pass rate.
I checked the round-trip empirically over 2639 objects: every object in this repository (926 blobs, 688 commits, 1011 trees, 1 tag) plus a hand-built corpus of the shapes that stress the parser — a gpgsig commit including the blank continuation line in PGP armor, mergetag, an encoding header, an empty message, a message with no trailing newline, trees carrying submodule (160000) and symlink (120000) entries, and a signed tag. All 2639 round-tripped byte-exactly. The proposed fix would have been correct today.
What makes it the wrong shape is that nothing downstream would notice if it ever stopped being true. Pack.Reader.emit_resolved/3 computes a resolved entry’s SHA from the delta result, and store_single_entry/2 stores it under that SHA — there is no expected SHA anywhere to check the reconstruction against. A base that was not byte-identical would not fail loudly; it would apply the delta to wrong bytes and write a corrupted object under a self-consistent but wrong SHA. That is a worse outcome than the rejected push this issue is about, and it would be reached by an innocuous future change to the encoder (normalising header order, say).
So read_raw/2 is added to ObjectResolver and Object, returning {type, content} straight out of storage:
- No fidelity dependency. The bytes are the stored bytes.
- Less work per base. A parse plus an encode is skipped on the push path, which resolves one base per REF_DELTA.
- No duplicated control flow.
read/2andread_raw/2share the same pack-first lookup;read/2just projects the raw result throughwrap_object/2. The size cap and SHA verification apply on both the pack and loose paths, as before.
Deletions
decompress_and_parse_object/2, parse_raw_object/2 and classify_object_type/2 go — hand-rolled object parsing duplicating Object, and the only reason the loose-only path existed. The deleted version called bare :zlib.uncompress/1 with no size cap, so the new path additionally bounds decompression the way every other read in the library does.
Tests
Every one of these fails before the fix. Two of them are deliberately built to fail only because of this bug.
receive_pack_thin_pack_test.exs (new) — thin pushes into a pack-only repo, with blob, commit and annotated-tag bases. The fixture asserts the base has no loose copy:
assert {:error, _} = Repo.storage_call(repo, :get_object, [base_blob_sha]),
"base blob must not have a loose copy, or this test is vacuous"
Without that guard these pass against the loose-only resolver and prove nothing. Two controls hold the boundaries — a loose base still resolves, and a base the repo genuinely lacks is still rejected, so the fix cannot have turned an unresolvable delta into a silent success.
receive_pack_git_client_test.exs — a real git push subprocess into a repo put through Maintenance.repack/1. This is the production scenario end to end: real git builds the thin pack, real protocol, real daemon. Reverting the fix reproduces the reported error exactly:
push into a repacked repo failed:
error: remote unpack failed: ref_delta_base_not_found: unresolvable REF_DELTA at offset 196
! [remote rejected] main -> main (ref_delta_base_not_found: unresolvable REF_DELTA at offset 196)
object_resolver_test.exs — read_raw/2 agrees with read/2 byte for byte across every object type (asserting the raw bytes rehash to the object’s own SHA), falls back to loose, reports not-found, and enforces the size cap.
Pre-fix run of the new protocol tests, before the real-git test was added:
1) a push whose blob is deltified against a packed base is accepted
left: {:error, {:ref_delta_base_not_found, "unresolvable REF_DELTA at offset 12"}}
right: :ok
2) a REF_DELTA against a packed base whose own base is a commit (same error)
3) annotated tag as a REF_DELTA base: resolves a packed tag base (same error)
Result: 2/5 passed
The 2 that passed pre-fix are the loose-base and genuinely-absent-base controls, as intended.
Requirements
- REQ-GIT-082 — thin-pack REF_DELTA bases resolve from packfiles, not only from loose objects
- REQ-GIT-083 — delta-base reads return stored object bytes rather than a parse/re-encode round trip
Both created before the tests referencing them; all new tests carry @tag requirements: [...].
Gates
mix test |
1054 passed, 0 failures (52 excluded :s3) |
mix dialyzer |
Total errors: 0 |
mix compile --warnings-as-errors |
clean |
mix format --check-formatted |
clean |
mix credo --strict |
2 warnings / 33 refactoring / 3 readability — byte-identical to unmodified main (verified by stashing); this change adds none. Not a CI gate in this repo. |
Two notes for the reviewer
The pre-push hook corrupted my checkout, again (#75). .githooks/pre-push runs mix test with git’s GIT_DIR exported into the hook process, which bypasses the GIT_CEILING_DIRECTORIES guard entirely. My first push attempt took the suite down to 981/1054 and left core.bare=true, remote.origin.url pointed at a test daemon (http://127.0.0.1:60982/repo), a stray lfs.url, origin/main force-updated to a fixture commit, this branch’s ref force-updated to a fixture commit called pushed, and both indexes rewritten. Repaired per the recipe on #75 and verified: git fsck clean, all branch tips back to their real SHAs, a colleague’s uncommitted work on another branch intact. Pushed with --no-verify after running the suite manually, which is the documented workaround. This bug destroys work and should be fixed ahead of anything else in this repo.
This push did not hit #78, so I cannot offer it as another confirmation. fangorn/ex_git_objectstore on the server is evidently still loose-object storage; fangorn/anvil, which is large and packed, is where five agents hit it today. That asymmetry is itself the point of the issue — the bug arrives when a repo gets packed, which is what #228 does to every repo it touches.