Skip to content

Commit

Permalink
lookup_prefix for ODB (#298)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Byron
Byron committed Feb 22, 2022
1 parent 6244c06 commit a4ccd18
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 57 deletions.
1 change: 1 addition & 0 deletions git-odb/src/find.rs
View file
@@ -1,5 +1,6 @@
/// A way to indicate if a lookup, despite successful, was ambiguous or yielded exactly
/// one result in the particular index.
// TODO: find better name, ambiguous with git_pack::index::PrefixLookupResult (entry_index inside)
pub type PrefixLookupResult = Result<git_hash::ObjectId, ()>;

///
Expand Down
69 changes: 51 additions & 18 deletions git-odb/src/store_impls/dynamic/find.rs
View file
Expand Up @@ -15,6 +15,8 @@ mod error {
pub enum Error {
#[error("An error occurred while obtaining an object from the loose object store")]
Loose(#[from] loose::find::Error),
#[error("An error occurred looking up a prefix which requires iteration")]
LooseWalkDir(#[from] loose::iter::Error),
#[error("An error occurred while obtaining an object from the packed object store")]
Pack(#[from] pack::data::decode_entry::Error),
#[error(transparent)]
Expand All @@ -32,8 +34,44 @@ where
S: Deref<Target = super::Store> + Clone,
{
#[allow(missing_docs)] // TODO: docs
pub fn lookup_prefix(&self, _prefix: git_hash::Prefix) -> Result<Option<crate::find::PrefixLookupResult>, Error> {
todo!()
pub fn lookup_prefix(&self, prefix: git_hash::Prefix) -> Result<Option<crate::find::PrefixLookupResult>, Error> {
let mut candidate: Option<git_hash::ObjectId> = None;
loop {
let snapshot = self.snapshot.borrow();
{
for index in snapshot.indices.iter() {
match (index.lookup_prefix(prefix), candidate) {
(Some(Ok(oid)), Some(candidate)) if candidate != oid => return Ok(Some(Err(()))),
(Some(Ok(_)), Some(_)) | (None, None) | (None, Some(_)) => continue,
(Some(Err(())), _) => return Ok(Some(Err(()))),
(Some(Ok(oid)), None) => {
candidate = Some(oid);
continue;
}
}
}
}

for lodb in snapshot.loose_dbs.iter() {
match (lodb.lookup_prefix(prefix)?, candidate) {
(Some(Ok(oid)), Some(candidate)) if candidate != oid => return Ok(Some(Err(()))),
(Some(Ok(_)), Some(_)) | (None, None) | (None, Some(_)) => continue,
(Some(Err(())), _) => return Ok(Some(Err(()))),
(Some(Ok(oid)), None) => {
candidate = Some(oid);
continue;
}
}
}

match self.store.load_one_index(self.refresh_mode, snapshot.marker)? {
Some(new_snapshot) => {
drop(snapshot);
*self.snapshot.borrow_mut() = new_snapshot;
}
None => return Ok(candidate.map(Ok)),
}
}
}
}

Expand All @@ -46,8 +84,8 @@ where
// TODO: probably make this method fallible, but that would mean its own error type.
fn contains(&self, id: impl AsRef<oid>) -> bool {
let id = id.as_ref();
let mut snapshot = self.snapshot.borrow_mut();
loop {
let mut snapshot = self.snapshot.borrow_mut();
{
for (idx, index) in snapshot.indices.iter().enumerate() {
if index.contains(id) {
Expand All @@ -67,8 +105,7 @@ where

match self.store.load_one_index(self.refresh_mode, snapshot.marker) {
Ok(Some(new_snapshot)) => {
drop(snapshot);
*self.snapshot.borrow_mut() = new_snapshot;
*snapshot = new_snapshot;
}
Ok(None) => return false, // nothing more to load, or our refresh mode doesn't allow disk refreshes
Err(_) => return false, // something went wrong, nothing we can communicate here with this trait. TODO: Maybe that should change?
Expand All @@ -83,8 +120,8 @@ where
pack_cache: &mut impl DecodeEntry,
) -> Result<Option<(Data<'a>, Option<Location>)>, Self::Error> {
let id = id.as_ref();
let mut snapshot = self.snapshot.borrow_mut();
'outer: loop {
let mut snapshot = self.snapshot.borrow_mut();
{
let marker = snapshot.marker;
for (idx, index) in snapshot.indices.iter_mut().enumerate() {
Expand All @@ -105,8 +142,7 @@ where
// The pack wasn't available anymore so we are supposed to try another round with a fresh index
match self.store.load_one_index(self.refresh_mode, snapshot.marker)? {
Some(new_snapshot) => {
drop(snapshot);
*self.snapshot.borrow_mut() = new_snapshot;
*snapshot = new_snapshot;
continue 'outer;
}
None => {
Expand Down Expand Up @@ -166,8 +202,7 @@ where

match self.store.load_one_index(self.refresh_mode, snapshot.marker)? {
Some(new_snapshot) => {
drop(snapshot);
*self.snapshot.borrow_mut() = new_snapshot;
*snapshot = new_snapshot;
}
None => return Ok(None),
}
Expand All @@ -180,8 +215,8 @@ where
"BUG: handle must be configured to `prevent_pack_unload()` before using this method"
);
let id = id.as_ref();
let mut snapshot = self.snapshot.borrow_mut();
'outer: loop {
let mut snapshot = self.snapshot.borrow_mut();
{
let marker = snapshot.marker;
for (idx, index) in snapshot.indices.iter_mut().enumerate() {
Expand All @@ -202,8 +237,7 @@ where
// The pack wasn't available anymore so we are supposed to try another round with a fresh index
match self.store.load_one_index(self.refresh_mode, snapshot.marker).ok()? {
Some(new_snapshot) => {
drop(snapshot);
*self.snapshot.borrow_mut() = new_snapshot;
*snapshot = new_snapshot;
continue 'outer;
}
None => {
Expand Down Expand Up @@ -239,8 +273,7 @@ where

match self.store.load_one_index(self.refresh_mode, snapshot.marker).ok()? {
Some(new_snapshot) => {
drop(snapshot);
*self.snapshot.borrow_mut() = new_snapshot;
*snapshot = new_snapshot;
}
None => return None,
}
Expand Down Expand Up @@ -279,17 +312,17 @@ where
"BUG: handle must be configured to `prevent_pack_unload()` before using this method"
);
let pack_id = PackId::from_intrinsic_pack_id(location.pack_id);
let mut snapshot = self.snapshot.borrow_mut();
let marker = snapshot.marker;
loop {
let mut snapshot = self.snapshot.borrow_mut();
let marker = snapshot.marker;
{
for index in snapshot.indices.iter_mut() {
if let Some(possibly_pack) = index.pack(pack_id) {
let pack = match possibly_pack {
Some(pack) => pack,
None => {
let pack = self.store.load_pack(pack_id, marker).ok()?.expect(
"BUG: pack must exist from previous call to locaion_by_oid() and must not be unloaded",
"BUG: pack must exist from previous call to location_by_oid() and must not be unloaded",
);
*possibly_pack = Some(pack);
possibly_pack.as_deref().expect("just put it in")
Expand Down
9 changes: 9 additions & 0 deletions git-odb/src/store_impls/dynamic/handle.rs
View file
Expand Up @@ -138,6 +138,15 @@ pub(crate) mod index_lookup {
}
}

#[allow(missing_docs)] // TODO: docs
pub(crate) fn lookup_prefix(&self, prefix: git_hash::Prefix) -> Option<crate::find::PrefixLookupResult> {
let res = match &self.file {
handle::SingleOrMultiIndex::Single { index, .. } => index.lookup_prefix(prefix),
handle::SingleOrMultiIndex::Multi { index, .. } => index.lookup_prefix(prefix),
}?;
Some(res.map(|entry_index| self.oid_at_index(entry_index).to_owned()))
}

/// See if the oid is contained in this index, and return its full id for lookup possibly alongside its data file if already
/// loaded.
/// Also return the index itself as it's needed to resolve intra-pack ref-delta objects. They are a possibility even though
Expand Down
68 changes: 29 additions & 39 deletions git-odb/tests/odb/store/dynamic.rs
View file
Expand Up @@ -381,53 +381,43 @@ fn lookup() {

mod lookup_prefix {
use crate::store::dynamic::db_with_all_object_sources;
use git_testtools::hex_to_id;

#[test]
#[should_panic]
fn returns_none_for_prefixes_without_any_match() {
let (handle, _tmp) = db_with_all_object_sources().unwrap();
let prefix = git_hash::Prefix::new(git_hash::ObjectId::null(git_hash::Kind::Sha1), 7).unwrap();
assert!(handle.lookup_prefix(prefix).unwrap().is_none());
}

// #[test]
// fn returns_some_err_for_prefixes_with_more_than_one_match() {
// let objects_dir = git_testtools::tempfile::tempdir().unwrap();
// git_testtools::copy_recursively_into_existing_dir(fixture_path("objects"), &objects_dir).unwrap();
// std::fs::write(
// objects_dir
// .path()
// .join("37")
// .join("d4ffffffffffffffffffffffffffffffffffff"),
// b"fake",
// )
// .unwrap();
// let store = git_odb::loose::Store::at(objects_dir.path(), git_hash::Kind::Sha1);
// let prefix = git_hash::Prefix::new(hex_to_id("37d4e6c5c48ba0d245164c4e10d5f41140cab980"), 4).unwrap();
// assert_eq!(
// store.lookup_prefix(prefix).unwrap(),
// Some(Err(())),
// "there are two objects with that prefix"
// );
// }

// #[test]
// fn iterable_objects_can_be_looked_up_with_varying_prefix_lengths() {
// let store = ldb();
// let hex_lengths = &[4, 7, 40];
// for (index, oid) in store.iter().map(Result::unwrap).enumerate() {
// let hex_len = hex_lengths[index % hex_lengths.len()];
// let prefix = git_hash::Prefix::new(oid, hex_len).unwrap();
// assert_eq!(
// store
// .lookup_prefix(prefix)
// .unwrap()
// .expect("object exists")
// .expect("unambiguous"),
// oid
// );
// }
// }
#[test]
fn returns_some_err_for_prefixes_with_more_than_one_match() {
let (store, _tmp) = db_with_all_object_sources().unwrap();
let prefix = git_hash::Prefix::new(hex_to_id("a7065b5e971a6d8b55875d8cf634a3a37202ab23"), 4).unwrap();
assert_eq!(
store.lookup_prefix(prefix).unwrap(),
Some(Err(())),
"there are two objects with that prefix"
);
}

#[test]
fn iterable_objects_can_be_looked_up_with_varying_prefix_lengths() {
let (store, _tmp) = db_with_all_object_sources().unwrap();
let hex_lengths = &[5, 7, 40];
for (index, oid) in store.iter().unwrap().map(Result::unwrap).enumerate() {
let hex_len = hex_lengths[index % hex_lengths.len()];
let prefix = git_hash::Prefix::new(oid, hex_len).unwrap();
assert_eq!(
store
.lookup_prefix(prefix)
.unwrap()
.expect("object exists")
.expect("unambiguous"),
oid
);
}
}
}

#[test]
Expand Down

0 comments on commit a4ccd18

Please sign in to comment.