From dbb68867f882b905fc6584d78cabfa0342039565 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 5 Nov 2023 06:50:15 +0100 Subject: [PATCH] change!: use `gix-object::Find` trait --- gix-ref/src/peel.rs | 11 +----- gix-ref/src/store/file/raw_ext.rs | 35 ++++++++++--------- gix-ref/src/store/file/transaction/mod.rs | 14 ++------ gix-ref/src/store/file/transaction/prepare.rs | 6 ++-- gix-ref/src/store/packed/transaction.rs | 7 ++-- gix-ref/tests/file/mod.rs | 14 ++++++++ gix-ref/tests/file/reference.rs | 19 +++++----- .../create_or_update/collisions.rs | 9 ++--- .../create_or_update/mod.rs | 8 ++--- gix-ref/tests/file/worktree.rs | 18 +++------- 10 files changed, 61 insertions(+), 80 deletions(-) diff --git a/gix-ref/src/peel.rs b/gix-ref/src/peel.rs index 47a231df095..b119a907334 100644 --- a/gix-ref/src/peel.rs +++ b/gix-ref/src/peel.rs @@ -1,12 +1,3 @@ -/// A function for use in [`crate::file::ReferenceExt::peel_to_id_in_place()`] to indicate no peeling should happen. -#[allow(clippy::type_complexity)] -pub fn none( - _id: gix_hash::ObjectId, - #[allow(clippy::ptr_arg)] _buf: &mut Vec, -) -> Result, Box> { - Ok(Some((gix_object::Kind::Commit, &[]))) -} - /// pub mod to_id { use std::path::PathBuf; @@ -26,7 +17,7 @@ pub mod to_id { #[error("Refusing to follow more than {max_depth} levels of indirection")] DepthLimitExceeded { max_depth: usize }, #[error("An error occurred when trying to resolve an object a reference points to")] - Find(#[from] Box), + Find(#[from] gix_object::find::Error), #[error("Object {oid} as referred to by {name:?} could not be found")] NotFound { oid: gix_hash::ObjectId, name: BString }, } diff --git a/gix-ref/src/store/file/raw_ext.rs b/gix-ref/src/store/file/raw_ext.rs index 0aa598351f7..ede518827fd 100644 --- a/gix-ref/src/store/file/raw_ext.rs +++ b/gix-ref/src/store/file/raw_ext.rs @@ -12,12 +12,6 @@ use crate::{ pub trait Sealed {} impl Sealed for crate::Reference {} -pub type FindObjectFn<'a> = dyn FnMut( - gix_hash::ObjectId, - &mut Vec, - ) -> Result, Box> - + 'a; - /// A trait to extend [Reference][crate::Reference] with functionality requiring a [file::Store]. pub trait ReferenceExt: Sealed { /// A step towards obtaining forward or reverse iterators on reference logs. @@ -26,18 +20,22 @@ pub trait ReferenceExt: Sealed { /// For details, see [`Reference::log_exists()`]. fn log_exists(&self, store: &file::Store) -> bool; - /// For details, see [`Reference::peel_to_id_in_place()`]. + /// Follow all symbolic targets this reference might point to and peel the underlying object + /// to the end of the chain, and return it, using `objects` to access them. + /// + /// This is useful to learn where this reference is ultimately pointing to. fn peel_to_id_in_place( &mut self, store: &file::Store, - find: &mut FindObjectFn<'_>, + objects: &dyn gix_object::Find, ) -> Result; - /// For details, see [`Reference::peel_to_id_in_place()`], with support for a known stable packed buffer. + /// Like [`ReferenceExt::peel_to_id_in_place()`], but with support for a known stable packed buffer + /// to use for resolving symbolic links. fn peel_to_id_in_place_packed( &mut self, store: &file::Store, - find: &mut FindObjectFn<'_>, + objects: &dyn gix_object::Find, packed: Option<&packed::Buffer>, ) -> Result; @@ -75,18 +73,18 @@ impl ReferenceExt for Reference { fn peel_to_id_in_place( &mut self, store: &file::Store, - find: &mut FindObjectFn<'_>, + objects: &dyn gix_object::Find, ) -> Result { let packed = store.assure_packed_refs_uptodate().map_err(|err| { peel::to_id::Error::Follow(file::find::existing::Error::Find(file::find::Error::PackedOpen(err))) })?; - self.peel_to_id_in_place_packed(store, find, packed.as_ref().map(|b| &***b)) + self.peel_to_id_in_place_packed(store, objects, packed.as_ref().map(|b| &***b)) } fn peel_to_id_in_place_packed( &mut self, store: &file::Store, - find: &mut FindObjectFn<'_>, + objects: &dyn gix_object::Find, packed: Option<&packed::Buffer>, ) -> Result { match self.peeled { @@ -118,10 +116,13 @@ impl ReferenceExt for Reference { let mut buf = Vec::new(); let mut oid = self.target.try_id().expect("peeled ref").to_owned(); let peeled_id = loop { - let (kind, data) = find(oid, &mut buf)?.ok_or_else(|| peel::to_id::Error::NotFound { - oid, - name: self.name.0.clone(), - })?; + let gix_object::Data { kind, data } = + objects + .try_find(&oid, &mut buf)? + .ok_or_else(|| peel::to_id::Error::NotFound { + oid, + name: self.name.0.clone(), + })?; match kind { gix_object::Kind::Tag => { oid = gix_object::TagRefIter::from_bytes(data).target_id().map_err(|_err| { diff --git a/gix-ref/src/store/file/transaction/mod.rs b/gix-ref/src/store/file/transaction/mod.rs index c3368c1b6cd..c1675803bde 100644 --- a/gix-ref/src/store/file/transaction/mod.rs +++ b/gix-ref/src/store/file/transaction/mod.rs @@ -8,16 +8,6 @@ use crate::{ transaction::RefEdit, }; -/// A function receiving an object id to resolve, returning its decompressed bytes, -/// used to obtain the peeled object ids for storage in packed-refs files. -/// -/// Resolution means to follow tag objects until the end of the chain. -pub type FindObjectFn<'a> = dyn FnMut( - gix_hash::ObjectId, - &mut Vec, - ) -> Result, Box> - + 'a; - /// How to handle packed refs during a transaction #[derive(Default)] pub enum PackedRefs<'a> { @@ -25,11 +15,11 @@ pub enum PackedRefs<'a> { #[default] DeletionsOnly, /// Propagate deletions as well as updates to references which are peeled, that is contain an object id - DeletionsAndNonSymbolicUpdates(Box>), + DeletionsAndNonSymbolicUpdates(Box), /// Propagate deletions as well as updates to references which are peeled, that is contain an object id. Furthermore delete the /// reference which is originally updated if it exists. If it doesn't, the new value will be written into the packed ref right away. /// Note that this doesn't affect symbolic references at all, which can't be placed into packed refs. - DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(Box>), + DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(Box), } #[derive(Debug)] diff --git a/gix-ref/src/store/file/transaction/prepare.rs b/gix-ref/src/store/file/transaction/prepare.rs index eb802f10f1d..34807325856 100644 --- a/gix-ref/src/store/file/transaction/prepare.rs +++ b/gix-ref/src/store/file/transaction/prepare.rs @@ -338,12 +338,10 @@ impl<'s, 'p> Transaction<'s, 'p> { self.packed_transaction = Some(match &mut self.packed_refs { PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(f) | PackedRefs::DeletionsAndNonSymbolicUpdates(f) => { - transaction.prepare(&mut edits_for_packed_transaction.into_iter(), f)? + transaction.prepare(&mut edits_for_packed_transaction.into_iter(), &**f)? } PackedRefs::DeletionsOnly => transaction - .prepare(&mut edits_for_packed_transaction.into_iter(), &mut |_, _| { - unreachable!("BUG: deletions never trigger object lookups") - })?, + .prepare(&mut edits_for_packed_transaction.into_iter(), &gix_object::find::Never)?, }); } } diff --git a/gix-ref/src/store/packed/transaction.rs b/gix-ref/src/store/packed/transaction.rs index e2955f8e069..41a6cdc9b95 100644 --- a/gix-ref/src/store/packed/transaction.rs +++ b/gix-ref/src/store/packed/transaction.rs @@ -2,7 +2,7 @@ use std::{fmt::Formatter, io::Write}; use crate::{ file, - store_impl::{file::transaction::FindObjectFn, packed, packed::Edit}, + store_impl::{packed, packed::Edit}, transaction::{Change, RefEdit}, Target, }; @@ -44,10 +44,11 @@ impl packed::Transaction { /// Lifecycle impl packed::Transaction { /// Prepare the transaction by checking all edits for applicability. + /// Use `objects` to access objects for the purpose of peeling them - this is only used if packed-refs are involved. pub fn prepare( mut self, edits: &mut dyn Iterator, - find: &mut FindObjectFn<'_>, + objects: &dyn gix_object::Find, ) -> Result { assert!(self.edits.is_none(), "BUG: cannot call prepare(…) more than once"); let buffer = &self.buffer; @@ -76,7 +77,7 @@ impl packed::Transaction { { let mut next_id = new; edit.peeled = loop { - let kind = find(next_id, &mut buf)?; + let kind = objects.try_find(&next_id, &mut buf)?.map(|d| d.kind); match kind { Some(gix_object::Kind::Tag) => { next_id = gix_object::TagRefIter::from_bytes(&buf).target_id().map_err(|_| { diff --git a/gix-ref/tests/file/mod.rs b/gix-ref/tests/file/mod.rs index 3d76e537a2e..c92c5b6c278 100644 --- a/gix-ref/tests/file/mod.rs +++ b/gix-ref/tests/file/mod.rs @@ -31,6 +31,20 @@ fn store_writable(name: &str) -> crate::Result<(gix_testtools::tempfile::TempDir )) } +struct EmptyCommit; +impl gix_object::Find for EmptyCommit { + fn try_find<'a>( + &self, + _id: &gix_hash::oid, + _buffer: &'a mut Vec, + ) -> Result>, gix_object::find::Error> { + Ok(Some(gix_object::Data { + kind: gix_object::Kind::Commit, + data: &[], + })) + } +} + mod log; mod reference; mod store; diff --git a/gix-ref/tests/file/reference.rs b/gix-ref/tests/file/reference.rs index 0b69c984bd9..3761b067f28 100644 --- a/gix-ref/tests/file/reference.rs +++ b/gix-ref/tests/file/reference.rs @@ -47,9 +47,9 @@ mod reflog { } mod peel { - use gix_odb::pack::Find; - use gix_ref::{file::ReferenceExt, peel, Reference}; + use gix_ref::{file::ReferenceExt, Reference}; + use crate::file::EmptyCommit; use crate::{file, file::store_with_packed_refs, hex_to_id}; #[test] @@ -77,11 +77,11 @@ mod peel { let store = store_with_packed_refs()?; let mut head: Reference = store.find_loose("HEAD")?.into(); let expected = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03"); - assert_eq!(head.peel_to_id_in_place(&store, &mut peel::none)?, expected); + assert_eq!(head.peel_to_id_in_place(&store, &EmptyCommit)?, expected); assert_eq!(head.target.try_id().map(ToOwned::to_owned), Some(expected)); let mut head = store.find("dt1")?; - assert_eq!(head.peel_to_id_in_place(&store, &mut peel::none)?, expected); + assert_eq!(head.peel_to_id_in_place(&store, &gix_object::find::Never)?, expected); assert_eq!(head.target.into_id(), expected); Ok(()) } @@ -119,12 +119,12 @@ mod peel { assert_eq!(r.kind(), gix_ref::Kind::Symbolic, "there is something to peel"); let commit = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03"); - assert_eq!(r.peel_to_id_in_place(&store, &mut peel::none)?, commit); + assert_eq!(r.peel_to_id_in_place(&store, &EmptyCommit)?, commit); assert_eq!(r.name.as_bstr(), "refs/remotes/origin/multi-link-target3"); let mut r: Reference = store.find_loose("dt1")?.into(); assert_eq!( - r.peel_to_id_in_place(&store, &mut peel::none)?, + r.peel_to_id_in_place(&store, &EmptyCommit)?, hex_to_id("4c3f4cce493d7beb45012e478021b5f65295e5a3"), "points to a tag object without actual object lookup" ); @@ -132,10 +132,7 @@ mod peel { let odb = gix_odb::at(store.git_dir().join("objects"))?; let mut r: Reference = store.find_loose("dt1")?.into(); assert_eq!( - r.peel_to_id_in_place(&store, &mut |oid, buf| { - odb.try_find(&oid, buf) - .map(|obj| obj.map(|(obj, _)| (obj.kind, obj.data))) - })?, + r.peel_to_id_in_place(&store, &odb)?, commit, "points to the commit with lookup" ); @@ -151,7 +148,7 @@ mod peel { assert_eq!(r.name.as_bstr(), "refs/loop-a"); assert!(matches!( - r.peel_to_id_in_place(&store, &mut peel::none).unwrap_err(), + r.peel_to_id_in_place(&store, &gix_object::find::Never).unwrap_err(), gix_ref::peel::to_id::Error::Cycle { .. } )); assert_eq!(r.name.as_bstr(), "refs/loop-a", "the ref is not changed on error"); diff --git a/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/collisions.rs b/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/collisions.rs index bba4e6f614e..586547e5434 100644 --- a/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/collisions.rs +++ b/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/collisions.rs @@ -7,6 +7,7 @@ use gix_ref::{ Target, }; +use crate::file::EmptyCommit; use crate::{ file::transaction::prepare_and_commit::{committer, create_at, create_symbolic_at, delete_at, empty_store}, hex_to_id, @@ -69,14 +70,14 @@ fn packed_refs_lock_is_mandatory_for_multiple_ongoing_transactions_even_if_one_d let _t1 = store .transaction() .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference( - Box::new(|_, _| Ok(Some(gix_object::Kind::Commit))), + Box::new(EmptyCommit), )) .prepare([create_at(ref_name)], Fail::Immediately, Fail::Immediately)?; let t2res = store .transaction() .prepare([delete_at(ref_name)], Fail::Immediately, Fail::Immediately); - assert_eq!(&t2res.unwrap_err().to_string()[..51], "The lock for the packed-ref file could not be obtai", "if packed-refs are about to be created, other transactions always acquire a packed-refs lock as to not miss anything"); + assert_eq!(&t2res.unwrap_err().to_string()[..54], "The lock for the packed-ref file could not be obtained", "if packed-refs are about to be created, other transactions always acquire a packed-refs lock as to not miss anything"); Ok(()) } @@ -86,7 +87,7 @@ fn conflicting_creation_into_packed_refs() -> crate::Result { store .transaction() .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference( - Box::new(|_, _| Ok(Some(gix_object::Kind::Commit))), + Box::new(EmptyCommit), )) .prepare( [ @@ -118,7 +119,7 @@ fn conflicting_creation_into_packed_refs() -> crate::Result { store .transaction() .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference( - Box::new(|_, _| Ok(Some(gix_object::Kind::Commit))), + Box::new(EmptyCommit), )) .prepare( [ diff --git a/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/mod.rs b/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/mod.rs index a3f9f2f22b1..a1d89269f92 100644 --- a/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/mod.rs +++ b/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/mod.rs @@ -3,7 +3,6 @@ use std::convert::TryInto; use gix_hash::ObjectId; use gix_lock::acquire::Fail; use gix_object::bstr::{BString, ByteSlice}; -use gix_object::Find; use gix_ref::{ file::{ transaction::{self, PackedRefs}, @@ -14,6 +13,7 @@ use gix_ref::{ Target, }; +use crate::file::EmptyCommit; use crate::{ file::{ store_with_packed_refs, store_writable, @@ -713,7 +713,7 @@ fn packed_refs_creation_with_packed_refs_mode_prune_removes_original_loose_refs( let edits = store .transaction() .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference( - Box::new(move |oid, buf| odb.try_find(&oid, buf).map(|obj| obj.map(|obj| obj.kind))), + Box::new(odb), )) .prepare( store @@ -782,9 +782,7 @@ fn packed_refs_creation_with_packed_refs_mode_leave_keeps_original_loose_refs() let edits = store .transaction() - .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdates(Box::new(|_, _| { - Ok(Some(gix_object::Kind::Commit)) - }))) + .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdates(Box::new(EmptyCommit))) .prepare(edits, Fail::Immediately, Fail::Immediately)? .commit(committer().to_ref())?; assert_eq!( diff --git a/gix-ref/tests/file/worktree.rs b/gix-ref/tests/file/worktree.rs index d4f8d9fe969..21000bd82f2 100644 --- a/gix-ref/tests/file/worktree.rs +++ b/gix-ref/tests/file/worktree.rs @@ -1,6 +1,5 @@ use std::{cmp::Ordering, path::PathBuf}; -use gix_object::Find; use gix_ref::{file::ReferenceExt, Reference}; use gix_testtools::Creation; @@ -61,13 +60,7 @@ fn into_peel( store: &gix_ref::file::Store, odb: gix_odb::Handle, ) -> impl Fn(gix_ref::Reference) -> gix_hash::ObjectId + '_ { - move |mut r: gix_ref::Reference| { - r.peel_to_id_in_place(store, &mut |id, buf| { - let data = odb.try_find(&id, buf)?; - Ok(data.map(|d| (d.kind, d.data))) - }) - .unwrap() - } + move |mut r: gix_ref::Reference| r.peel_to_id_in_place(store, &odb).unwrap() } enum Mode { @@ -205,6 +198,7 @@ mod writable { FullName, FullNameRef, Target, }; + use crate::file::EmptyCommit; use crate::{ file::{ transaction::prepare_and_commit::committer, @@ -232,9 +226,7 @@ mod writable { let (store, _odb, _tmp) = main_store(packed, Mode::Write)?; let mut t = store.transaction(); if packed { - t = t.packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdates(Box::new(|_, _| { - Ok(Some(gix_object::Kind::Commit)) - }))); + t = t.packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdates(Box::new(EmptyCommit))); } let edits = t @@ -514,9 +506,7 @@ mod writable { let mut t = store.transaction(); if packed { - t = t.packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdates(Box::new(|_, _| { - Ok(Some(gix_object::Kind::Commit)) - }))); + t = t.packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdates(Box::new(EmptyCommit))); } let edits = t