feat: use gix-negotiate in fetch machinery.

Thanks to it we are finally able to do pack negotiations just like git can,
as many rounds as it takes and with all available algorithms.

Works for V1 and V2 and for stateless and stateful transports.

Author: Byron

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
 
@@ -670,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

crate-status.md

@@ -131,6 +131,7 @@ gix-object = { version = "^0.29.2", path = "../gix-object" }
 gix-actor = { version = "^0.20.0", path = "../gix-actor" }
 gix-pack = { version = "^0.35.0", path = "../gix-pack", features = ["object-cache-dynamic"] }
 gix-revision = { version = "^0.14.0", path = "../gix-revision" }
+gix-negotiate = { version = "0.1.0", path = "../gix-negotiate" }
 
 gix-path = { version = "^0.8.0", path = "../gix-path" }
 gix-url = { version = "^0.18.0", path = "../gix-url" }

gix/Cargo.toml

@@ -44,7 +44,12 @@ impl PrepareFetch {
     /// it was newly initialized.
     ///
     /// Note that all data we created will be removed once this instance drops if the operation wasn't successful.
-    pub fn fetch_only<P>(
+    ///
+    /// ### Note for users of `async`
+    ///
+    /// Even though
+    #[gix_protocol::maybe_async::maybe_async]
+    pub async fn fetch_only<P>(
         &mut self,
         mut progress: P,
         should_interrupt: &std::sync::atomic::AtomicBool,
@@ -101,17 +106,19 @@ impl PrepareFetch {
         .expect("valid")
         .to_owned();
         let pending_pack: remote::fetch::Prepare<'_, '_, _> = {
-            let mut connection = remote.connect(remote::Direction::Fetch)?;
+            let mut connection = remote.connect(remote::Direction::Fetch).await?;
             if let Some(f) = self.configure_connection.as_mut() {
                 f(&mut connection).map_err(|err| Error::RemoteConnection(err))?;
             }
-            connection.prepare_fetch(&mut progress, {
-                let mut opts = self.fetch_options.clone();
-                if !opts.extra_refspecs.contains(&head_refspec) {
-                    opts.extra_refspecs.push(head_refspec)
-                }
-                opts
-            })?
+            connection
+                .prepare_fetch(&mut progress, {
+                    let mut opts = self.fetch_options.clone();
+                    if !opts.extra_refspecs.contains(&head_refspec) {
+                        opts.extra_refspecs.push(head_refspec)
+                    }
+                    opts
+                })
+                .await?
         };
         if pending_pack.ref_map().object_hash != repo.object_hash() {
             unimplemented!("configure repository to expect a different object hash as advertised by the server")
@@ -127,7 +134,8 @@ impl PrepareFetch {
                 message: reflog_message.clone(),
             })
             .with_shallow(self.shallow.clone())
-            .receive(progress, should_interrupt)?;
+            .receive(progress, should_interrupt)
+            .await?;
 
         util::append_config_to_repo_config(repo, config);
         util::update_head(
@@ -141,6 +149,7 @@ impl PrepareFetch {
     }
 
     /// Similar to [`fetch_only()`][Self::fetch_only()`], but passes ownership to a utility type to configure a checkout operation.
+    #[cfg(feature = "blocking-network-client")]
     pub fn fetch_then_checkout<P>(
         &mut self,
         progress: P,
@@ -155,5 +164,4 @@ impl PrepareFetch {
     }
 }
 
-#[cfg(feature = "blocking-network-client")]
 mod util;

gix/src/clone/fetch/mod.rs

@@ -160,7 +160,7 @@ mod access_feat {
 }
 
 ///
-#[cfg(feature = "blocking-network-client")]
+#[cfg(any(feature = "async-network-client-async-std", feature = "blocking-network-client"))]
 pub mod fetch;
 
 ///

gix/src/clone/mod.rs

@@ -511,7 +511,8 @@ pub mod validate {
                 gix_config::Integer::try_from(value)?
                     .to_decimal()
                     .ok_or_else(|| format!("integer {value} cannot be represented as `usize`"))?,
-            )?;
+            )
+            .map_err(|_| "cannot use sign for unsigned integer")?;
             Ok(())
         }
     }

gix/src/config/tree/keys.rs

@@ -6,6 +6,8 @@ use crate::{
 impl Protocol {
     /// The `protocol.allow` key.
     pub const ALLOW: Allow = Allow::new_with_validate("allow", &config::Tree::PROTOCOL, validate::Allow);
+    /// The `protocol.version` key.
+    pub const VERSION: Version = Version::new_with_validate("version", &config::Tree::PROTOCOL, validate::Version);
 
     /// The `protocol.<name>` subsection
     pub const NAME_PARAMETER: NameParameter = NameParameter;
@@ -14,6 +16,9 @@ impl Protocol {
 /// The `protocol.allow` key type.
 pub type Allow = keys::Any<validate::Allow>;
 
+/// The `protocol.version` key.
+pub type Version = keys::Any<validate::Version>;
+
 #[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))]
 mod allow {
     use std::borrow::Cow;
@@ -39,7 +44,7 @@ mod allow {
 pub struct NameParameter;
 
 impl NameParameter {
-    /// The `credential.<url>.helper` key.
+    /// The `protocol.<name>.allow` key.
     pub const ALLOW: Allow = Allow::new_with_validate("allow", &Protocol::NAME_PARAMETER, validate::Allow);
 }
 
@@ -63,14 +68,46 @@ impl Section for Protocol {
     }
 
     fn keys(&self) -> &[&dyn Key] {
-        &[&Self::ALLOW]
+        &[&Self::ALLOW, &Self::VERSION]
     }
 
     fn sub_sections(&self) -> &[&dyn Section] {
         &[&Self::NAME_PARAMETER]
     }
 }
 
+mod key_impls {
+    impl super::Version {
+        /// Convert `value` into the corresponding protocol version, possibly applying the correct default.
+        #[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))]
+        pub fn try_into_protocol_version(
+            &'static self,
+            value: Option<Result<i64, gix_config::value::Error>>,
+        ) -> Result<gix_protocol::transport::Protocol, crate::config::key::GenericErrorWithValue> {
+            let value = match value {
+                None => return Ok(gix_protocol::transport::Protocol::V2),
+                Some(v) => v,
+            };
+            Ok(match value {
+                Ok(0) => gix_protocol::transport::Protocol::V0,
+                Ok(1) => gix_protocol::transport::Protocol::V1,
+                Ok(2) => gix_protocol::transport::Protocol::V2,
+                Ok(other) => {
+                    return Err(crate::config::key::GenericErrorWithValue::from_value(
+                        self,
+                        other.to_string().into(),
+                    ))
+                }
+                Err(err) => {
+                    return Err(
+                        crate::config::key::GenericErrorWithValue::from_value(self, "unknown".into()).with_source(err),
+                    )
+                }
+            })
+        }
+    }
+}
+
 mod validate {
     use crate::{bstr::BStr, config::tree::keys};
 
@@ -82,4 +119,17 @@ mod validate {
             Ok(())
         }
     }
+
+    pub struct Version;
+    impl keys::Validate for Version {
+        fn validate(&self, value: &BStr) -> Result<(), Box<dyn std::error::Error + Send + Sync + 'static>> {
+            let value = gix_config::Integer::try_from(value)?
+                .to_decimal()
+                .ok_or_else(|| format!("integer {value} cannot be represented as integer"))?;
+            match value {
+                0 | 1 | 2 => Ok(()),
+                _ => Err(format!("protocol version {value} is unknown").into()),
+            }
+        }
+    }
 }

gix/src/config/tree/sections/protocol.rs

@@ -81,6 +81,7 @@ pub use gix_ignore as ignore;
 #[doc(inline)]
 pub use gix_index as index;
 pub use gix_lock as lock;
+pub use gix_negotiate as negotiate;
 pub use gix_object as objs;
 pub use gix_object::bstr;
 pub use gix_odb as odb;

gix/src/lib.rs

@@ -24,8 +24,8 @@ mod error {
         Connect(#[from] gix_protocol::transport::client::connect::Error),
         #[error("The {} url was missing - don't know where to establish a connection to", direction.as_str())]
         MissingUrl { direction: remote::Direction },
-        #[error("Protocol named {given:?} is not a valid protocol. Choose between 1 and 2")]
-        UnknownProtocol { given: BString },
+        #[error("The given protocol version was invalid. Choose between 1 and 2")]
+        UnknownProtocol { source: config::key::GenericErrorWithValue },
         #[error("Could not verify that \"{}\" url is a valid git directory before attempting to use it", url.to_bstring())]
         FileUrl {
             source: Box<gix_discover::is_git::Error>,
@@ -128,25 +128,9 @@ impl<'repo> Remote<'repo> {
             Ok(url)
         }
 
-        use gix_protocol::transport::Protocol;
-        let version = self
-            .repo
-            .config
-            .resolved
-            .integer("protocol", None, "version")
-            .unwrap_or(Ok(2))
-            .map_err(|err| Error::UnknownProtocol { given: err.input })
-            .and_then(|num| {
-                Ok(match num {
-                    1 => Protocol::V1,
-                    2 => Protocol::V2,
-                    num => {
-                        return Err(Error::UnknownProtocol {
-                            given: num.to_string().into(),
-                        })
-                    }
-                })
-            })?;
+        let version = crate::config::tree::Protocol::VERSION
+            .try_into_protocol_version(self.repo.config.resolved.integer("protocol", None, "version"))
+            .map_err(|err| Error::UnknownProtocol { source: err })?;
 
         let url = self.url(direction).ok_or(Error::MissingUrl { direction })?.to_owned();
         if !self.repo.config.url_scheme()?.allow(&url.scheme) {

gix/src/remote/connect.rs

@@ -43,13 +43,17 @@ impl RefLogMessage {
 pub enum Status {
     /// Nothing changed as the remote didn't have anything new compared to our tracking branches, thus no pack was received
     /// and no new object was added.
+    ///
+    /// As we could determine that nothing changed without remote interaction, there was no negotiation at all.
     NoPackReceived {
         /// However, depending on the refspecs, references might have been updated nonetheless to point to objects as
         /// reported by the remote.
         update_refs: refs::update::Outcome,
     },
     /// There was at least one tip with a new object which we received.
     Change {
+        /// The number of rounds it took to minimize the pack to contain only the objects we don't have.
+        negotiation_rounds: usize,
         /// Information collected while writing the pack and its index.
         write_pack_bundle: gix_pack::bundle::write::Outcome,
         /// Information collected while updating references.
@@ -58,6 +62,8 @@ pub enum Status {
     /// A dry run was performed which leaves the local repository without any change
     /// nor will a pack have been received.
     DryRun {
+        /// The number of rounds it took to minimize the *would-be-sent*-pack to contain only the objects we don't have.
+        negotiation_rounds: usize,
         /// Information about what updates to refs would have been done.
         update_refs: refs::update::Outcome,
     },