fix!: simplify ancestor module..

Byron · Byron · commit e0969c3cc928 · 2024-04-07T20:14:35.000+02:00
We remove ability to reuse `State` as ultimately,this capability
wasn't all that useful.
diff --git a/gix-traverse/src/commit/ancestors.rs b/gix-traverse/src/commit/ancestors.rs
@@ -1,14 +1,8 @@
+use gix_date::SecondsSinceUnixEpoch;
+use gix_hash::ObjectId;
+use gix_hashtable::HashSet;
 use smallvec::SmallVec;
-
-/// An iterator over the ancestors one or more starting commits
-pub struct Ancestors<Find, Predicate, StateMut> {
-    objects: Find,
-    cache: Option<gix_commitgraph::Graph>,
-    predicate: Predicate,
-    state: StateMut,
-    parents: super::Parents,
-    sorting: Sorting,
-}
+use std::collections::VecDeque;
 
 /// Specify how to sort commits during traversal.
 ///
@@ -58,46 +52,39 @@ pub enum Sorting {
     },
 }
 
+/// The error is part of the item returned by the [Ancestors](super::Ancestors) iterator.
+#[derive(Debug, thiserror::Error)]
+#[allow(missing_docs)]
+pub enum Error {
+    #[error(transparent)]
+    Find(#[from] gix_object::find::existing_iter::Error),
+    #[error(transparent)]
+    ObjectDecode(#[from] gix_object::decode::Error),
+}
+
+/// The state used and potentially shared by multiple graph traversals.
+#[derive(Clone)]
+pub(super) struct State {
+    next: VecDeque<ObjectId>,
+    queue: gix_revwalk::PriorityQueue<SecondsSinceUnixEpoch, ObjectId>,
+    buf: Vec<u8>,
+    seen: HashSet<ObjectId>,
+    parents_buf: Vec<u8>,
+    parent_ids: SmallVec<[(ObjectId, SecondsSinceUnixEpoch); 2]>,
+}
+
 ///
 #[allow(clippy::empty_docs)]
-pub mod ancestors {
-    use std::{
-        borrow::{Borrow, BorrowMut},
-        collections::VecDeque,
-    };
-
+mod init {
     use gix_date::SecondsSinceUnixEpoch;
     use gix_hash::{oid, ObjectId};
-    use gix_hashtable::HashSet;
     use gix_object::{CommitRefIter, FindExt};
-    use smallvec::SmallVec;
 
     use super::{
-        super::{Either, Info, ParentIds, Parents},
-        collect_parents, Ancestors, Sorting,
+        super::{Ancestors, Either, Info, ParentIds, Parents},
+        collect_parents, Error, Sorting, State,
     };
 
-    /// The error is part of the item returned by the [Ancestors] iterator.
-    #[derive(Debug, thiserror::Error)]
-    #[allow(missing_docs)]
-    pub enum Error {
-        #[error(transparent)]
-        Find(#[from] gix_object::find::existing_iter::Error),
-        #[error(transparent)]
-        ObjectDecode(#[from] gix_object::decode::Error),
-    }
-
-    /// The state used and potentially shared by multiple graph traversals.
-    #[derive(Clone)]
-    pub struct State {
-        next: VecDeque<ObjectId>,
-        queue: gix_revwalk::PriorityQueue<SecondsSinceUnixEpoch, ObjectId>,
-        buf: Vec<u8>,
-        seen: HashSet<ObjectId>,
-        parents_buf: Vec<u8>,
-        parent_ids: SmallVec<[(ObjectId, SecondsSinceUnixEpoch); 2]>,
-    }
-
     impl Default for State {
         fn default() -> Self {
             State {
@@ -121,12 +108,11 @@ pub mod ancestors {
     }
 
     /// Builder
-    impl<Find, Predicate, StateMut> Ancestors<Find, Predicate, StateMut>
+    impl<Find, Predicate> Ancestors<Find, Predicate>
     where
         Find: gix_object::Find,
-        StateMut: BorrowMut<State>,
     {
-        /// Set the sorting method, either topological or by author date
+        /// Set the `sorting` method.
         pub fn sorting(mut self, sorting: Sorting) -> Result<Self, Error> {
             self.sorting = sorting;
             match self.sorting {
@@ -135,7 +121,7 @@ pub mod ancestors {
                 }
                 Sorting::ByCommitTimeNewestFirst | Sorting::ByCommitTimeNewestFirstCutoffOlderThan { .. } => {
                     let cutoff_time = self.sorting.cutoff_time();
-                    let state = self.state.borrow_mut();
+                    let state = &mut self.state;
                     for commit_id in state.next.drain(..) {
                         let commit_iter = self.objects.find_commit_iter(&commit_id, &mut state.buf)?;
                         let time = commit_iter.committer()?.time.seconds;
@@ -173,7 +159,7 @@ pub mod ancestors {
         }
 
         fn queue_to_vecdeque(&mut self) {
-            let state = self.state.borrow_mut();
+            let state = &mut self.state;
             state.next.extend(
                 std::mem::replace(&mut state.queue, gix_revwalk::PriorityQueue::new())
                     .into_iter_unordered()
@@ -182,55 +168,48 @@ pub mod ancestors {
         }
     }
 
-    /// Initialization
-    impl<Find, StateMut> Ancestors<Find, fn(&oid) -> bool, StateMut>
+    /// Lifecyle
+    impl<Find> Ancestors<Find, fn(&oid) -> bool>
     where
         Find: gix_object::Find,
-        StateMut: BorrowMut<State>,
     {
         /// Create a new instance.
         ///
         /// * `find` - a way to lookup new object data during traversal by their `ObjectId`, writing their data into buffer and returning
         ///    an iterator over commit tokens if the object is present and is a commit. Caching should be implemented within this function
         ///    as needed.
-        /// * `state` - all state used for the traversal. If multiple traversals are performed, allocations can be minimized by reusing
-        ///   this state.
         /// * `tips`
         ///   * the starting points of the iteration, usually commits
         ///   * each commit they lead to will only be returned once, including the tip that started it
-        pub fn new(tips: impl IntoIterator<Item = impl Into<ObjectId>>, state: StateMut, find: Find) -> Self {
-            Self::filtered(tips, state, find, |_| true)
+        pub fn new(tips: impl IntoIterator<Item = impl Into<ObjectId>>, find: Find) -> Self {
+            Self::filtered(tips, find, |_| true)
         }
     }
 
-    /// Initialization
-    impl<Find, Predicate, StateMut> Ancestors<Find, Predicate, StateMut>
+    /// Lifecyle
+    impl<Find, Predicate> Ancestors<Find, Predicate>
     where
         Find: gix_object::Find,
         Predicate: FnMut(&oid) -> bool,
-        StateMut: BorrowMut<State>,
     {
         /// Create a new instance with commit filtering enabled.
         ///
         /// * `find` - a way to lookup new object data during traversal by their `ObjectId`, writing their data into buffer and returning
         ///    an iterator over commit tokens if the object is present and is a commit. Caching should be implemented within this function
         ///    as needed.
-        /// * `state` - all state used for the traversal. If multiple traversals are performed, allocations can be minimized by reusing
-        ///   this state.
         /// * `tips`
         ///   * the starting points of the iteration, usually commits
         ///   * each commit they lead to will only be returned once, including the tip that started it
         /// * `predicate` - indicate whether a given commit should be included in the result as well
         ///   as whether its parent commits should be traversed.
         pub fn filtered(
             tips: impl IntoIterator<Item = impl Into<ObjectId>>,
-            mut state: StateMut,
             find: Find,
             mut predicate: Predicate,
         ) -> Self {
             let tips = tips.into_iter();
+            let mut state = State::default();
             {
-                let state = state.borrow_mut();
                 state.clear();
                 state.next.reserve(tips.size_hint().0);
                 for tip in tips.map(Into::into) {
@@ -250,27 +229,24 @@ pub mod ancestors {
             }
         }
     }
+
     /// Access
-    impl<Find, Predicate, StateMut> Ancestors<Find, Predicate, StateMut>
-    where
-        StateMut: Borrow<State>,
-    {
-        /// Return an iterator for accessing more of the current commits data.
+    impl<Find, Predicate> Ancestors<Find, Predicate> {
+        /// Return an iterator for accessing data of the current commit, parsed lazily.
         pub fn commit_iter(&self) -> CommitRefIter<'_> {
-            CommitRefIter::from_bytes(&self.state.borrow().buf)
+            CommitRefIter::from_bytes(&self.state.buf)
         }
 
-        /// Return the current commits data.
+        /// Return the current commits' raw data, which can be parsed using [`gix_object::CommitRef::from_bytes()`].
         pub fn commit_data(&self) -> &[u8] {
-            &self.state.borrow().buf
+            &self.state.buf
         }
     }
 
-    impl<Find, Predicate, StateMut> Iterator for Ancestors<Find, Predicate, StateMut>
+    impl<Find, Predicate> Iterator for Ancestors<Find, Predicate>
     where
         Find: gix_object::Find,
         Predicate: FnMut(&oid) -> bool,
-        StateMut: BorrowMut<State>,
     {
         type Item = Result<Info, Error>;
 
@@ -300,17 +276,16 @@ pub mod ancestors {
     }
 
     /// Utilities
-    impl<Find, Predicate, StateMut> Ancestors<Find, Predicate, StateMut>
+    impl<Find, Predicate> Ancestors<Find, Predicate>
     where
         Find: gix_object::Find,
         Predicate: FnMut(&oid) -> bool,
-        StateMut: BorrowMut<State>,
     {
         fn next_by_commit_date(
             &mut self,
             cutoff_older_than: Option<SecondsSinceUnixEpoch>,
         ) -> Option<Result<Info, Error>> {
-            let state = self.state.borrow_mut();
+            let state = &mut self.state;
 
             let (commit_time, oid) = state.queue.pop()?;
             let mut parents: ParentIds = Default::default();
@@ -371,14 +346,13 @@ pub mod ancestors {
     }
 
     /// Utilities
-    impl<Find, Predicate, StateMut> Ancestors<Find, Predicate, StateMut>
+    impl<Find, Predicate> Ancestors<Find, Predicate>
     where
         Find: gix_object::Find,
         Predicate: FnMut(&oid) -> bool,
-        StateMut: BorrowMut<State>,
     {
         fn next_by_topology(&mut self) -> Option<Result<Info, Error>> {
-            let state = self.state.borrow_mut();
+            let state = &mut self.state;
             let oid = state.next.pop_front()?;
             let mut parents: ParentIds = Default::default();
             match super::super::find(self.cache.as_ref(), &self.objects, &oid, &mut state.buf) {
@@ -430,8 +404,6 @@ pub mod ancestors {
     }
 }
 
-pub use ancestors::{Error, State};
-
 fn collect_parents(
     dest: &mut SmallVec<[(gix_hash::ObjectId, gix_date::SecondsSinceUnixEpoch); 2]>,
     cache: Option<&gix_commitgraph::Graph>,
diff --git a/gix-traverse/src/commit/mod.rs b/gix-traverse/src/commit/mod.rs
@@ -1,9 +1,19 @@
 use gix_object::FindExt;
 use smallvec::SmallVec;
 
+/// An iterator over the ancestors one or more starting commits
+pub struct Ancestors<Find, Predicate> {
+    objects: Find,
+    cache: Option<gix_commitgraph::Graph>,
+    predicate: Predicate,
+    state: ancestors::State,
+    parents: Parents,
+    sorting: Sorting,
+}
+
 /// Simple ancestors traversal
 pub mod ancestors;
-pub use ancestors::{Ancestors, Sorting};
+pub use ancestors::Sorting;
 
 // Topological traversal
 pub mod topo;
diff --git a/gix-traverse/tests/commit/mod.rs b/gix-traverse/tests/commit/mod.rs
@@ -64,17 +64,12 @@ mod ancestor {
             let (store, tips, expected) = self.setup()?;
 
             for use_commitgraph in [false, true] {
-                let oids = commit::Ancestors::filtered(
-                    tips.clone(),
-                    commit::ancestors::State::default(),
-                    &store,
-                    predicate.clone(),
-                )
-                .sorting(self.sorting)?
-                .parents(self.mode)
-                .commit_graph(self.setup_commitgraph(store.store_ref(), use_commitgraph))
-                .map(|res| res.map(|info| info.id))
-                .collect::<Result<Vec<_>, _>>()?;
+                let oids = commit::Ancestors::filtered(tips.clone(), &store, predicate.clone())
+                    .sorting(self.sorting)?
+                    .parents(self.mode)
+                    .commit_graph(self.setup_commitgraph(store.store_ref(), use_commitgraph))
+                    .map(|res| res.map(|info| info.id))
+                    .collect::<Result<Vec<_>, _>>()?;
 
                 assert_eq!(oids, expected);
             }
@@ -85,7 +80,7 @@ mod ancestor {
             let (store, tips, expected) = self.setup()?;
 
             for use_commitgraph in [false, true] {
-                let oids = commit::Ancestors::new(tips.clone(), commit::ancestors::State::default(), &store)
+                let oids = commit::Ancestors::new(tips.clone(), &store)
                     .sorting(self.sorting)?
                     .parents(self.mode)
                     .commit_graph(self.setup_commitgraph(store.store_ref(), use_commitgraph))
@@ -343,7 +338,7 @@ mod ancestor {
 
     /// Some dates adjusted to be a year apart, but still 'c1' and 'c2' with the same date.
     mod adjusted_dates {
-        use gix_traverse::commit::{ancestors, Ancestors, Parents, Sorting};
+        use gix_traverse::commit::{Ancestors, Parents, Sorting};
 
         use crate::{commit::ancestor::TraversalAssertion, hex_to_id};
 
@@ -398,7 +393,6 @@ mod ancestor {
             let store = gix_odb::at(dir.join(".git").join("objects"))?;
             let iter = Ancestors::new(
                 Some(hex_to_id("9902e3c3e8f0c569b4ab295ddf473e6de763e1e7" /* c2 */)),
-                ancestors::State::default(),
                 &store,
             )
             .sorting(Sorting::ByCommitTimeNewestFirstCutoffOlderThan {
