Merge pull request #1564 from Byron/improvements

improvements

Author: Byron

gitoxide-core/src/repository/merge_base.rs

@@ -20,7 +20,8 @@ pub fn merge_base(
         .collect::<Result<_, _>>()?;
 
     let cache = repo.commit_graph_if_enabled()?;
-    let bases = repo.merge_bases_many_with_cache(first_id, &other_ids, cache.as_ref())?;
+    let mut graph = repo.revision_graph(cache.as_ref());
+    let bases = repo.merge_bases_many_with_graph(first_id, &other_ids, &mut graph)?;
     if bases.is_empty() {
         bail!("No base found for {first} and {others}", others = others.join(", "))
     }

gix-negotiate/src/consecutive.rs

@@ -23,7 +23,7 @@ impl Algorithm {
         let mut is_common = false;
         let mut has_mark = false;
         if let Some(commit) = graph
-            .try_lookup_or_insert_commit(id, |data| {
+            .get_or_insert_commit(id, |data| {
                 has_mark = data.flags.intersects(mark);
                 data.flags |= mark;
                 is_common = data.flags.contains(Flags::COMMON);
@@ -47,7 +47,7 @@ impl Algorithm {
     ) -> Result<(), Error> {
         let mut is_common = false;
         if let Some(commit) = graph
-            .try_lookup_or_insert_commit(id, |data| is_common = data.flags.contains(Flags::COMMON))?
+            .get_or_insert_commit(id, |data| is_common = data.flags.contains(Flags::COMMON))?
             .filter(|_| !is_common)
         {
             let mut queue = gix_revwalk::PriorityQueue::from_iter(Some((commit.commit_time, (id, 0_usize))));
@@ -64,11 +64,11 @@ impl Algorithm {
                 {
                     self.add_to_queue(id, Flags::SEEN, graph)?;
                 } else if matches!(ancestors, Ancestors::AllUnseen) || generation < 2 {
-                    if let Some(commit) = graph.try_lookup_or_insert_commit(id, |_| {})? {
+                    if let Some(commit) = graph.get_or_insert_commit(id, |_| {})? {
                         for parent_id in commit.parents.clone() {
                             let mut prev_flags = Flags::default();
                             if let Some(parent) = graph
-                                .try_lookup_or_insert_commit(parent_id, |data| {
+                                .get_or_insert_commit(parent_id, |data| {
                                     prev_flags = data.flags;
                                     data.flags |= Flags::COMMON;
                                 })?

gix-negotiate/src/lib.rs

@@ -144,4 +144,4 @@ pub trait Negotiator {
 }
 
 /// An error that happened during any of the methods on a [`Negotiator`].
-pub type Error = gix_revwalk::graph::try_lookup_or_insert_default::Error;
+pub type Error = gix_revwalk::graph::get_or_insert_default::Error;

gix-negotiate/src/skipping.rs

@@ -20,7 +20,7 @@ impl Default for Algorithm {
 impl Algorithm {
     /// Add `id` to our priority queue and *add* `flags` to it.
     fn add_to_queue(&mut self, id: ObjectId, mark: Flags, graph: &mut crate::Graph<'_, '_>) -> Result<(), Error> {
-        let commit = graph.try_lookup_or_insert_commit(id, |entry| {
+        let commit = graph.get_or_insert_commit(id, |entry| {
             entry.flags |= mark | Flags::SEEN;
         })?;
         if let Some(timestamp) = commit.map(|c| c.commit_time) {
@@ -35,15 +35,15 @@ impl Algorithm {
     fn mark_common(&mut self, id: ObjectId, graph: &mut crate::Graph<'_, '_>) -> Result<(), Error> {
         let mut is_common = false;
         if let Some(commit) = graph
-            .try_lookup_or_insert_commit(id, |entry| {
+            .get_or_insert_commit(id, |entry| {
                 is_common = entry.flags.contains(Flags::COMMON);
                 entry.flags |= Flags::COMMON;
             })?
             .filter(|_| !is_common)
         {
             let mut queue = gix_revwalk::PriorityQueue::from_iter(Some((commit.commit_time, id)));
             while let Some(id) = queue.pop_value() {
-                if let Some(commit) = graph.try_lookup_or_insert_commit(id, |entry| {
+                if let Some(commit) = graph.get_or_insert_commit(id, |entry| {
                     if !entry.flags.contains(Flags::POPPED) {
                         self.non_common_revs -= 1;
                     }
@@ -56,7 +56,7 @@ impl Algorithm {
                         }
                         let mut was_unseen_or_common = false;
                         if let Some(parent) = graph
-                            .try_lookup_or_insert_commit(parent_id, |entry| {
+                            .get_or_insert_commit(parent_id, |entry| {
                                 was_unseen_or_common =
                                     !entry.flags.contains(Flags::SEEN) || entry.flags.contains(Flags::COMMON);
                                 entry.flags |= Flags::COMMON;

gix-revision/src/merge_base.rs

@@ -18,19 +18,15 @@ bitflags::bitflags! {
 #[derive(Debug, thiserror::Error)]
 #[allow(missing_docs)]
 pub enum Error {
-    #[error(transparent)]
-    IterParents(#[from] gix_revwalk::graph::commit::iter_parents::Error),
-    #[error("A commit could not be found")]
-    FindExistingCommit(#[from] gix_object::find::existing_iter::Error),
-    #[error("A commit could not be decoded during traversal")]
-    Decode(#[from] gix_object::decode::Error),
+    #[error("A commit could not be inserted into the graph")]
+    InsertCommit(#[from] gix_revwalk::graph::get_or_insert_default::Error),
 }
 
 pub(crate) mod function {
     use super::Error;
     use crate::{merge_base::Flags, Graph, PriorityQueue};
     use gix_hash::ObjectId;
-    use gix_revwalk::graph::LazyCommit;
+    use gix_revwalk::graph;
     use std::cmp::Ordering;
 
     /// Given a commit at `first` id, traverse the commit `graph` and return all possible merge-base between it and `others`,
@@ -39,19 +35,23 @@ pub(crate) mod function {
     ///
     /// Note that this function doesn't do any work if `first` is contained in `others`, which is when `first` will be returned
     /// as only merge-base right away. This is even the case if some commits of `others` are disjoint.
+    ///
+    /// # Performance
+    ///
+    /// For repeated calls, be sure to re-use `graph` as its content will be kept and reused for a great speed-up. The contained flags
+    /// will automatically be cleared.
     pub fn merge_base(
         first: ObjectId,
         others: &[ObjectId],
-        graph: &mut Graph<'_, '_, Flags>,
+        graph: &mut Graph<'_, '_, graph::Commit<Flags>>,
     ) -> Result<Option<Vec<ObjectId>>, Error> {
         let _span = gix_trace::coarse!("gix_revision::merge_base()", ?first, ?others);
         if others.is_empty() || others.contains(&first) {
             return Ok(Some(vec![first]));
         }
 
-        graph.clear();
+        graph.clear_commit_data(|f| *f = Flags::empty());
         let bases = paint_down_to_common(first, others, graph)?;
-        graph.clear();
 
         let bases = remove_redundant(&bases, graph)?;
         Ok((!bases.is_empty()).then_some(bases))
@@ -61,11 +61,12 @@ pub(crate) mod function {
     /// That way, we return only the topologically most recent commits in `commits`.
     fn remove_redundant(
         commits: &[(ObjectId, GenThenTime)],
-        graph: &mut Graph<'_, '_, Flags>,
+        graph: &mut Graph<'_, '_, graph::Commit<Flags>>,
     ) -> Result<Vec<ObjectId>, Error> {
         if commits.is_empty() {
             return Ok(Vec::new());
         }
+        graph.clear_commit_data(|f| *f = Flags::empty());
         let _span = gix_trace::detail!("gix_revision::remove_redundant()", num_commits = %commits.len());
         let sorted_commits = {
             let mut v = commits.to_vec();
@@ -77,36 +78,46 @@ pub(crate) mod function {
 
         let mut walk_start = Vec::with_capacity(commits.len());
         for (id, _) in commits {
-            graph.insert_parents_with_lookup(id, &mut |parent_id, parent_data, maybe_flags| -> Result<_, Error> {
-                if maybe_flags.map_or(true, |flags| !flags.contains(Flags::STALE)) {
-                    walk_start.push((parent_id, GenThenTime::try_from(parent_data)?));
-                }
-                Ok(Flags::empty())
-            })?;
-            graph.insert(*id, Flags::RESULT);
+            let commit = graph.get_mut(id).expect("previously added");
+            commit.data |= Flags::RESULT;
+            for parent_id in commit.parents.clone() {
+                graph.get_or_insert_full_commit(parent_id, |parent| {
+                    // prevent double-addition
+                    if !parent.data.contains(Flags::STALE) {
+                        parent.data |= Flags::STALE;
+                        walk_start.push((parent_id, GenThenTime::from(&*parent)));
+                    }
+                })?;
+            }
         }
         walk_start.sort_by(|a, b| a.0.cmp(&b.0));
+        // allow walking everything at first.
+        walk_start
+            .iter_mut()
+            .for_each(|(id, _)| graph.get_mut(id).expect("added previously").data.remove(Flags::STALE));
         let mut count_still_independent = commits.len();
 
         let mut stack = Vec::new();
         while let Some((commit_id, commit_info)) = walk_start.pop().filter(|_| count_still_independent > 1) {
             stack.clear();
-            graph.insert(commit_id, Flags::STALE);
+            graph.get_mut(&commit_id).expect("added").data |= Flags::STALE;
             stack.push((commit_id, commit_info));
 
             while let Some((commit_id, commit_info)) = stack.last().copied() {
-                let flags = graph.get_mut(&commit_id).expect("all commits have been added");
-                if flags.contains(Flags::RESULT) {
-                    flags.remove(Flags::RESULT);
+                let commit = graph.get_mut(&commit_id).expect("all commits have been added");
+                let commit_parents = commit.parents.clone();
+                if commit.data.contains(Flags::RESULT) {
+                    commit.data.remove(Flags::RESULT);
                     count_still_independent -= 1;
                     if count_still_independent <= 1 {
                         break;
                     }
-                    if commit_id == sorted_commits[min_gen_pos].0 {
+                    if *commit_id == *sorted_commits[min_gen_pos].0 {
                         while min_gen_pos < commits.len() - 1
                             && graph
                                 .get(&sorted_commits[min_gen_pos].0)
                                 .expect("already added")
+                                .data
                                 .contains(Flags::STALE)
                         {
                             min_gen_pos += 1;
@@ -120,87 +131,81 @@ pub(crate) mod function {
                     continue;
                 }
 
-                let mut pushed_one_parent = false;
-                graph.insert_parents_with_lookup(&commit_id, &mut |parent_id,
-                                                                    parent_data,
-                                                                    maybe_flags|
-                 -> Result<_, Error> {
-                    let is_new_parent = !pushed_one_parent
-                        && maybe_flags.map_or(true, |flags| {
-                            let res = !flags.contains(Flags::STALE);
-                            *flags |= Flags::STALE;
-                            res
-                        });
-                    if is_new_parent {
-                        stack.push((parent_id, GenThenTime::try_from(parent_data)?));
-                        pushed_one_parent = true;
+                let previous_len = stack.len();
+                for parent_id in &commit_parents {
+                    if graph
+                        .get_or_insert_full_commit(*parent_id, |parent| {
+                            if !parent.data.contains(Flags::STALE) {
+                                parent.data |= Flags::STALE;
+                                stack.push((*parent_id, GenThenTime::from(&*parent)));
+                            }
+                        })?
+                        .is_some()
+                    {
+                        break;
                     }
-                    Ok(Flags::STALE)
-                })?;
+                }
 
-                if !pushed_one_parent {
+                if previous_len == stack.len() {
                     stack.pop();
                 }
             }
         }
 
         Ok(commits
             .iter()
-            .filter_map(|(id, _info)| graph.get(id).filter(|flags| !flags.contains(Flags::STALE)).map(|_| *id))
+            .filter_map(|(id, _info)| {
+                graph
+                    .get(id)
+                    .filter(|commit| !commit.data.contains(Flags::STALE))
+                    .map(|_| *id)
+            })
             .collect())
     }
 
     fn paint_down_to_common(
         first: ObjectId,
         others: &[ObjectId],
-        graph: &mut Graph<'_, '_, Flags>,
+        graph: &mut Graph<'_, '_, graph::Commit<Flags>>,
     ) -> Result<Vec<(ObjectId, GenThenTime)>, Error> {
         let mut queue = PriorityQueue::<GenThenTime, ObjectId>::new();
-        graph.insert_data(first, |commit| -> Result<_, Error> {
-            queue.insert(commit.try_into()?, first);
-            Ok(Flags::COMMIT1)
+        graph.get_or_insert_full_commit(first, |commit| {
+            commit.data |= Flags::COMMIT1;
+            queue.insert(GenThenTime::from(&*commit), first);
         })?;
 
         for other in others {
-            graph.insert_data(*other, |commit| -> Result<_, Error> {
-                queue.insert(commit.try_into()?, *other);
-                Ok(Flags::COMMIT2)
+            graph.get_or_insert_full_commit(*other, |commit| {
+                commit.data |= Flags::COMMIT2;
+                queue.insert(GenThenTime::from(&*commit), *other);
             })?;
         }
 
         let mut out = Vec::new();
-        while queue
-            .iter_unordered()
-            .any(|id| graph.get(id).map_or(false, |data| !data.contains(Flags::STALE)))
-        {
+        while queue.iter_unordered().any(|id| {
+            graph
+                .get(id)
+                .map_or(false, |commit| !commit.data.contains(Flags::STALE))
+        }) {
             let (info, commit_id) = queue.pop().expect("we have non-stale");
-            let flags_mut = graph.get_mut(&commit_id).expect("everything queued is in graph");
-            let mut flags_without_result = *flags_mut & (Flags::COMMIT1 | Flags::COMMIT2 | Flags::STALE);
+            let commit = graph.get_mut(&commit_id).expect("everything queued is in graph");
+            let mut flags_without_result = commit.data & (Flags::COMMIT1 | Flags::COMMIT2 | Flags::STALE);
             if flags_without_result == (Flags::COMMIT1 | Flags::COMMIT2) {
-                if !flags_mut.contains(Flags::RESULT) {
-                    *flags_mut |= Flags::RESULT;
+                if !commit.data.contains(Flags::RESULT) {
+                    commit.data |= Flags::RESULT;
                     out.push((commit_id, info));
                 }
                 flags_without_result |= Flags::STALE;
             }
 
-            graph.insert_parents_with_lookup(&commit_id, &mut |parent_id, parent, ex_flags| -> Result<_, Error> {
-                let queue_info = match ex_flags {
-                    Some(ex_flags) => {
-                        if (*ex_flags & flags_without_result) != flags_without_result {
-                            *ex_flags |= flags_without_result;
-                            Some(GenThenTime::try_from(parent)?)
-                        } else {
-                            None
-                        }
+            for parent_id in commit.parents.clone() {
+                graph.get_or_insert_full_commit(parent_id, |parent| {
+                    if (parent.data & flags_without_result) != flags_without_result {
+                        parent.data |= flags_without_result;
+                        queue.insert(GenThenTime::from(&*parent), parent_id);
                     }
-                    None => Some(GenThenTime::try_from(parent)?),
-                };
-                if let Some(info) = queue_info {
-                    queue.insert(info, parent_id);
-                }
-                Ok(flags_without_result)
-            })?;
+                })?;
+            }
         }
 
         Ok(out)
@@ -215,15 +220,12 @@ pub(crate) mod function {
         time: gix_date::SecondsSinceUnixEpoch,
     }
 
-    impl TryFrom<gix_revwalk::graph::LazyCommit<'_, '_>> for GenThenTime {
-        type Error = gix_object::decode::Error;
-
-        fn try_from(commit: LazyCommit<'_, '_>) -> Result<Self, Self::Error> {
-            let (generation, timestamp) = commit.generation_and_timestamp()?;
-            Ok(GenThenTime {
-                generation: generation.unwrap_or(gix_commitgraph::GENERATION_NUMBER_INFINITY),
-                time: timestamp,
-            })
+    impl From<&graph::Commit<Flags>> for GenThenTime {
+        fn from(commit: &graph::Commit<Flags>) -> Self {
+            GenThenTime {
+                generation: commit.generation.unwrap_or(gix_commitgraph::GENERATION_NUMBER_INFINITY),
+                time: commit.commit_time,
+            }
         }
     }
 

gix-revision/tests/merge_base/mod.rs

@@ -17,7 +17,6 @@ mod baseline {
                     .then(|| gix_commitgraph::Graph::from_info_dir(&odb.store_ref().path().join("info")).unwrap());
                 for expected in parse_expectations(&baseline_path)? {
                     let mut graph = gix_revision::Graph::new(&odb, cache.as_ref());
-
                     let actual = merge_base(expected.first, &expected.others, &mut graph)?;
                     assert_eq!(
                         actual,
@@ -27,6 +26,17 @@ mod baseline {
                         input = expected.plain_input
                     );
                 }
+                let mut graph = gix_revision::Graph::new(&odb, cache.as_ref());
+                for expected in parse_expectations(&baseline_path)? {
+                    let actual = merge_base(expected.first, &expected.others, &mut graph)?;
+                    assert_eq!(
+                        actual,
+                        expected.bases,
+                        "sample (reused graph) {file:?}:{input}",
+                        file = baseline_path.with_extension("").file_name(),
+                        input = expected.plain_input
+                    );
+                }
             }
         }
         assert_ne!(count, 0, "there must be at least one baseline");