feat: allow graph sharing by unifying Flags type.

This makes the graph used in `gix-negotiate` shareable by callers, which can
do their own traversal and store their own flags. The knowlege of this traversal
can be kept using such shared flags, like the `PARSED` bit which should be set whenever
parents are traversed.

That way we are able to emulate the algorithms git uses perfectly, as we keep exactly the
same state.

Author: Byron

gix-negotiate/src/consecutive.rs

@@ -1,30 +1,15 @@
-use crate::{Error, Negotiator};
+use crate::{Error, Flags, Negotiator};
 use gix_hash::ObjectId;
 use gix_revision::graph::CommitterTimestamp;
-use smallvec::SmallVec;
-bitflags::bitflags! {
-    /// Whether something can be read or written.
-    #[derive(Debug, Default, Copy, Clone)]
-    pub struct Flags: u8 {
-        /// The revision is known to be in common with the remote.
-        const COMMON = 1 << 0;
-        /// The revision is common and was set by merit of a remote tracking ref (e.g. `refs/heads/origin/main`).
-        const COMMON_REF = 1 << 1;
-        /// The revision has entered the priority queue.
-        const SEEN = 1 << 2;
-        /// The revision was popped off our primary priority queue, used to avoid double-counting of `non_common_revs`
-        const POPPED = 1 << 3;
-    }
-}
 
 pub(crate) struct Algorithm<'find> {
