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,93 +1,82 @@
-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>,
+pub(crate) struct Algorithm {
     revs: gix_revision::PriorityQueue<CommitterTimestamp, ObjectId>,
     non_common_revs: usize,
 }
 
-impl<'a> Algorithm<'a> {
-    pub fn new(graph: gix_revision::Graph<'a, Flags>) -> Self {
+impl Default for Algorithm {
+    fn default() -> Self {
         Self {
-            graph,
             revs: gix_revision::PriorityQueue::new(),
             non_common_revs: 0,
         }
     }
+}
 
+impl Algorithm {
     /// Add `id` to our priority queue and *add* `flags` to it.
-    fn add_to_queue(&mut self, id: ObjectId, mark: Flags) -> Result<(), Error> {
+    fn add_to_queue(&mut self, id: ObjectId, mark: Flags, graph: &mut crate::Graph<'_>) -> 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) = 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;
             }
         }
         Ok(())
     }
 
-    fn mark_common(&mut self, id: ObjectId, mode: Mark, ancestors: Ancestors) -> Result<(), Error> {
+    fn mark_common(
+        &mut self,
+        id: ObjectId,
+        mode: Mark,
+        ancestors: Ancestors,
+        graph: &mut crate::Graph<'_>,
+    ) -> Result<(), Error> {
         let mut is_common = false;
-        if let Some(commit) = self
-            .graph
-            .try_lookup_and_insert(id, |current| is_common = current.contains(Flags::COMMON))?
+        if let Some(commit) = graph
+            .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)) {
-                    self.add_to_queue(id, Flags::SEEN)?;
+                if graph
+                    .get(&id)
+                    .map_or(true, |commit| !commit.data.flags.contains(Flags::SEEN))
+                {
+                    self.add_to_queue(id, Flags::SEEN, graph)?;
                 } 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) = 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;
+                            if let Some(parent) = graph
+                                .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,38 +87,27 @@ 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)) {
-            self.add_to_queue(id, Flags::COMMON_REF | Flags::SEEN)?;
-            self.mark_common(id, Mark::AncestorsOnly, Ancestors::DirectUnseen)?;
+impl Negotiator for Algorithm {
+    fn known_common(&mut self, id: ObjectId, graph: &mut crate::Graph<'_>) -> Result<(), Error> {
+        if graph
+            .get(&id)
+            .map_or(true, |commit| !commit.data.flags.contains(Flags::SEEN))
+        {
+            self.add_to_queue(id, Flags::COMMON_REF | Flags::SEEN, graph)?;
+            self.mark_common(id, Mark::AncestorsOnly, Ancestors::DirectUnseen, graph)?;
         }
         Ok(())
     }
 
-    fn add_tip(&mut self, id: ObjectId) -> Result<(), Error> {
-        self.add_to_queue(id, Flags::SEEN)
+    fn add_tip(&mut self, id: ObjectId, graph: &mut crate::Graph<'_>) -> Result<(), Error> {
+        self.add_to_queue(id, Flags::SEEN, graph)
     }
 
-    fn next_have(&mut self) -> Option<Result<ObjectId, Error>> {
-        let mut parents = SmallVec::new();
+    fn next_have(&mut self, graph: &mut crate::Graph<'_>) -> Option<Result<ObjectId, Error>> {
         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 = 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,21 +122,17 @@ 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)) {
-                    if let Err(err) = self.add_to_queue(parent_id, mark) {
+            for parent_id in commit.parents.clone() {
+                if 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, graph) {
                         return Some(Err(err));
                     }
                 }
                 if mark.contains(Flags::COMMON) {
-                    if let Err(err) = self.mark_common(parent_id, Mark::AncestorsOnly, Ancestors::AllUnseen) {
+                    if let Err(err) = self.mark_common(parent_id, Mark::AncestorsOnly, Ancestors::AllUnseen, graph) {
                         return Some(Err(err));
                     }
                 }
@@ -170,9 +144,11 @@ 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));
-        self.mark_common(id, Mark::ThisCommitAndAncestors, Ancestors::DirectUnseen)?;
+    fn in_common_with_remote(&mut self, id: ObjectId, graph: &mut crate::Graph<'_>) -> Result<bool, Error> {
+        let known_to_be_common = graph
+            .get(&id)
+            .map_or(false, |commit| commit.data.flags.contains(Flags::COMMON));
+        self.mark_common(id, Mark::ThisCommitAndAncestors, Ancestors::DirectUnseen, graph)?;
         Ok(known_to_be_common)
     }
 }

gix-negotiate/src/lib.rs

@@ -2,11 +2,64 @@
 //! 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 is already available locally and doesn't need to be fetched by the remote.
+        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 original_ttl: u16,
+    /// Used by `skipping`.
+    pub ttl: u16,
+    /// Additional information about each commit
+    pub 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 {
@@ -19,6 +72,17 @@ pub enum Algorithm {
     Skipping,
 }
 
+impl std::fmt::Display for Algorithm {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Algorithm::Noop => "noop",
+            Algorithm::Consecutive => "consecutive",
+            Algorithm::Skipping => "skipping",
+        }
+        .fmt(f)
+    }
+}
+
 /// Calculate how many `HAVE` lines we may send in one round, with variation depending on whether the `transport_is_stateless` or not.
 /// `window_size` is the previous (or initial) value of the window size.
 pub fn window_size(transport_is_stateless: bool, window_size: impl Into<Option<usize>>) -> usize {
@@ -44,26 +108,11 @@ pub fn window_size(transport_is_stateless: bool, window_size: impl Into<Option<u
 
 impl Algorithm {
     /// Create an instance of a negotiator which implements this algorithm.
-    pub fn into_negotiator<'find, Find, E>(
-        self,
-        find: Find,
-        cache: impl Into<Option<gix_commitgraph::Graph>>,
-    ) -> Box<dyn Negotiator + 'find>
-    where
-        Find:
-            for<'a> FnMut(&gix_hash::oid, &'a mut Vec<u8>) -> Result<Option<gix_object::CommitRefIter<'a>>, E> + 'find,
-        E: std::error::Error + Send + Sync + 'static,
-    {
+    pub fn into_negotiator(self) -> Box<dyn Negotiator> {
         match &self {
             Algorithm::Noop => Box::new(noop::Noop) as Box<dyn Negotiator>,
-            Algorithm::Consecutive => {
-                let graph = gix_revision::Graph::<'_, consecutive::Flags>::new(find, cache);
-                Box::new(consecutive::Algorithm::new(graph))
-            }
-            Algorithm::Skipping => {
-                let graph = gix_revision::Graph::<'_, skipping::Entry>::new(find, cache);
-                Box::new(skipping::Algorithm::new(graph))
-            }
+            Algorithm::Consecutive => Box::<consecutive::Algorithm>::default(),
+            Algorithm::Skipping => Box::<skipping::Algorithm>::default(),
         }
     }
 }
@@ -73,32 +122,23 @@ pub trait Negotiator {
     /// Mark `id` as common between the remote and us.
     ///
     /// These ids are typically the local tips of remote tracking branches.
-    fn known_common(&mut self, id: gix_hash::ObjectId) -> Result<(), Error>;
+    fn known_common(&mut self, id: gix_hash::ObjectId, graph: &mut Graph<'_>) -> Result<(), Error>;
 
     /// Add `id` as starting point of a traversal across commits that aren't necessarily common between the remote and us.
     ///
     /// These tips are usually the commits of local references whose tips should lead to objects that we have in common with the remote.
-    fn add_tip(&mut self, id: gix_hash::ObjectId) -> Result<(), Error>;
+    fn add_tip(&mut self, id: gix_hash::ObjectId, graph: &mut Graph<'_>) -> Result<(), Error>;
 
     /// Produce the next id of an object that we want the server to know we have. It's an object we don't know we have in common or not.
     ///
     /// Returns `None` if we have exhausted all options, which might mean we have traversed the entire commit graph.
-    fn next_have(&mut self) -> Option<Result<gix_hash::ObjectId, Error>>;
+    fn next_have(&mut self, graph: &mut Graph<'_>) -> Option<Result<gix_hash::ObjectId, Error>>;
 
     /// Mark `id` as being common with the remote (as informed by the remote itself) and return `true` if we knew it was common already.
     ///
     /// We can assume to have already seen `id` as we were the one to inform the remote in a prior `have`.
-    fn in_common_with_remote(&mut self, id: gix_hash::ObjectId) -> Result<bool, Error>;
+    fn in_common_with_remote(&mut self, id: gix_hash::ObjectId, graph: &mut Graph<'_>) -> Result<bool, Error>;
 }
 
 /// An error that happened during any of the methods on a [`Negotiator`].
-#[derive(Debug, thiserror::Error)]
-#[allow(missing_docs)]
-pub enum Error {
-    #[error(transparent)]
-    DecodeCommit(#[from] gix_object::decode::Error),
-    #[error(transparent)]
-    DecodeCommitInGraph(#[from] gix_commitgraph::file::commit::Error),
-    #[error(transparent)]
-    LookupCommitInGraph(#[from] gix_revision::graph::lookup::Error),
-}
+pub type Error = gix_revision::graph::lookup::commit::Error;