Merge branch 'integrate-gix-negotiate'

Author: Byron

Diff for: Cargo.lock

@@ -233,6 +233,7 @@ Check out the [performance discussion][gix-traverse-performance] as well.
   * [x] nested traversal
 * **commits**
   * [x] ancestor graph traversal similar to `git revlog`
+  * [ ] `commitgraph` support
 * [x] API documentation
     * [ ] Examples
 
@@ -551,6 +552,7 @@ The git staging area.
 
 * [x] read-only access
     * [x] Graph lookup of commit information to obtain timestamps, generation and parents, and extra edges
+    * [ ] [Corrected generation dates](https://github.com/git/git/commit/e8b63005c48696a26f976f5f9b0ccaf1983e439d)
     * [ ] Bloom filter index
     * [ ] Bloom filter data
 * [ ] create and update graphs and graph files
@@ -669,8 +671,11 @@ See its [README.md](https://github.com/Byron/gitoxide/blob/main/gix-lock/README.
         * [x] fetch
            * [x] shallow (remains shallow, options to adjust shallow boundary)
            * [ ] a way to auto-explode small packs to avoid them to pile up
-           * [ ] 'ref-in-want'
-           * [ ] standard negotiation algorithms (right now we only have a 'naive' one)
+           * [x] 'ref-in-want'
+           * [ ] 'wanted-ref'
+           * [ ] standard negotiation algorithms `consecutive`, `skipping` and `noop`.
+           * [ ] a more efficient way to deal [with common `have`](https://github.com/git/git/blob/9e49351c3060e1fa6e0d2de64505b7becf157f28/fetch-pack.c#L525)
+                 during negotiation - we would submit known non-common `haves` each round in stateless transports whereas git prunes the set to known common ones.
         * [ ] push
         * [x] ls-refs
         * [x] ls-refs with ref-spec filter

Diff for: crate-status.md

@@ -100,7 +100,7 @@ pub(crate) mod function {
                     });
 
                     let stdout = std::io::BufReader::new(child.stdout.take().expect("we configured it"));
-                    let mut lines = stdout.lines().filter_map(Result::ok).peekable();
+                    let mut lines = stdout.lines().map_while(Result::ok).peekable();
                     while let Some(baseline) = parse_attributes(&mut lines) {
                         if tx_base.send(baseline).is_err() {
                             child.kill().ok();

Diff for: gitoxide-core/src/repository/attributes/validate_baseline.rs

@@ -92,12 +92,24 @@ pub(crate) mod function {
                 writeln!(err, "The cloned repository appears to be empty")?;
             }
             Status::DryRun { .. } => unreachable!("dry-run unsupported"),
-            Status::Change { update_refs, .. } => {
+            Status::Change {
+                update_refs,
+                negotiation_rounds,
+                ..
+            } => {
                 let remote = repo
                     .find_default_remote(gix::remote::Direction::Fetch)
                     .expect("one origin remote")?;
                 let ref_specs = remote.refspecs(gix::remote::Direction::Fetch);
-                print_updates(&repo, update_refs, ref_specs, fetch_outcome.ref_map, &mut out, &mut err)?;
+                print_updates(
+                    &repo,
+                    negotiation_rounds,
+                    update_refs,
+                    ref_specs,
+                    fetch_outcome.ref_map,
+                    &mut out,
+                    &mut err,
+                )?;
             }
         };
 

Diff for: gitoxide-core/src/repository/clone.rs

@@ -63,14 +63,34 @@ pub(crate) mod function {
         let ref_specs = remote.refspecs(gix::remote::Direction::Fetch);
         match res.status {
             Status::NoPackReceived { update_refs } => {
-                print_updates(&repo, update_refs, ref_specs, res.ref_map, &mut out, err)
+                print_updates(&repo, 1, update_refs, ref_specs, res.ref_map, &mut out, err)
             }
-            Status::DryRun { update_refs } => print_updates(&repo, update_refs, ref_specs, res.ref_map, &mut out, err),
+            Status::DryRun {
+                update_refs,
+                negotiation_rounds,
+            } => print_updates(
+                &repo,
+                negotiation_rounds,
+                update_refs,
+                ref_specs,
+                res.ref_map,
+                &mut out,
+                err,
+            ),
             Status::Change {
                 update_refs,
                 write_pack_bundle,
+                negotiation_rounds,
             } => {
-                print_updates(&repo, update_refs, ref_specs, res.ref_map, &mut out, err)?;
+                print_updates(
+                    &repo,
+                    negotiation_rounds,
+                    update_refs,
+                    ref_specs,
+                    res.ref_map,
+                    &mut out,
+                    err,
+                )?;
                 if let Some(data_path) = write_pack_bundle.data_path {
                     writeln!(out, "pack  file: \"{}\"", data_path.display()).ok();
                 }
@@ -88,6 +108,7 @@ pub(crate) mod function {
 
     pub(crate) fn print_updates(
         repo: &gix::Repository,
+        negotiation_rounds: usize,
         update_refs: gix::remote::fetch::refs::update::Outcome,
         refspecs: &[gix::refspec::RefSpec],
         mut map: gix::remote::fetch::RefMap,
@@ -191,6 +212,9 @@ pub(crate) mod function {
                 refspecs.len()
             )?;
         }
+        if negotiation_rounds != 1 {
+            writeln!(err, "needed {negotiation_rounds} rounds of pack-negotiation")?;
+        }
         Ok(())
     }
 }

Diff for: gitoxide-core/src/repository/fetch.rs

@@ -3,7 +3,7 @@ use gix_actor::Identity;
 
 #[test]
 fn round_trip() -> gix_testtools::Result {
-    static DEFAULTS: &'static [&'static [u8]] =     &[
+    static DEFAULTS: &[&[u8]] =     &[
         b"Sebastian Thiel <[email protected]>",
         ".. ☺️Sebastian 王知明 Thiel🙌 .. <[email protected]>".as_bytes(),
         b".. whitespace  \t  is explicitly allowed    - unicode aware trimming must be done elsewhere  <[email protected]>"

Diff for: gix-actor/tests/identity/mod.rs

@@ -64,7 +64,7 @@ fn trim() {
 
 #[test]
 fn round_trip() -> Result<(), Box<dyn std::error::Error>> {
-    static DEFAULTS: &'static [&'static [u8]] =     &[
+    static DEFAULTS: &[&[u8]] =     &[
         b"Sebastian Thiel <[email protected]> 1 -0030",
         ".. ☺️Sebastian 王知明 Thiel🙌 .. <[email protected]> 1528473343 +0230".as_bytes(),
         b".. whitespace  \t  is explicitly allowed    - unicode aware trimming must be done elsewhere  <[email protected]> 1528473343 +0230"

Diff for: gix-actor/tests/signature/mod.rs

@@ -51,6 +51,9 @@ impl<'a> Commit<'a> {
             root_tree_id: gix_hash::oid::from_bytes_unchecked(&bytes[..file.hash_len]),
             parent1: ParentEdge::from_raw(read_u32(&bytes[file.hash_len..][..4])),
             parent2: ParentEdge::from_raw(read_u32(&bytes[file.hash_len + 4..][..4])),
+            // TODO: Add support for corrected commit date offset overflow.
+            //      See https://github.com/git/git/commit/e8b63005c48696a26f976f5f9b0ccaf1983e439d and
+            //          https://github.com/git/git/commit/f90fca638e99a031dce8e3aca72427b2f9b4bb38 for more details and hints at a test.
             generation: read_u32(&bytes[file.hash_len + 8..][..4]) >> 2,
             commit_timestamp: u64::from_be_bytes(bytes[file.hash_len + 8..][..8].try_into().unwrap())
                 & 0x0003_ffff_ffff,

Diff for: gix-commitgraph/src/file/commit.rs

@@ -49,7 +49,7 @@ mod io {
         writer.write_all(b"b\nc\n").expect("success");
         drop(writer);
         assert_eq!(
-            reader.lines().flat_map(Result::ok).collect::<Vec<_>>(),
+            reader.lines().map_while(Result::ok).collect::<Vec<_>>(),
             vec!["a", "b", "c"]
         )
     }

Diff for: gix-features/tests/pipe.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)
     }
 }