-    graph: gix_revision::Graph<'find, Flags>,
+    graph: crate::Graph<'find>,
     revs: gix_revision::PriorityQueue<CommitterTimestamp, ObjectId>,
     non_common_revs: usize,
 }
 
 impl<'a> Algorithm<'a> {
-    pub fn new(graph: gix_revision::Graph<'a, Flags>) -> Self {
+    pub fn new(graph: crate::Graph<'a>) -> Self {
         Self {
             graph,
             revs: gix_revision::PriorityQueue::new(),
@@ -35,15 +20,17 @@ impl<'a> Algorithm<'a> {
     /// Add `id` to our priority queue and *add* `flags` to it.
     fn add_to_queue(&mut self, id: ObjectId, mark: Flags) -> Result<(), Error> {
         let mut is_common = false;
-        if self.graph.get(&id).map_or(false, |flags| flags.intersects(mark)) {
-            return Ok(());
-        }
-        let commit = self.graph.try_lookup_and_insert(id, |current| {
-            *current |= mark;
-            is_common = current.contains(Flags::COMMON);
-        })?;
-        if let Some(timestamp) = commit.map(|c| c.committer_timestamp()).transpose()? {
-            self.revs.insert(timestamp, id);
+        let mut has_mark = false;
+        if let Some(commit) = self
+            .graph
+            .try_lookup_or_insert_commit(id, |data| {
+                has_mark = data.flags.intersects(mark);
+                data.flags |= mark;
+                is_common = data.flags.contains(Flags::COMMON);
+            })?
+            .filter(|_| !has_mark)
+        {
+            self.revs.insert(commit.commit_time, id);
             if !is_common {
                 self.non_common_revs += 1;
             }
@@ -55,39 +42,39 @@ impl<'a> Algorithm<'a> {
         let mut is_common = false;
         if let Some(commit) = self
             .graph
-            .try_lookup_and_insert(id, |current| is_common = current.contains(Flags::COMMON))?
+            .try_lookup_or_insert_commit(id, |data| is_common = data.flags.contains(Flags::COMMON))?
             .filter(|_| !is_common)
         {
-            let mut queue =
-                gix_revision::PriorityQueue::from_iter(Some((commit.committer_timestamp()?, (id, 0_usize))));
+            let mut queue = gix_revision::PriorityQueue::from_iter(Some((commit.commit_time, (id, 0_usize))));
             if let Mark::ThisCommitAndAncestors = mode {
-                let current = self.graph.get_mut(&id).expect("just inserted");
-                *current |= Flags::COMMON;
-                if current.contains(Flags::SEEN) && !current.contains(Flags::POPPED) {
+                commit.data.flags |= Flags::COMMON;
+                if commit.data.flags.contains(Flags::SEEN) && !commit.data.flags.contains(Flags::POPPED) {
                     self.non_common_revs -= 1;
                 }
             }
-            let mut parents = SmallVec::new();
             while let Some((id, generation)) = queue.pop() {
-                if self.graph.get(&id).map_or(true, |d| !d.contains(Flags::SEEN)) {
+                if self
+                    .graph
+                    .get(&id)
+                    .map_or(true, |commit| !commit.data.flags.contains(Flags::SEEN))
+                {
                     self.add_to_queue(id, Flags::SEEN)?;
                 } else if matches!(ancestors, Ancestors::AllUnseen) || generation < 2 {
-                    if let Some(commit) = self.graph.try_lookup_and_insert(id, |_| {})? {
-                        collect_parents(commit.iter_parents(), &mut parents)?;
-                        for parent_id in parents.drain(..) {
+                    if let Some(commit) = self.graph.try_lookup_or_insert_commit(id, |_| {})? {
+                        for parent_id in commit.parents.clone() {
                             let mut prev_flags = Flags::default();
                             if let Some(parent) = self
                                 .graph
-                                .try_lookup_and_insert(parent_id, |d| {
-                                    prev_flags = *d;
-                                    *d |= Flags::COMMON;
+                                .try_lookup_or_insert_commit(parent_id, |data| {
+                                    prev_flags = data.flags;
+                                    data.flags |= Flags::COMMON;
                                 })?
                                 .filter(|_| !prev_flags.contains(Flags::COMMON))
                             {
                                 if prev_flags.contains(Flags::SEEN) && !prev_flags.contains(Flags::POPPED) {
                                     self.non_common_revs -= 1;
                                 }
-                                queue.insert(parent.committer_timestamp()?, (parent_id, generation + 1))
+                                queue.insert(parent.commit_time, (parent_id, generation + 1))
                             }
                         }
                     }
@@ -98,23 +85,13 @@ impl<'a> Algorithm<'a> {
     }
 }
 
-pub(crate) fn collect_parents(
-    parents: gix_revision::graph::commit::Parents<'_>,
-    out: &mut SmallVec<[ObjectId; 2]>,
-) -> Result<(), Error> {
-    out.clear();
-    for parent in parents {
-        out.push(parent.map_err(|err| match err {
-            gix_revision::graph::commit::iter_parents::Error::DecodeCommit(err) => Error::DecodeCommit(err),
-            gix_revision::graph::commit::iter_parents::Error::DecodeCommitGraph(err) => Error::DecodeCommitInGraph(err),
-        })?);
-    }
-    Ok(())
-}
-
 impl<'a> Negotiator for Algorithm<'a> {
     fn known_common(&mut self, id: ObjectId) -> Result<(), Error> {
-        if self.graph.get(&id).map_or(true, |d| !d.contains(Flags::SEEN)) {
+        if self
+            .graph
+            .get(&id)
+            .map_or(true, |commit| !commit.data.flags.contains(Flags::SEEN))
+        {
             self.add_to_queue(id, Flags::COMMON_REF | Flags::SEEN)?;
             self.mark_common(id, Mark::AncestorsOnly, Ancestors::DirectUnseen)?;
         }
@@ -126,10 +103,10 @@ impl<'a> Negotiator for Algorithm<'a> {
     }
 
     fn next_have(&mut self) -> Option<Result<ObjectId, Error>> {
-        let mut parents = SmallVec::new();
         loop {
             let id = self.revs.pop().filter(|_| self.non_common_revs != 0)?;
-            let flags = self.graph.get_mut(&id).expect("it was added to the graph by now");
+            let commit = self.graph.get_mut(&id).expect("it was added to the graph by now");
+            let flags = &mut commit.data.flags;
             *flags |= Flags::POPPED;
 
             if !flags.contains(Flags::COMMON) {
@@ -144,15 +121,12 @@ impl<'a> Negotiator for Algorithm<'a> {
                 (Some(id), Flags::SEEN)
             };
 
-            let commit = match self.graph.try_lookup(&id) {
-                Ok(c) => c.expect("it was found before, must still be there"),
-                Err(err) => return Some(Err(err.into())),
-            };
-            if let Err(err) = collect_parents(commit.iter_parents(), &mut parents) {
-                return Some(Err(err));
-            }
-            for parent_id in parents.drain(..) {
-                if self.graph.get(&parent_id).map_or(true, |d| !d.contains(Flags::SEEN)) {
+            for parent_id in commit.parents.clone() {
+                if self
+                    .graph
+                    .get(&parent_id)
+                    .map_or(true, |commit| !commit.data.flags.contains(Flags::SEEN))
+                {
                     if let Err(err) = self.add_to_queue(parent_id, mark) {
                         return Some(Err(err));
                     }
@@ -171,7 +145,10 @@ impl<'a> Negotiator for Algorithm<'a> {
     }
 
     fn in_common_with_remote(&mut self, id: ObjectId) -> Result<bool, Error> {
-        let known_to_be_common = self.graph.get(&id).map_or(false, |d| d.contains(Flags::COMMON));
+        let known_to_be_common = self
+            .graph
+            .get(&id)
+            .map_or(false, |commit| commit.data.flags.contains(Flags::COMMON));
         self.mark_common(id, Mark::ThisCommitAndAncestors, Ancestors::DirectUnseen)?;
         Ok(known_to_be_common)
     }

gix-negotiate/src/lib.rs

@@ -2,11 +2,65 @@
 //! the pack it sends to only contain what we don't have.
 #![deny(rust_2018_idioms, missing_docs)]
 #![forbid(unsafe_code)]
-
 mod consecutive;
 mod noop;
 mod skipping;
 
+bitflags::bitflags! {
+    /// Multi purpose, shared flags that are used by negotiation algorithms and by the caller as well
+    ///
+    /// However, in this crate we can't implement the calling side, so we marry this type to whatever is needed in downstream crates.
+    #[derive(Debug, Default, Copy, Clone)]
+    pub struct Flags: u8 {
+        /// The object was already handled and doesn't need to be looked at again.
+        const COMPLETE = 1 << 0;
+        /// A commit from an alternate object database.
+        const ALTERNATE = 1 << 1;
+
+        /// The revision is known to be in common with the remote.
+        ///
+        /// Used by `consecutive` and `skipping`.
+        const COMMON = 1 << 2;
+        /// The revision has entered the priority queue.
+        ///
+        /// Used by `consecutive` and `skipping`.
+        const SEEN = 1 << 3;
+        /// The revision was popped off our primary priority queue, used to avoid double-counting of `non_common_revs`
+        ///
+        /// Used by `consecutive` and `skipping`.
+        const POPPED = 1 << 4;
+
+        /// The revision is common and was set by merit of a remote tracking ref (e.g. `refs/heads/origin/main`).
+        ///
+        /// Used by `consecutive`.
+        const COMMON_REF = 1 << 5;
+
+        /// The remote let us know it has the object. We still have to tell the server we have this object or one of its descendants.
+        /// We won't tell the server about its ancestors.
+        ///
+        /// Used by `skipping`.
+        const ADVERTISED = 1 << 6;
+    }
+}
+
+/// Additional data to store with each commit when used by any of our algorithms.
+///
+/// It's shared among those who use the [`Negotiator`] trait, and all implementations of it.
+#[derive(Default, Copy, Clone)]
+pub struct Metadata {
+    /// Used by `skipping`.
+    /// Only used if commit is not COMMON
+    pub(crate) original_ttl: u16,
+    /// Used by `skipping`.
+    pub(crate) ttl: u16,
+
+    /// Additional information about each commit
+    pub(crate) flags: Flags,
+}
+
+/// The graph our callers use to store traversal information, for (re-)use in the negotiation implementation.
+pub type Graph<'find> = gix_revision::Graph<'find, gix_revision::graph::Commit<Metadata>>;
+
 /// The way the negotiation is performed.
 #[derive(Default, Debug, Copy, Clone, Eq, PartialEq)]
 pub enum Algorithm {
@@ -57,11 +111,11 @@ impl Algorithm {
         match &self {
             Algorithm::Noop => Box::new(noop::Noop) as Box<dyn Negotiator>,
             Algorithm::Consecutive => {
-                let graph = gix_revision::Graph::<'_, consecutive::Flags>::new(find, cache);
+                let graph = Graph::new(find, cache);
                 Box::new(consecutive::Algorithm::new(graph))
             }
             Algorithm::Skipping => {
-                let graph = gix_revision::Graph::<'_, skipping::Entry>::new(find, cache);
+                let graph = Graph::new(find, cache);
                 Box::new(skipping::Algorithm::new(graph))
             }
         }
@@ -101,4 +155,6 @@ pub enum Error {
     DecodeCommitInGraph(#[from] gix_commitgraph::file::commit::Error),
     #[error(transparent)]
     LookupCommitInGraph(#[from] gix_revision::graph::lookup::Error),
+    #[error(transparent)]
+    LookupCommitInGraphTBD(#[from] gix_revision::graph::lookup::commit::Error), // TODO: should replace most of the above
 }