Merge branch 'improvements'

Byron · Byron · commit e82f795a56c6 · 2024-08-23T13:01:26.000+02:00
diff --git a/examples/log.rs b/examples/log.rs
@@ -8,7 +8,7 @@ use clap::Parser;
 use gix::{
     bstr::{BString, ByteSlice},
     date::time::format,
-    traverse::commit::simple::Sorting,
+    revision::walk::Sorting,
 };
 
 fn main() {
diff --git a/gitoxide-core/src/repository/commitgraph/list.rs b/gitoxide-core/src/repository/commitgraph/list.rs
@@ -2,7 +2,7 @@ pub(crate) mod function {
     use std::{borrow::Cow, ffi::OsString};
 
     use anyhow::{bail, Context};
-    use gix::{prelude::ObjectIdExt, traverse::commit::simple::Sorting};
+    use gix::{prelude::ObjectIdExt, revision::walk::Sorting};
 
     use crate::OutputFormat;
 
diff --git a/gitoxide-core/src/repository/revision/list.rs b/gitoxide-core/src/repository/revision/list.rs
@@ -17,7 +17,7 @@ pub const PROGRESS_RANGE: std::ops::RangeInclusive<u8> = 0..=2;
 
 pub(crate) mod function {
     use anyhow::{bail, Context};
-    use gix::{hashtable::HashMap, traverse::commit::simple::Sorting, Progress};
+    use gix::{hashtable::HashMap, revision::walk::Sorting, Progress};
     use layout::{
         backends::svg::SVGWriter,
         core::{base::Orientation, geometry::Point, style::StyleAttr},
diff --git a/gix-traverse/src/commit/topo/iter.rs b/gix-traverse/src/commit/topo/iter.rs
@@ -180,7 +180,7 @@ where
 
     fn collect_parents(&mut self, id: &oid) -> Result<SmallVec<[(ObjectId, GenAndCommitTime); 1]>, Error> {
         collect_parents(
-            self.commit_graph.as_ref(),
+            &mut self.commit_graph,
             &self.find,
             id,
             matches!(self.parents, Parents::First),
@@ -193,7 +193,7 @@ where
         &mut self,
         id: &oid,
     ) -> Result<SmallVec<[(ObjectId, GenAndCommitTime); 1]>, Error> {
-        collect_parents(self.commit_graph.as_ref(), &self.find, id, false, &mut self.buf)
+        collect_parents(&mut self.commit_graph, &self.find, id, false, &mut self.buf)
     }
 
     fn pop_commit(&mut self) -> Option<Result<Info, Error>> {
@@ -236,7 +236,7 @@ where
 }
 
 fn collect_parents<Find>(
-    cache: Option<&gix_commitgraph::Graph>,
+    cache: &mut Option<gix_commitgraph::Graph>,
     f: Find,
     id: &oid,
     first_only: bool,
@@ -246,7 +246,7 @@ where
     Find: gix_object::Find,
 {
     let mut parents = SmallVec::<[(ObjectId, GenAndCommitTime); 1]>::new();
-    match find(cache, &f, id, buf)? {
+    match find(cache.as_ref(), &f, id, buf)? {
         Either::CommitRefIter(c) => {
             for token in c {
                 use gix_object::commit::ref_iter::Token as T;
@@ -265,15 +265,21 @@ where
             // Need to check the cache again. That a commit is not in the cache
             // doesn't mean a parent is not.
             for (id, gen_time) in parents.iter_mut() {
-                let commit = find(cache, &f, id, buf)?;
+                let commit = find(cache.as_ref(), &f, id, buf)?;
                 *gen_time = gen_and_commit_time(commit)?;
             }
         }
         Either::CachedCommit(c) => {
             for pos in c.iter_parents() {
+                let Ok(pos) = pos else {
+                    // drop corrupt cache and use ODB from now on.
+                    *cache = None;
+                    return collect_parents(cache, f, id, first_only, buf);
+                };
                 let parent_commit = cache
+                    .as_ref()
                     .expect("cache exists if CachedCommit was returned")
-                    .commit_at(pos?);
+                    .commit_at(pos);
                 parents.push((
                     parent_commit.id().into(),
                     (parent_commit.generation(), parent_commit.committer_timestamp() as i64),
diff --git a/gix-traverse/src/commit/topo/mod.rs b/gix-traverse/src/commit/topo/mod.rs
@@ -11,8 +11,6 @@ pub enum Error {
     #[error("Internal state (bitflags) not found")]
     MissingStateUnexpected,
     #[error(transparent)]
-    CommitGraphFile(#[from] gix_commitgraph::file::commit::Error),
-    #[error(transparent)]
     ObjectDecode(#[from] gix_object::decode::Error),
     #[error(transparent)]
     Find(#[from] gix_object::find::existing_iter::Error),
diff --git a/gix/src/repository/mod.rs b/gix/src/repository/mod.rs
@@ -67,7 +67,6 @@ mod object;
 mod pathspec;
 mod reference;
 mod remote;
-#[cfg(feature = "revision")]
 mod revision;
 mod shallow;
 mod state;
diff --git a/gix/src/repository/revision.rs b/gix/src/repository/revision.rs
@@ -1,4 +1,6 @@
-use crate::{bstr::BStr, revision, Id};
+use crate::revision;
+#[cfg(feature = "revision")]
+use crate::{bstr::BStr, Id};
 
 /// Methods for resolving revisions by spec or working with the commit graph.
 impl crate::Repository {
@@ -9,6 +11,7 @@ impl crate::Repository {
     /// - `@` actually stands for `HEAD`, whereas `git` resolves it to the object pointed to by `HEAD` without making the
     ///   `HEAD` ref available for lookups.
     #[doc(alias = "revparse", alias = "git2")]
+    #[cfg(feature = "revision")]
     pub fn rev_parse<'a>(&self, spec: impl Into<&'a BStr>) -> Result<revision::Spec<'_>, revision::spec::parse::Error> {
         revision::Spec::from_bstr(
             spec,
@@ -22,6 +25,7 @@ impl crate::Repository {
 
     /// Parse a revision specification and return single object id as represented by this instance.
     #[doc(alias = "revparse_single", alias = "git2")]
+    #[cfg(feature = "revision")]
     pub fn rev_parse_single<'repo, 'a>(
         &'repo self,
         spec: impl Into<&'a BStr>,
diff --git a/gix/src/revision/mod.rs b/gix/src/revision/mod.rs
@@ -8,7 +8,7 @@ pub use gix_revision as plumbing;
 ///
 #[allow(clippy::empty_docs)]
 pub mod walk;
-pub use walk::iter::Walk;
+pub use walk::iter_impl::Walk;
 
 ///
 #[cfg(feature = "revision")]
diff --git a/gix/src/revision/spec/parse/delegate/navigate.rs b/gix/src/revision/spec/parse/delegate/navigate.rs
@@ -192,7 +192,7 @@ impl<'repo> delegate::Navigate for Delegate<'repo> {
                     match oid
                         .attach(repo)
                         .ancestors()
-                        .sorting(gix_traverse::commit::simple::Sorting::ByCommitTimeNewestFirst)
+                        .sorting(crate::revision::walk::Sorting::ByCommitTimeNewestFirst)
                         .all()
                     {
                         Ok(iter) => {
@@ -245,7 +245,7 @@ impl<'repo> delegate::Navigate for Delegate<'repo> {
                                     .filter(|r| r.id().header().ok().map_or(false, |obj| obj.kind().is_commit()))
                                     .filter_map(|r| r.detach().peeled),
                             )
-                            .sorting(gix_traverse::commit::simple::Sorting::ByCommitTimeNewestFirst)
+                            .sorting(crate::revision::walk::Sorting::ByCommitTimeNewestFirst)
                             .all()
                         {
                             Ok(iter) => {
diff --git a/gix/src/revision/spec/parse/types.rs b/gix/src/revision/spec/parse/types.rs
@@ -189,7 +189,7 @@ pub enum Error {
         next: Option<Box<dyn std::error::Error + Send + Sync + 'static>>,
     },
     #[error(transparent)]
-    Traverse(#[from] gix_traverse::commit::simple::Error),
+    Traverse(#[from] crate::revision::walk::iter::Error),
     #[error(transparent)]
     Walk(#[from] crate::revision::walk::Error),
     #[error("Spec does not contain a single object id")]
diff --git a/gix/src/revision/walk.rs b/gix/src/revision/walk.rs
@@ -3,7 +3,7 @@ use gix_object::FindExt;
 
 use crate::{ext::ObjectIdExt, revision, Repository};
 
-/// The error returned by [`Platform::all()`].
+/// The error returned by [`Platform::all()`] and [`Platform::selected()`].
 #[derive(Debug, thiserror::Error)]
 #[allow(missing_docs)]
 pub enum Error {
@@ -15,6 +15,65 @@ pub enum Error {
     ConfigBoolean(#[from] crate::config::boolean::Error),
 }
 
+/// Specify how to sort commits during a [revision::Walk] traversal.
+///
+/// ### Sample History
+///
+/// The following history will be referred to for explaining how the sort order works, with the number denoting the commit timestamp
+/// (*their X-alignment doesn't matter*).
+///
+/// ```text
+/// ---1----2----4----7 <- second parent of 8
+///     \              \
+///      3----5----6----8---
+/// ```
+#[derive(Default, Debug, Copy, Clone)]
+pub enum Sorting {
+    /// Commits are sorted as they are mentioned in the commit graph.
+    ///
+    /// In the *sample history* the order would be `8, 6, 7, 5, 4, 3, 2, 1`
+    ///
+    /// ### Note
+    ///
+    /// This is not to be confused with `git log/rev-list --topo-order`, which is notably different from
+    /// as it avoids overlapping branches.
+    #[default]
+    BreadthFirst,
+    /// Commits are sorted by their commit time in descending order, that is newest first.
+    ///
+    /// The sorting applies to all currently queued commit ids and thus is full.
+    ///
+    /// In the *sample history* the order would be `8, 7, 6, 4, 5, 2, 3, 1`
+    ///
+    /// # Performance
+    ///
+    /// This mode benefits greatly from having an [object cache](crate::Repository::object_cache_size) configured
+    /// to avoid having to look up each commit twice.
+    ByCommitTimeNewestFirst,
+    /// This sorting is similar to `ByCommitTimeNewestFirst`, but adds a cutoff to not return commits older than
+    /// a given time, stopping the iteration once no younger commits is queued to be traversed.
+    ///
+    /// As the query is usually repeated with different cutoff dates, this search mode benefits greatly from an object cache.
+    ///
+    /// In the *sample history* and a cut-off date of 4, the returned list of commits would be `8, 7, 6, 4`
+    ByCommitTimeNewestFirstCutoffOlderThan {
+        /// The amount of seconds since unix epoch to use as cut-off time.
+        seconds: gix_date::SecondsSinceUnixEpoch,
+    },
+}
+
+impl Sorting {
+    fn into_simple(self) -> Option<gix_traverse::commit::simple::Sorting> {
+        Some(match self {
+            Sorting::BreadthFirst => gix_traverse::commit::simple::Sorting::BreadthFirst,
+            Sorting::ByCommitTimeNewestFirst => gix_traverse::commit::simple::Sorting::ByCommitTimeNewestFirst,
+            Sorting::ByCommitTimeNewestFirstCutoffOlderThan { seconds } => {
+                gix_traverse::commit::simple::Sorting::ByCommitTimeNewestFirstCutoffOlderThan { seconds }
+            }
+        })
+    }
+}
+
 /// Information about a commit that we obtained naturally as part of the iteration.
 #[derive(Debug, Clone)]
 pub struct Info<'repo> {
@@ -91,7 +150,8 @@ impl<'repo> Info<'repo> {
 pub struct Platform<'repo> {
     pub(crate) repo: &'repo Repository,
     pub(crate) tips: Vec<ObjectId>,
-    pub(crate) sorting: gix_traverse::commit::simple::Sorting,
+    pub(crate) prune: Vec<ObjectId>,
+    pub(crate) sorting: Sorting,
     pub(crate) parents: gix_traverse::commit::Parents,
     pub(crate) use_commit_graph: Option<bool>,
     pub(crate) commit_graph: Option<gix_commitgraph::Graph>,
@@ -106,14 +166,15 @@ impl<'repo> Platform<'repo> {
             parents: Default::default(),
             use_commit_graph: None,
             commit_graph: None,
+            prune: Vec::new(),
         }
     }
 }
 
 /// Create-time builder methods
 impl<'repo> Platform<'repo> {
     /// Set the sort mode for commits to the given value. The default is to order topologically breadth-first.
-    pub fn sorting(mut self, sorting: gix_traverse::commit::simple::Sorting) -> Self {
+    pub fn sorting(mut self, sorting: Sorting) -> Self {
         self.sorting = sorting;
         self
     }
@@ -143,6 +204,37 @@ impl<'repo> Platform<'repo> {
         self.commit_graph = graph;
         self
     }
+
+    /// Prune the commit with the given `ids` such that they won't be returned, and such that none of their ancestors is returned either.
+    ///
+    /// Note that this forces the [sorting](Self::sorting) to
+    /// [`ByCommitTimeNewestFirstCutoffOlderThan`](Sorting::ByCommitTimeNewestFirstCutoffOlderThan) configured with
+    /// the oldest available commit time, ensuring that no commits older than the oldest of `ids` will be returned either.
+    ///
+    /// Also note that commits that can't be accessed or are missing are simply ignored for the purpose of obtaining the cutoff date.
+    #[doc(alias = "hide", alias = "git2")]
+    pub fn with_pruned(mut self, ids: impl IntoIterator<Item = impl Into<ObjectId>>) -> Self {
+        let mut cutoff = match self.sorting {
+            Sorting::ByCommitTimeNewestFirstCutoffOlderThan { seconds } => Some(seconds),
+            Sorting::BreadthFirst | Sorting::ByCommitTimeNewestFirst => None,
+        };
+        for id in ids.into_iter() {
+            let id = id.into();
+            if !self.prune.contains(&id) {
+                if let Some(time) = self.repo.find_commit(id).ok().and_then(|c| c.time().ok()) {
+                    if cutoff.is_none() || cutoff > Some(time.seconds) {
+                        cutoff = time.seconds.into();
+                    }
+                }
+                self.prune.push(id);
+            }
+        }
+
+        if let Some(cutoff) = cutoff {
+            self.sorting = Sorting::ByCommitTimeNewestFirstCutoffOlderThan { seconds: cutoff }
+        }
+        self
+    }
 }
 
 /// Produce the iterator
@@ -162,7 +254,9 @@ impl<'repo> Platform<'repo> {
             parents,
             use_commit_graph,
             commit_graph,
+            mut prune,
         } = self;
+        prune.sort();
         Ok(revision::Walk {
             repo,
             inner: Box::new(
@@ -176,9 +270,12 @@ impl<'repo> Platform<'repo> {
                         if !filter(id) {
                             return false;
                         }
+                        let id = id.to_owned();
+                        if prune.binary_search(&id).is_ok() {
+                            return false;
+                        }
                         match shallow_commits.as_ref() {
                             Some(commits) => {
-                                let id = id.to_owned();
                                 if let Ok(idx) = grafted_parents_to_skip.binary_search(&id) {
                                     grafted_parents_to_skip.remove(idx);
                                     return false;
@@ -195,38 +292,50 @@ impl<'repo> Platform<'repo> {
                         }
                     }
                 })
-                .sorting(sorting)?
+                .sorting(sorting.into_simple().expect("for now there is nothing else"))?
                 .parents(parents)
                 .commit_graph(
                     commit_graph.or(use_commit_graph
                         .map_or_else(|| self.repo.config.may_use_commit_graph(), Ok)?
                         .then(|| self.repo.commit_graph().ok())
                         .flatten()),
-                ),
+                )
+                .map(|res| res.map_err(iter::Error::from)),
             ),
         })
     }
     /// Return an iterator to traverse all commits reachable as configured by the [Platform].
     ///
     /// # Performance
     ///
-    /// It's highly recommended to set an [`object cache`][Repository::object_cache_size()] on the parent repo
+    /// It's highly recommended to set an [`object cache`](Repository::object_cache_size()) on the parent repo
     /// to greatly speed up performance if the returned id is supposed to be looked up right after.
     pub fn all(self) -> Result<revision::Walk<'repo>, Error> {
         self.selected(|_| true)
     }
 }
 
-pub(crate) mod iter {
+///
+#[allow(clippy::empty_docs)]
+pub mod iter {
+    /// The error returned by the [Walk](crate::revision::Walk) iterator.
+    #[derive(Debug, thiserror::Error)]
+    #[allow(missing_docs)]
+    pub enum Error {
+        #[error(transparent)]
+        SimpleTraversal(#[from] gix_traverse::commit::simple::Error),
+    }
+}
+
+pub(crate) mod iter_impl {
     /// The iterator returned by [`crate::revision::walk::Platform::all()`].
     pub struct Walk<'repo> {
         pub(crate) repo: &'repo crate::Repository,
-        pub(crate) inner:
-            Box<dyn Iterator<Item = Result<gix_traverse::commit::Info, gix_traverse::commit::simple::Error>> + 'repo>,
+        pub(crate) inner: Box<dyn Iterator<Item = Result<gix_traverse::commit::Info, super::iter::Error>> + 'repo>,
     }
 
     impl<'repo> Iterator for Walk<'repo> {
-        type Item = Result<super::Info<'repo>, gix_traverse::commit::simple::Error>;
+        type Item = Result<super::Info<'repo>, super::iter::Error>;
 
         fn next(&mut self) -> Option<Self::Item> {
             self.inner
diff --git a/gix/tests/id/mod.rs b/gix/tests/id/mod.rs
diff --git a/gix/tests/repository/shallow.rs b/gix/tests/repository/shallow.rs

Original file line number	Diff line number	Diff line change
`@@ -192,7 +192,7 @@ impl<'repo> delegate::Navigate for Delegate<'repo> {`
`192`	`192`	`match oid`
`193`	`193`	`.attach(repo)`
`194`	`194`	`.ancestors()`
`195`		`- .sorting(gix_traverse::commit::simple::Sorting::ByCommitTimeNewestFirst)`
	`195`	`+ .sorting(crate::revision::walk::Sorting::ByCommitTimeNewestFirst)`
`196`	`196`	`.all()`
`197`	`197`	`{`
`198`	`198`	`Ok(iter) => {`
`@@ -245,7 +245,7 @@ impl<'repo> delegate::Navigate for Delegate<'repo> {`
`245`	`245`	`.filter(\|r\| r.id().header().ok().map_or(false, \|obj\| obj.kind().is_commit()))`
`246`	`246`	`.filter_map(\|r\| r.detach().peeled),`
`247`	`247`	`)`
`248`		`- .sorting(gix_traverse::commit::simple::Sorting::ByCommitTimeNewestFirst)`
	`248`	`+ .sorting(crate::revision::walk::Sorting::ByCommitTimeNewestFirst)`
`249`	`249`	`.all()`
`250`	`250`	`{`
`251`	`251`	`Ok(iter) => {`