feat: build .idx files in decode and fix pack decode bugsFeat/update decode#46
Conversation
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jl.jiang <jiangjl9807@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR implements .idx (pack index) file generation during pack file decoding and fixes several bugs in the decode implementation. The core functionality adds CRC32 checksum computation over compressed pack data and integrates it into the index generation pipeline.
Key changes:
- Added CRC32 tracking during pack object decoding via
CrcCountingReaderwrapper - Modified pack index builder to use idx v2 format for both SHA-1 and SHA-256 (removing the incomplete v3 implementation)
- Added comprehensive integration tests covering SHA-1/SHA-256 and deltified/non-deltified packs
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/internal/pack/decode.rs | Introduces CrcCountingReader to compute CRC32 checksums during pack decoding; updates all object type handlers to track CRC32 |
| src/internal/pack/cache_object.rs | Adds crc32 field to CacheObject struct and updates all constructors and test fixtures |
| src/internal/pack/pack_index.rs | Simplifies header writing to always use idx v2 format; removes incomplete v3 implementation |
| src/internal/pack/index_entry.rs | Updates IndexEntry conversion to use CRC32 from metadata with fallback to decompressed data |
| src/internal/metadata/entry_meta.rs | Adds crc32 field and setter method to EntryMeta |
| src/internal/pack/mod.rs | Exports pack_index module publicly |
| tests/decode-index-pack.rs | New integration test that decodes pack files, generates idx, and validates offsets match |
| tests/data/packs/*.pack | Test pack files stored in Git LFS for SHA-1/SHA-256 and delta/non-delta cases |
| BUCK | Adds new decode-index-pack test target with full dependency list |
| third-party/rust/crates/*/BUCK | Dependency adjustments for various Windows and platform-specific crates |
| buckal.snap | Updated fingerprint reflecting code changes |
| let buf = self.inner.fill_buf().unwrap_or(&[]); | ||
| self.crc.update(&buf[..amt.min(buf.len())]); | ||
| self.bytes_read += amt as u64; |
There was a problem hiding this comment.
The consume method has a critical bug: it calls fill_buf() again to get the buffer for CRC calculation, but this is incorrect because the buffer returned by fill_buf() might have changed after the consumer has called consume(). The correct approach is to track the buffer before consume() is called, or rely only on the Read implementation for CRC calculation. This could lead to incorrect CRC32 values being computed, which would break the integrity of the generated .idx files.
| let buf = self.inner.fill_buf().unwrap_or(&[]); | |
| self.crc.update(&buf[..amt.min(buf.len())]); | |
| self.bytes_read += amt as u64; | |
| // Get the buffer before consuming, as per BufRead contract. | |
| let buf = self.inner.fill_buf().unwrap_or(&[]); | |
| let to_consume = amt.min(buf.len()); | |
| self.crc.update(&buf[..to_consume]); | |
| self.bytes_read += to_consume as u64; |
| // Use the CRC32 from metadata if available (calculated from compressed data), | ||
| // otherwise fallback to calculating it from decompressed data (which is technically wrong for .idx but handles legacy cases) | ||
| let crc32 = pack_entry | ||
| .meta | ||
| .crc32 | ||
| .unwrap_or_else(|| calculate_crc32(&pack_entry.inner.data)); |
There was a problem hiding this comment.
The comment is misleading: it states that calculating CRC32 from decompressed data is "technically wrong for .idx but handles legacy cases". This is not just "technically wrong" – it's fundamentally incorrect for Git pack index files, which require CRC32 to be computed over compressed data. Using CRC32 of decompressed data would produce an invalid .idx file that Git cannot use. The fallback should only occur when the metadata is genuinely unavailable, and ideally should return an error rather than silently producing incorrect data.
| // Use the CRC32 from metadata if available (calculated from compressed data), | |
| // otherwise fallback to calculating it from decompressed data (which is technically wrong for .idx but handles legacy cases) | |
| let crc32 = pack_entry | |
| .meta | |
| .crc32 | |
| .unwrap_or_else(|| calculate_crc32(&pack_entry.inner.data)); | |
| // Use the CRC32 from metadata (must be calculated from compressed data). | |
| // If not present, this is a fatal error: .idx files require CRC32 of compressed data. | |
| let crc32 = pack_entry | |
| .meta | |
| .crc32 | |
| .ok_or(GitError::ConversionError(String::from( | |
| "missing CRC32 in pack entry metadata; cannot generate valid .idx entry", | |
| )))?; |
| map | ||
| } | ||
|
|
||
| fn decode_pack( |
There was a problem hiding this comment.
The function name 'decode_pack' is somewhat ambiguous. A more descriptive name like 'decode_pack_and_extract_metadata' or 'decode_pack_file' would better communicate that this function returns entries with metadata, not just a decoded pack structure.
| let large_count = offsets_bytes | ||
| .chunks_exact(4) | ||
| .filter(|raw| u32::from_be_bytes((*raw).try_into().unwrap()) & 0x8000_0000 != 0) | ||
| .count(); |
There was a problem hiding this comment.
The logic for counting large offsets by filtering with a bitmask is repeated later when parsing offsets (lines 85-90). Consider extracting the large offset detection logic into a helper function to avoid duplication and improve maintainability.
| use crate::utils::CountingReader; | ||
|
|
||
| /// A reader that counts bytes read and computes CRC32 checksum. | ||
| /// which is used to verify the integrity of decompressed data. |
There was a problem hiding this comment.
The comment states that CRC32 is used to "verify the integrity of decompressed data", but CRC32 in Git pack indexes is calculated over the compressed data (including the object header), not the decompressed data. The implementation correctly computes CRC32 over compressed data, but this comment is misleading.
| /// which is used to verify the integrity of decompressed data. | |
| /// The CRC32 is computed over the compressed data (including the object header), | |
| /// as required by the Git pack index format to verify the integrity of the compressed object. |
…decode (libra-tools#46) * feat: add crc32 to decode and introduce idx builder Signed-off-by: jackieismpc <jackieismpc@gmail.com> * build: resolve buck2 failure Signed-off-by: jl.jiang <jiangjl9807@gmail.com> --------- Signed-off-by: jackieismpc <jackieismpc@gmail.com> Signed-off-by: jl.jiang <jiangjl9807@gmail.com> Co-authored-by: jl.jiang <jiangjl9807@gmail.com>
Summary
.idxfiles for pack files.#28