<!-- agent --> `TreeAdditionsComparedToAncestor` cleared its collected changes before every parent diff. For merge commits, objects added relative to an earlier parent were marked seen and then discarded, producing incomplete packs. Add a regression graph whose merge tree matches its second parent and verify that the blob found only against the first parent is counted. Clear the delegate once before the parent loop so each parent’s additions accumulate. Expose a read-only `gix_pack::testing::Memory` adapter for loose in-memory objects. Use it with `gix_odb::memory::Proxy` in the regression and deduplicate the equivalent benchmark adapter. Git baseline: 15c6308cf7ad276b306aa5b3ababfbdebfb1a917; list-objects.c:add_edge_parents accounts for every merge parent tree during object traversal.
Unified · Split
| @@ -2294,6 +2294,7 @@ dependencies = [ · 2293 unmodified lines | ||
| 2294 | 2294 | "gix-testtools", |
| 2295 | 2295 | "gix-traverse", |
| 2296 | 2296 | "gix-zlib", |
| 2297 | + "insta", | |
| 2297 | 2298 | "maplit", |
| 2298 | 2299 | "memmap2", |
| 2299 | 2300 | "parking_lot", |
| @@ -41,6 +41,8 @@ serde = ["dep:serde", "gix-object/serde", "gix-zlib/serde"] · 40 unmodified lines | ||
| 41 | 41 | parallel = ["gix-features/parallel", "dep:crossbeam-deque"] |
| 42 | 42 | ## Make it possible to compile to the `wasm32-unknown-unknown` target. |
| 43 | 43 | wasm = ["gix-diff?/wasm"] |
| 44 | +## Add test-utilities for tests and benchmarks. | |
| 45 | +testing = [] | |
| 44 | 46 | |
| 45 | 47 | [dependencies] |
| 46 | 48 | gix-features = { version = "^0.49.1", path = "../gix-features", features = ["crc32", "progress"] } |
| @@ -75,14 +77,16 @@ document-features = { version = "0.2.0", optional = true } · 28 unmodified lines | ||
| 75 | 77 | gix-tempfile = { version = "^24.0.0", default-features = false, path = "../gix-tempfile", optional = true } |
| 76 | 78 | |
| 77 | 79 | [dev-dependencies] |
| 78 | −gix-pack = { path = ".", features = ["sha1", "sha256", "pack-cache-lru-dynamic", "pack-cache-lru-static", "object-cache-dynamic", "serde"] } | |
| 80 | +gix-pack = { path = ".", features = ["sha1", "sha256", "pack-cache-lru-dynamic", "pack-cache-lru-static", "object-cache-dynamic", "serde", "testing"] } | |
| 79 | 81 | gix-testtools = { path = "../tests/tools", default-features = false, features = ["sha1", "sha256"] } |
| 80 | 82 | gix-odb = { path = "../gix-odb", features = ["sha1", "sha256"] } |
| 81 | 83 | gix-object = { path = "../gix-object" } |
| 82 | 84 | gix-traverse = { path = "../gix-traverse" } |
| 85 | + | |
| 83 | 86 | bstr = { version = "1.12.0", default-features = false, features = ["std"] } |
| 84 | 87 | maplit = "1.0.2" |
| 85 | 88 | criterion = "0.8.2" |
| 89 | +insta = "1.46.3" | |
| 86 | 90 | |
| 87 | 91 | [package.metadata.docs.rs] |
| 88 | 92 | all-features = true |
| @@ -7,68 +7,22 @@ · 6 unmodified lines | ||
| 7 | 7 | //! |
| 8 | 8 | //! Note that the discovery phase is as fast as the object database can traverse objects, so nothing to test here |
| 9 | 9 | //! and we use in-memory speeds for this. |
| 10 | −use std::{ | |
| 11 | − io, | |
| 12 | − sync::{Arc, atomic::AtomicBool}, | |
| 13 | −}; | |
| 10 | +use std::{io, sync::atomic::AtomicBool}; | |
| 14 | 11 | |
| 15 | 12 | use criterion::{BatchSize, BenchmarkId, Criterion, Throughput, criterion_group}; |
| 16 | 13 | use gix_features::{parallel::InOrderIter, progress}; |
| 17 | 14 | use gix_object::Write as _; |
| 18 | −use gix_pack::data::output::{self, bytes::FromEntriesIter, count, entry}; | |
| 15 | +use gix_pack::{ | |
| 16 | + data::output::{self, bytes::FromEntriesIter, count, entry}, | |
| 17 | + testing::Memory, | |
| 18 | +}; | |
| 19 | 19 | |
| 20 | 20 | /// Object counts to exercise. Kept modest so the benchmark stays runnable while still showing how |
| 21 | 21 | /// the phases scale; raise locally to probe larger, more degenerate repositories. |
| 22 | 22 | const OBJECT_COUNTS: &[usize] = &[1_000, 10_000, 50_000]; |
| 23 | 23 | |
| 24 | −/// Adapt in-memory objects to `gix_pack::Find`. `gix_odb::memory::Proxy` is still used to populate | |
| 25 | −/// the storage, but `count` and `write` need pack-location APIs as well as cheap clones for write | |
| 26 | −/// workers. | |
| 27 | −#[derive(Clone)] | |
| 28 | −struct MemoryOdb { | |
| 29 | − objects: Arc<gix_odb::memory::Storage>, | |
| 30 | −} | |
| 31 | − | |
| 32 | −impl gix_pack::Find for MemoryOdb { | |
| 33 | − fn contains(&self, id: &gix_hash::oid) -> bool { | |
| 34 | − self.objects.contains_key(id) | |
| 35 | − } | |
| 36 | − | |
| 37 | − fn try_find_cached<'a>( | |
| 38 | − &self, | |
| 39 | − id: &gix_hash::oid, | |
| 40 | − buffer: &'a mut Vec<u8>, | |
| 41 | − _pack_cache: &mut dyn gix_pack::cache::DecodeEntry, | |
| 42 | − ) -> Result<Option<(gix_object::Data<'a>, Option<gix_pack::data::entry::Location>)>, gix_object::find::Error> { | |
| 43 | − Ok(self.objects.get(id).map(|(kind, data)| { | |
| 44 | − buffer.clear(); | |
| 45 | − buffer.extend_from_slice(data); | |
| 46 | − ( | |
| 47 | − gix_object::Data { | |
| 48 | − kind: *kind, | |
| 49 | − object_hash: id.kind(), | |
| 50 | − data: buffer, | |
| 51 | − }, | |
| 52 | − None, | |
| 53 | − ) | |
| 54 | − })) | |
| 55 | − } | |
| 56 | − | |
| 57 | − fn location_by_oid(&self, _id: &gix_hash::oid, _buf: &mut Vec<u8>) -> Option<gix_pack::data::entry::Location> { | |
| 58 | − None | |
| 59 | − } | |
| 60 | − | |
| 61 | − fn pack_offsets_and_oid(&self, _pack_id: u32) -> Option<Vec<(gix_pack::data::Offset, gix_hash::ObjectId)>> { | |
| 62 | − None | |
| 63 | − } | |
| 64 | − | |
| 65 | − fn entry_by_location(&self, _location: &gix_pack::data::entry::Location) -> Option<gix_pack::find::Entry> { | |
| 66 | − None | |
| 67 | − } | |
| 68 | −} | |
| 69 | − | |
| 70 | 24 | /// Create a fresh in-memory object database populated with `count` unique blobs. |
| 71 | −fn memory_odb(count: usize) -> (MemoryOdb, Vec<gix_hash::ObjectId>) { | |
| 25 | +fn memory_odb(count: usize) -> (Memory, Vec<gix_hash::ObjectId>) { | |
| 72 | 26 | let mut odb = gix_odb::memory::Proxy::new(gix_odb::sink(gix_hash::Kind::Sha1), gix_hash::Kind::Sha1); |
| 73 | 27 | let mut ids = Vec::with_capacity(count); |
| 74 | 28 | for i in 0..count { |
| @@ -78,19 +32,14 @@ fn memory_odb(count: usize) -> (MemoryOdb, Vec<gix_hash::ObjectId>) { · 3 unmodified lines | ||
| 78 | 32 | .expect("can write an in-memory object"); |
| 79 | 33 | ids.push(id); |
| 80 | 34 | } |
| 81 | − let objects = odb | |
| 35 | + let mut objects = odb | |
| 82 | 36 | .take_object_memory() |
| 83 | 37 | .expect("in-memory object storage is still enabled"); |
| 84 | − ( | |
| 85 | − MemoryOdb { | |
| 86 | − objects: Arc::new(objects), | |
| 87 | − }, | |
| 88 | − ids, | |
| 89 | − ) | |
| 38 | + (Memory::new(objects.drain()), ids) | |
| 90 | 39 | } |
| 91 | 40 | |
| 92 | 41 | /// Phase 1: collect the counts for `ids` without object expansion - exactly the objects given. |
| 93 | −fn count_objects_unthreaded(odb: &MemoryOdb, ids: &[gix_hash::ObjectId]) -> Vec<output::Count> { | |
| 42 | +fn count_objects_unthreaded(odb: &Memory, ids: &[gix_hash::ObjectId]) -> Vec<output::Count> { | |
| 94 | 43 | let mut input = ids.iter().copied().map(Ok); |
| 95 | 44 | let (counts, _outcome) = count::objects_unthreaded( |
| 96 | 45 | odb, |
| @@ -120,7 +69,7 @@ impl io::Write for CountingSink { · 23 unmodified lines | ||
| 120 | 69 | } |
| 121 | 70 | |
| 122 | 71 | /// Phase 2: resolve, sort and encode `counts` into a pack stream, returning the pack size in bytes. |
| 123 | −fn write_pack(odb: &MemoryOdb, counts: Vec<output::Count>) -> u64 { | |
| 72 | +fn write_pack(odb: &Memory, counts: Vec<output::Count>) -> u64 { | |
| 124 | 73 | let num_objects = counts.len() as u32; |
| 125 | 74 | let entries = InOrderIter::from(entry::iter_from_counts( |
| 126 | 75 | counts, |
| @@ -220,6 +220,7 @@ mod expand { · 219 unmodified lines | ||
| 220 | 220 | out = objects.dissolve(stats); |
| 221 | 221 | &traverse_delegate.non_trees |
| 222 | 222 | } else { |
| 223 | + changes_delegate.clear(); | |
| 223 | 224 | for commit_id in &parent_commit_ids { |
| 224 | 225 | let parent_tree_id = { |
| 225 | 226 | let (parent_commit_obj, location) = db.find(commit_id, buf2)?; |
| @@ -251,7 +252,6 @@ mod expand { · 25 unmodified lines | ||
| 251 | 252 | ) |
| 252 | 253 | }; |
| 253 | 254 | |
| 254 | − changes_delegate.clear(); | |
| 255 | 255 | let objects = CountingObjects::new(db); |
| 256 | 256 | gix_diff::tree( |
| 257 | 257 | parent_tree, |
| @@ -57,6 +57,10 @@ pub mod multi_index; · 56 unmodified lines | ||
| 57 | 57 | /// |
| 58 | 58 | pub mod verify; |
| 59 | 59 | |
| 60 | +/// | |
| 61 | +#[cfg(feature = "testing")] | |
| 62 | +pub mod testing; | |
| 63 | + | |
| 60 | 64 | mod mmap { |
| 61 | 65 | use std::path::Path; |
| 62 | 66 | |
| @@ -0,0 +1,51 @@ | ||
| 1 | +use crate::find::Entry; | |
| 2 | + | |
| 3 | +/// An in-memory object database without pack locations. | |
| 4 | +#[derive(Clone)] | |
| 5 | +pub struct Memory { | |
| 6 | + objects: std::sync::Arc<gix_hashtable::HashMap<gix_hash::ObjectId, (gix_object::Kind, Vec<u8>)>>, | |
| 7 | +} | |
| 8 | + | |
| 9 | +impl Memory { | |
| 10 | + /// Create a database containing all `objects`. | |
| 11 | + /// | |
| 12 | + /// Each item is `(id, (kind, data))`, where `data` is the uncompressed object body without a | |
| 13 | + /// loose-object header. IDs are trusted rather than recomputed. If an ID occurs more than once, | |
| 14 | + /// the last item wins. | |
| 15 | + pub fn new(objects: impl IntoIterator<Item = (gix_hash::ObjectId, (gix_object::Kind, Vec<u8>))>) -> Self { | |
| 16 | + Self { | |
| 17 | + objects: std::sync::Arc::new(objects.into_iter().collect()), | |
| 18 | + } | |
| 19 | + } | |
| 20 | +} | |
| 21 | + | |
| 22 | +impl crate::Find for Memory { | |
| 23 | + fn contains(&self, id: &gix_hash::oid) -> bool { | |
| 24 | + self.objects.contains_key(id) | |
| 25 | + } | |
| 26 | + | |
| 27 | + fn try_find_cached<'a>( | |
| 28 | + &self, | |
| 29 | + id: &gix_hash::oid, | |
| 30 | + buffer: &'a mut Vec<u8>, | |
| 31 | + _pack_cache: &mut dyn crate::cache::DecodeEntry, | |
| 32 | + ) -> Result<Option<(gix_object::Data<'a>, Option<crate::data::entry::Location>)>, gix_object::find::Error> { | |
| 33 | + Ok(self.objects.get(id).map(|(kind, data)| { | |
| 34 | + buffer.clear(); | |
| 35 | + buffer.extend_from_slice(data); | |
| 36 | + (gix_object::Data::new(buffer, *kind, id.kind()), None) | |
| 37 | + })) | |
| 38 | + } | |
| 39 | + | |
| 40 | + fn location_by_oid(&self, _id: &gix_hash::oid, _buf: &mut Vec<u8>) -> Option<crate::data::entry::Location> { | |
| 41 | + None | |
| 42 | + } | |
| 43 | + | |
| 44 | + fn pack_offsets_and_oid(&self, _pack_id: u32) -> Option<Vec<(crate::data::Offset, gix_hash::ObjectId)>> { | |
| 45 | + None | |
| 46 | + } | |
| 47 | + | |
| 48 | + fn entry_by_location(&self, _location: &crate::data::entry::Location) -> Option<Entry> { | |
| 49 | + None | |
| 50 | + } | |
| 51 | +} | |
| @@ -367,6 +367,81 @@ fn traversals() -> crate::Result { · 366 unmodified lines | ||
| 367 | 367 | Ok(()) |
| 368 | 368 | } |
| 369 | 369 | |
| 370 | +#[test] | |
| 371 | +fn tree_additions_from_each_merge_parent_are_kept() -> crate::Result { | |
| 372 | + use gix_object::Write; | |
| 373 | + | |
| 374 | + let object_hash = object_hash(); | |
| 375 | + let mut db = gix_odb::memory::Proxy::new(gix_odb::sink(object_hash), object_hash); | |
| 376 | + let added_blob_id = db.write_buf(gix_object::Kind::Blob, b"added")?; | |
| 377 | + let base_tree_id = db.write(&gix_object::Tree::empty())?; | |
| 378 | + let merged_tree_id = db.write(&gix_object::Tree { | |
| 379 | + entries: vec![gix_object::tree::Entry { | |
| 380 | + mode: gix_object::tree::EntryKind::Blob.into(), | |
| 381 | + oid: added_blob_id, | |
| 382 | + filename: "added".into(), | |
| 383 | + }], | |
| 384 | + })?; | |
| 385 | + let write_commit = |tree, parents: &[gix_hash::ObjectId]| { | |
| 386 | + db.write(&gix_object::Commit { | |
| 387 | + tree, | |
| 388 | + parents: parents.iter().copied().collect(), | |
| 389 | + author: Default::default(), | |
| 390 | + committer: Default::default(), | |
| 391 | + encoding: None, | |
| 392 | + message: "commit".into(), | |
| 393 | + extra_headers: Vec::new(), | |
| 394 | + }) | |
| 395 | + }; | |
| 396 | + let base_commit_id = write_commit(base_tree_id, &[])?; | |
| 397 | + let feature_commit_id = write_commit(merged_tree_id, &[base_commit_id])?; | |
| 398 | + let merge_commit_id = write_commit(merged_tree_id, &[base_commit_id, feature_commit_id])?; | |
| 399 | + let mut objects = db | |
| 400 | + .take_object_memory() | |
| 401 | + .expect("in-memory object storage is still enabled"); | |
| 402 | + let db = gix_pack::testing::Memory::new(objects.drain()); | |
| 403 | + let mut input = std::iter::once(Ok::<_, Box<dyn std::error::Error + Send + Sync>>(merge_commit_id)); | |
| 404 | + | |
| 405 | + let (counts, stats) = output::count::objects_unthreaded( | |
| 406 | + &db, | |
| 407 | + &mut input, | |
| 408 | + &progress::Discard, | |
| 409 | + &AtomicBool::new(false), | |
| 410 | + count::objects::ObjectExpansion::TreeAdditionsComparedToAncestor, | |
| 411 | + )?; | |
| 412 | + | |
| 413 | + let expected_ids = std::collections::BTreeSet::from([ | |
| 414 | + added_blob_id, | |
| 415 | + base_tree_id, | |
| 416 | + merged_tree_id, | |
| 417 | + base_commit_id, | |
| 418 | + feature_commit_id, | |
| 419 | + merge_commit_id, | |
| 420 | + ]); | |
| 421 | + assert_eq!( | |
| 422 | + counts.len(), | |
| 423 | + expected_ids.len(), | |
| 424 | + "each expected object must be counted exactly once" | |
| 425 | + ); | |
| 426 | + assert_eq!( | |
| 427 | + counts | |
| 428 | + .iter() | |
| 429 | + .map(|count| count.id) | |
| 430 | + .collect::<std::collections::BTreeSet<_>>(), | |
| 431 | + expected_ids, | |
| 432 | + "additions found against every merge parent must remain in the pack" | |
| 433 | + ); | |
| 434 | + insta::assert_debug_snapshot!(stats, @" | |
| 435 | + Outcome { | |
| 436 | + input_objects: 1, | |
| 437 | + expanded_objects: 5, | |
| 438 | + decoded_objects: 5, | |
| 439 | + total_objects: 6, | |
| 440 | + } | |
| 441 | + "); | |
| 442 | + Ok(()) | |
| 443 | +} | |
| 444 | + | |
| 370 | 445 | /// Reproduces https://github.com/GitoxideLabs/gitoxide/issues/2024: with the default backend's |
| 371 | 446 | /// level 1 being much weaker than it used to be, entries have to be compressed with the |
| 372 | 447 | /// configured level, defaulting to what `git` uses. |