Merge pull request #2274 from djc/simplify-ls-refs

protocol: split async and blocking APIs for fetching refmaps

Author: Byron

gitoxide-core/src/pack/receive.rs

@@ -87,17 +87,17 @@ where
         fetch_refspecs: fetch_refspecs.clone(),
         extra_refspecs: vec![],
     };
-    let refmap = handshake
-        .fetch_or_extract_refmap(
-            &mut progress,
-            &mut transport.inner,
-            user_agent.clone(),
-            trace_packetlines,
-            true,
-            context,
-        )
+
+    let fetch_refmap = handshake.prepare_lsrefs_or_extract_refmap(user_agent.clone(), true, context)?;
+
+    #[cfg(feature = "async-client")]
+    let refmap = fetch_refmap
+        .fetch_async(&mut progress, &mut transport.inner, trace_packetlines)
         .await?;
 
+    #[cfg(feature = "blocking-client")]
+    let refmap = fetch_refmap.fetch_blocking(&mut progress, &mut transport.inner, trace_packetlines)?;
+
     if refmap.mappings.is_empty() && !refmap.remote_refs.is_empty() {
         return Err(Error::NoMapping {
             refspecs: refmap.refspecs.clone(),

gix-protocol/src/fetch/negotiate.rs

@@ -111,7 +111,7 @@ pub struct Round {
 /// * `graph`
 ///     - The commit-graph for use by the `negotiator` - we populate it with tips to initialize the graph traversal.
 /// * `ref_map`
-///     - The references known on the remote, as previously obtained with [`RefMap::fetch()`].
+///     - The references known on the remote, as previously obtained with [`crate::Handshake::prepare_lsrefs_or_extract_refmap()`].
 /// * `shallow`
 ///     - How to deal with shallow repositories. It does affect how negotiations are performed.
 /// * `mapping_is_ignored`

gix-protocol/src/fetch/refmap/init.rs

@@ -1,13 +1,6 @@
-use std::{borrow::Cow, collections::HashSet};
-
-use bstr::{BString, ByteSlice, ByteVec};
-use gix_features::progress::Progress;
+use bstr::{BString, ByteSlice};
 use gix_transport::client::Capabilities;
 
-#[cfg(feature = "async-client")]
-use crate::transport::client::async_io::Transport;
-#[cfg(feature = "blocking-client")]
-use crate::transport::client::blocking_io::Transport;
 use crate::{
     fetch::{
         refmap::{Mapping, Source, SpecIndex},
@@ -16,7 +9,7 @@ use crate::{
     handshake::Ref,
 };
 
-/// The error returned by [`RefMap::fetch()`].
+/// The error returned by [`crate::Handshake::prepare_lsrefs_or_extract_refmap()`].
 #[derive(Debug, thiserror::Error)]
 #[allow(missing_docs)]
 pub enum Error {
@@ -28,7 +21,7 @@ pub enum Error {
     ListRefs(#[from] crate::ls_refs::Error),
 }
 
-/// For use in [`RefMap::fetch()`].
+/// For use in [`RefMap::from_refs()`].
 #[derive(Debug, Clone)]
 pub struct Context {
     /// All explicit refspecs to identify references on the remote that you are interested in.
@@ -41,58 +34,14 @@ pub struct Context {
 }
 
 impl Context {
-    fn aggregate_refspecs(&self) -> Vec<gix_refspec::RefSpec> {
+    pub(crate) fn aggregate_refspecs(&self) -> Vec<gix_refspec::RefSpec> {
         let mut all_refspecs = self.fetch_refspecs.clone();
         all_refspecs.extend(self.extra_refspecs.iter().cloned());
         all_refspecs
     }
 }
 
 impl RefMap {
-    /// Create a new instance by obtaining all references on the remote that have been filtered through our remote's specs
-    /// for _fetching_.
-    ///
-    /// * `progress` is used if `ls-refs` is invoked on the remote. Always the case when V2 is used.
-    /// * `capabilities` are the capabilities of the server, obtained by a [handshake](crate::handshake()).
-    /// * `transport` is a way to communicate with the server to obtain the reference listing.
-    /// * `user_agent` is passed to the server.
-    /// * `trace_packetlines` traces all packet lines if `true`, for debugging primarily.
-    /// * `prefix_from_spec_as_filter_on_remote`
-    ///     - Use a two-component prefix derived from the ref-spec's source, like `refs/heads/`  to let the server pre-filter refs
-    ///       with great potential for savings in traffic and local CPU time.
-    /// * `context` to provide more [configuration](Context).
-    #[allow(clippy::result_large_err)]
-    #[maybe_async::maybe_async]
-    pub async fn fetch<T>(
-        mut progress: impl Progress,
-        capabilities: &Capabilities,
-        transport: &mut T,
-        user_agent: (&'static str, Option<Cow<'static, str>>),
-        trace_packetlines: bool,
-        prefix_from_spec_as_filter_on_remote: bool,
-        context: Context,
-    ) -> Result<Self, Error>
-    where
-        T: Transport,
-    {
-        let _span = gix_trace::coarse!("gix_protocol::fetch::RefMap::new()");
-        let all_refspecs = context.aggregate_refspecs();
-        let remote_refs = crate::ls_refs(
-            transport,
-            capabilities,
-            |_capabilities, arguments| {
-                push_prefix_arguments(prefix_from_spec_as_filter_on_remote, arguments, &all_refspecs);
-                Ok(crate::ls_refs::Action::Continue)
-            },
-            &mut progress,
-            trace_packetlines,
-            user_agent,
-        )
-        .await?;
-
-        Self::from_refs(remote_refs, capabilities, context)
-    }
-
     /// Create a ref-map from already obtained `remote_refs`. Use `context` to pass in refspecs.
     /// `capabilities` are used to determine the object format.
     pub fn from_refs(remote_refs: Vec<Ref>, capabilities: &Capabilities, context: Context) -> Result<RefMap, Error> {
@@ -161,26 +110,3 @@ impl RefMap {
         })
     }
 }
-
-fn push_prefix_arguments(
-    prefix_from_spec_as_filter_on_remote: bool,
-    arguments: &mut Vec<BString>,
-    all_refspecs: &[gix_refspec::RefSpec],
-) {
-    if !prefix_from_spec_as_filter_on_remote {
-        return;
-    }
-
-    let mut seen = HashSet::new();
-    for spec in all_refspecs {
-        let spec = spec.to_ref();
-        if seen.insert(spec.instruction()) {
-            let mut prefixes = Vec::with_capacity(1);
-            spec.expand_prefixes(&mut prefixes);
-            for mut prefix in prefixes {
-                prefix.insert_str(0, "ref-prefix ");
-                arguments.push(prefix);
-            }
-        }
-    }
-}

gix-protocol/src/fetch/types.rs

@@ -18,7 +18,7 @@ pub struct Options<'a> {
     pub reject_shallow_remote: bool,
 }
 
-/// For use in [`RefMap::fetch()`] and [`fetch`](crate::fetch()).
+/// For use in [`crate::Handshake::prepare_lsrefs_or_extract_refmap()`] and [`fetch`](crate::fetch()).
 #[cfg(feature = "handshake")]
 pub struct Context<'a, T> {
     /// The outcome of the handshake performed with the remote.
@@ -29,7 +29,7 @@ pub struct Context<'a, T> {
     ///
     /// This is always done if the underlying protocol is V2, which is implied by the absence of refs in the `handshake` outcome.
     pub transport: &'a mut T,
-    /// How to self-identify during the `ls-refs` call in [`RefMap::fetch()`] or the `fetch` call in [`fetch()`](crate::fetch()).
+    /// How to self-identify during the `ls-refs` call in [`crate::Handshake::prepare_lsrefs_or_extract_refmap()`] or the `fetch` call in [`fetch()`](crate::fetch()).
     ///
     /// This could be read from the `gitoxide.userAgent` configuration variable.
     pub user_agent: (&'static str, Option<std::borrow::Cow<'static, str>>),

gix-protocol/src/handshake/mod.rs

@@ -72,45 +72,86 @@ pub(crate) mod hero {
     #[cfg(feature = "fetch")]
     mod fetch {
         #[cfg(feature = "async-client")]
-        use crate::transport::client::async_io::Transport;
+        use crate::transport::client::async_io;
         #[cfg(feature = "blocking-client")]
-        use crate::transport::client::blocking_io::Transport;
-        use crate::Handshake;
+        use crate::transport::client::blocking_io;
+        use crate::{fetch::RefMap, Handshake};
         use gix_features::progress::Progress;
         use std::borrow::Cow;
 
+        /// Intermediate state while potentially fetching a refmap after the handshake.
+        pub enum ObtainRefMap<'a> {
+            /// We already got a refmap to use from the V1 handshake, which always sends them.
+            Existing(RefMap),
+            /// We need to invoke another `ls-refs` command to retrieve the refmap as part of the V2 protocol.
+            LsRefsCommand(crate::LsRefsCommand<'a>, crate::fetch::refmap::init::Context),
+        }
+
+        impl ObtainRefMap<'_> {
+            /// Fetch the refmap, either by returning the existing one or invoking the `ls-refs` command.
+            #[cfg(feature = "async-client")]
+            #[allow(clippy::result_large_err)]
+            pub async fn fetch_async(
+                self,
+                mut progress: impl Progress,
+                transport: &mut impl async_io::Transport,
+                trace_packetlines: bool,
+            ) -> Result<RefMap, crate::fetch::refmap::init::Error> {
+                let (cmd, cx) = match self {
+                    ObtainRefMap::Existing(map) => return Ok(map),
+                    ObtainRefMap::LsRefsCommand(cmd, cx) => (cmd, cx),
+                };
+
+                let _span = gix_trace::coarse!("gix_protocol::handshake::ObtainRefMap::fetch_async()");
+                let capabilities = cmd.capabilities;
+                let remote_refs = cmd.invoke_async(transport, &mut progress, trace_packetlines).await?;
+                RefMap::from_refs(remote_refs, capabilities, cx)
+            }
+
+            /// Fetch the refmap, either by returning the existing one or invoking the `ls-refs` command.
+            #[cfg(feature = "blocking-client")]
+            #[allow(clippy::result_large_err)]
+            pub fn fetch_blocking(
+                self,
+                mut progress: impl Progress,
+                transport: &mut impl blocking_io::Transport,
+                trace_packetlines: bool,
+            ) -> Result<RefMap, crate::fetch::refmap::init::Error> {
+                let (cmd, cx) = match self {
+                    ObtainRefMap::Existing(map) => return Ok(map),
+                    ObtainRefMap::LsRefsCommand(cmd, cx) => (cmd, cx),
+                };
+
+                let _span = gix_trace::coarse!("gix_protocol::handshake::ObtainRefMap::fetch_blocking()");
+                let capabilities = cmd.capabilities;
+                let remote_refs = cmd.invoke_blocking(transport, &mut progress, trace_packetlines)?;
+                RefMap::from_refs(remote_refs, capabilities, cx)
+            }
+        }
+
         impl Handshake {
-            /// Obtain a [refmap](crate::fetch::RefMap) either from this instance, taking it out in the process, if the handshake was
-            /// created from a V1 connection, or use `transport` to fetch the refmap as a separate command invocation.
+            /// Prepare fetching a [refmap](RefMap) if not present in the handshake.
             #[allow(clippy::result_large_err)]
-            #[maybe_async::maybe_async]
-            pub async fn fetch_or_extract_refmap<T>(
+            pub fn prepare_lsrefs_or_extract_refmap(
                 &mut self,
-                mut progress: impl Progress,
-                transport: &mut T,
                 user_agent: (&'static str, Option<Cow<'static, str>>),
-                trace_packetlines: bool,
                 prefix_from_spec_as_filter_on_remote: bool,
                 refmap_context: crate::fetch::refmap::init::Context,
-            ) -> Result<crate::fetch::RefMap, crate::fetch::refmap::init::Error>
-            where
-                T: Transport,
-            {
-                Ok(match self.refs.take() {
-                    Some(refs) => crate::fetch::RefMap::from_refs(refs, &self.capabilities, refmap_context)?,
-                    None => {
-                        crate::fetch::RefMap::fetch(
-                            &mut progress,
-                            &self.capabilities,
-                            transport,
-                            user_agent,
-                            trace_packetlines,
-                            prefix_from_spec_as_filter_on_remote,
-                            refmap_context,
-                        )
-                        .await?
-                    }
-                })
+            ) -> Result<ObtainRefMap<'_>, crate::fetch::refmap::init::Error> {
+                if let Some(refs) = self.refs.take() {
+                    return Ok(ObtainRefMap::Existing(RefMap::from_refs(
+                        refs,
+                        &self.capabilities,
+                        refmap_context,
+                    )?));
+                }
+
+                let all_refspecs = refmap_context.aggregate_refspecs();
+                let prefix_refspecs = prefix_from_spec_as_filter_on_remote.then_some(&all_refspecs[..]);
+                Ok(ObtainRefMap::LsRefsCommand(
+                    crate::LsRefsCommand::new(prefix_refspecs, &self.capabilities, user_agent),
+                    refmap_context,
+                ))
             }
         }
     }
@@ -150,6 +191,7 @@ mod error {
         }
     }
 }
+
 #[cfg(feature = "handshake")]
 pub use error::Error;
 

gix-protocol/src/lib.rs

@@ -5,7 +5,7 @@
 //! * create a `Transport`, either blocking or async
 //! * perform a [`handshake()`]
 //! * execute a [`Command`]
-//!     - [list references](ls_refs())
+//!     - [list references](LsRefsCommand)
 //!          - create a mapping between [refspecs and references](fetch::RefMap)
 //!     - [receive a pack](fetch())
 //!
@@ -67,7 +67,7 @@ pub use handshake::hero::Handshake;
 ///
 pub mod ls_refs;
 #[cfg(any(feature = "blocking-client", feature = "async-client"))]
-pub use ls_refs::function::ls_refs;
+pub use ls_refs::function::LsRefsCommand;
 
 mod util;
 pub use util::*;