Skip to content

Commit

Permalink
fix: improve handling of overflows when there are more pack than we c…
Browse files Browse the repository at this point in the history
…an hold.

Internally, there is a statically allocated vec which holds opened packs and
indices.
When reconciling the disk-state with what's currently loaded, it was possible
to get into a situation where there were more files than we could fit into
the slotmap.

The system now fails loudly with a specific Error so callers can deal with it.
  • Loading branch information
Byron committed Jan 13, 2025
1 parent 167f49f commit 34197e7
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 9 deletions.
2 changes: 1 addition & 1 deletion gix-odb/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ pub struct Store {

/// The below state acts like a slot-map with each slot is mutable when the write lock is held, but readable independently of it.
/// This allows multiple file to be loaded concurrently if there is multiple handles requesting to load packs or additional indices.
/// The map is static and cannot typically change.
/// The map is static and cannot change.
/// It's read often and changed rarely.
pub(crate) files: Vec<types::MutableIndexAndPack>,

Expand Down
7 changes: 4 additions & 3 deletions gix-odb/src/store_impls/dynamic/load_index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ impl super::Store {
Option::as_ref(&files_guard).expect("slot is set or we wouldn't know it points to this file");
if index_info.is_multi_index() && files.mtime() != mtime {
// we have a changed multi-pack index. We can't just change the existing slot as it may alter slot indices
// that are currently available. Instead we have to move what's there into a new slot, along with the changes,
// that are currently available. Instead, we have to move what's there into a new slot, along with the changes,
// and later free the slot or dispose of the index in the slot (like we do for removed/missing files).
index_paths_to_add.push_back((index_info, mtime, Some(slot_idx)));
// If the current slot is loaded, the soon-to-be copied multi-index path will be loaded as well.
Expand Down Expand Up @@ -295,10 +295,11 @@ impl super::Store {
.map_or(0, |idx| (idx + 1) % self.files.len());
let mut num_indices_checked = 0;
let mut needs_generation_change = false;
dbg!(idx_by_index_path.len());
let mut slot_indices_to_remove: Vec<_> = idx_by_index_path.into_values().collect();
while let Some((mut index_info, mtime, move_from_slot_idx)) = index_paths_to_add.pop_front() {
'increment_slot_index: loop {
if num_indices_checked == self.files.len() {
if dbg!(num_indices_checked) == dbg!(self.files.len()) {
return Err(Error::InsufficientSlots {
current: self.files.len(),
needed: index_paths_to_add.len() + 1, /*the one currently popped off*/
Expand Down Expand Up @@ -502,7 +503,7 @@ impl super::Store {
}
// Unlike libgit2, do not sort by modification date, but by size and put the biggest indices first. That way
// the chance to hit an object should be higher. We leave it to the handle to sort by LRU.
// Git itself doesn't change the order which may safe time, but we want it to be stable which also helps some tests.
// Git itself doesn't change the order which may save time, but we want it to be stable which also helps some tests.
// NOTE: this will work well for well-packed repos or those using geometric repacking, but force us to open a lot
// of files when dealing with new objects, as there is no notion of recency here as would be with unmaintained
// repositories. Different algorithms should be provided, like newest packs first, and possibly a mix of both
Expand Down
11 changes: 6 additions & 5 deletions gix/tests/gix/remote/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,13 @@ mod blocking_and_async_io {
)?;
Ok(())
}
let remote_dir = tempfile::tempdir()?;
let remote_repo = gix::init_bare(remote_dir.path())?;
create_empty_commit(&remote_repo)?;

for max_packs in 1..=2 {
let remote_dir = tempfile::tempdir()?;
let remote_repo = gix::init_bare(remote_dir.path())?;
create_empty_commit(&remote_repo)?;

let local_dir = tempfile::tempdir()?;
dbg!(max_packs);
let (local_repo, _) = gix::clone::PrepareFetch::new(
remote_repo.path(),
local_dir.path(),
Expand All @@ -122,7 +123,7 @@ mod blocking_and_async_io {
)
.expect("remote is configured after clone")?;
for round in 1.. {
eprintln!("Fetch number {round}…");
dbg!(round);
create_empty_commit(&remote_repo)?;
match remote
.connect(Fetch)?
Expand Down

0 comments on commit 34197e7

Please sign in to comment.