Merge pull request #2242 from djc/ls-refs

Isolate logic surrounding ls-refs command

Author: Byron

gitoxide-core/src/pack/receive.rs

@@ -4,6 +4,7 @@ use std::{
     sync::{atomic::AtomicBool, Arc},
 };
 
+use crate::{net, pack::receive::protocol::fetch::negotiate, OutputFormat};
 #[cfg(feature = "async-client")]
 use gix::protocol::transport::client::async_io::connect;
 #[cfg(feature = "blocking-client")]
@@ -23,8 +24,6 @@ pub use gix::{
     NestedProgress, Progress,
 };
 
-use crate::{net, pack::receive::protocol::fetch::negotiate, OutputFormat};
-
 pub const PROGRESS_RANGE: std::ops::RangeInclusive<u8> = 1..=3;
 pub struct Context<W> {
     pub thread_limit: Option<usize>,
@@ -65,7 +64,7 @@ where
     .is_some();
 
     let agent = gix::protocol::agent(gix::env::agent());
-    let mut handshake = gix::protocol::fetch::handshake(
+    let mut handshake: gix::protocol::Handshake = gix::protocol::fetch::handshake(
         &mut transport.inner,
         gix::protocol::credentials::builtin,
         vec![("agent".into(), Some(agent.clone()))],
@@ -82,18 +81,21 @@ where
         })
         .collect::<Result<_, _>>()?;
     let user_agent = ("agent", Some(agent.clone().into()));
-    let refmap = gix::protocol::fetch::RefMap::new(
-        &mut progress,
-        &fetch_refspecs,
-        gix::protocol::fetch::Context {
-            handshake: &mut handshake,
-            transport: &mut transport.inner,
-            user_agent: user_agent.clone(),
+
+    let context = gix::protocol::fetch::refmap::init::Context {
+        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,
-        },
-        gix::protocol::fetch::refmap::init::Options::default(),
-    )
-    .await?;
+            true,
+            context,
+        )
+        .await?;
 
     if refmap.mappings.is_empty() && !refmap.remote_refs.is_empty() {
         return Err(Error::NoMapping {

gix-protocol/src/fetch/handshake.rs

@@ -6,10 +6,7 @@ use maybe_async::maybe_async;
 use crate::transport::client::async_io::Transport;
 #[cfg(feature = "blocking-client")]
 use crate::transport::client::blocking_io::Transport;
-use crate::{
-    credentials,
-    handshake::{Error, Outcome},
-};
+use crate::{credentials, handshake::Error, Handshake};
 
 /// Perform a handshake with the server on the other side of `transport`, with `authenticate` being used if authentication
 /// turns out to be required. `extra_parameters` are the parameters `(name, optional value)` to add to the handshake,
@@ -22,7 +19,7 @@ pub async fn upload_pack<AuthFn, T>(
     authenticate: AuthFn,
     extra_parameters: Vec<(String, Option<String>)>,
     progress: &mut impl Progress,
-) -> Result<Outcome, Error>
+) -> Result<Handshake, Error>
 where
     AuthFn: FnMut(credentials::helper::Action) -> credentials::protocol::Result,
     T: Transport,

gix-protocol/src/fetch/mod.rs

@@ -5,7 +5,7 @@
 ///
 /// * [handshake](handshake())
 /// * **ls-refs**
-///     * [get available refs by refspecs](RefMap::new())
+///     * [get available refs by refspecs](RefMap::fetch())
 /// * **fetch pack**
 ///     * `negotiate` until a pack can be received (TBD)
 /// * [officially terminate the connection](crate::indicate_end_of_interaction())

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::new()`].
+///     - The references known on the remote, as previously obtained with [`RefMap::fetch()`].
 /// * `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,21 +1,22 @@
-use std::collections::HashSet;
+use std::{borrow::Cow, collections::HashSet};
 
-use bstr::{BString, ByteVec};
+use bstr::{BString, ByteSlice, ByteVec};
 use gix_features::progress::Progress;
+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,
     fetch::{
         refmap::{Mapping, Source, SpecIndex},
         RefMap,
     },
+    handshake::Ref,
 };
 
-/// The error returned by [`RefMap::new()`].
+/// The error returned by [`RefMap::fetch()`].
 #[derive(Debug, thiserror::Error)]
 #[allow(missing_docs)]
 pub enum Error {
@@ -27,96 +28,82 @@ pub enum Error {
     ListRefs(#[from] crate::ls_refs::Error),
 }
 
-/// For use in [`RefMap::new()`].
+/// For use in [`RefMap::fetch()`].
 #[derive(Debug, Clone)]
-pub struct Options {
-    /// 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. Defaults to `true`.
-    pub prefix_from_spec_as_filter_on_remote: bool,
+pub struct Context {
+    /// All explicit refspecs to identify references on the remote that you are interested in.
+    /// Note that these are copied to [`RefMap::refspecs`] for convenience, as `RefMap::mappings` refer to them by index.
+    pub fetch_refspecs: Vec<gix_refspec::RefSpec>,
     /// A list of refspecs to use as implicit refspecs which won't be saved or otherwise be part of the remote in question.
     ///
     /// This is useful for handling `remote.<name>.tagOpt` for example.
     pub extra_refspecs: Vec<gix_refspec::RefSpec>,
 }
 
-impl Default for Options {
-    fn default() -> Self {
-        Options {
-            prefix_from_spec_as_filter_on_remote: true,
-            extra_refspecs: Vec::new(),
-        }
+impl Context {
+    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
+    /// Create a new instance by obtaining all references on the remote that have been filtered through our remote's specs
     /// for _fetching_.
     ///
-    /// A [context](fetch::Context) is provided to bundle what would be additional parameters,
-    /// and [options](Options) are used to further configure the call.
-    ///
     /// * `progress` is used if `ls-refs` is invoked on the remote. Always the case when V2 is used.
-    /// * `fetch_refspecs` are all explicit refspecs to identify references on the remote that you are interested in.
-    ///    Note that these are copied to [`RefMap::refspecs`] for convenience, as `RefMap::mappings` refer to them by index.
+    /// * `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 new<T>(
+    pub async fn fetch<T>(
         mut progress: impl Progress,
-        fetch_refspecs: &[gix_refspec::RefSpec],
-        fetch::Context {
-            handshake,
-            transport,
-            user_agent,
-            trace_packetlines,
-        }: fetch::Context<'_, T>,
-        Options {
-            prefix_from_spec_as_filter_on_remote,
-            extra_refspecs,
-        }: Options,
+        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 null = gix_hash::ObjectId::null(gix_hash::Kind::Sha1); // OK to hardcode Sha1, it's not supposed to match, ever.
+        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?;
 
-        let all_refspecs = {
-            let mut s: Vec<_> = fetch_refspecs.to_vec();
-            s.extend(extra_refspecs.clone());
-            s
-        };
-        let remote_refs = match handshake.refs.take() {
-            Some(refs) => refs,
-            None => {
-                crate::ls_refs(
-                    transport,
-                    &handshake.capabilities,
-                    |_capabilities, arguments, features| {
-                        features.push(user_agent);
-                        if prefix_from_spec_as_filter_on_remote {
-                            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);
-                                    }
-                                }
-                            }
-                        }
-                        Ok(crate::ls_refs::Action::Continue)
-                    },
-                    &mut progress,
-                    trace_packetlines,
-                )
-                .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> {
+        let all_refspecs = context.aggregate_refspecs();
+        let Context {
+            fetch_refspecs,
+            extra_refspecs,
+        } = context;
         let num_explicit_specs = fetch_refspecs.len();
         let group = gix_refspec::MatchGroup::from_fetch_specs(all_refspecs.iter().map(gix_refspec::RefSpec::to_ref));
+        let null = gix_hash::ObjectId::null(gix_hash::Kind::Sha1); // OK to hardcode Sha1, it's not supposed to match, ever.
         let (res, fixes) = group
             .match_lhs(remote_refs.iter().map(|r| {
                 let (full_ref_name, target, object) = r.unpack();
@@ -150,24 +137,9 @@ impl RefMap {
             })
             .collect();
 
-        let object_hash = extract_object_format(handshake)?;
-        Ok(RefMap {
-            mappings,
-            refspecs: fetch_refspecs.to_vec(),
-            extra_refspecs,
-            fixes,
-            remote_refs,
-            object_hash,
-        })
-    }
-}
-
-/// Assume sha1 if server says nothing, otherwise configure anything beyond sha1 in the local repo configuration
-#[allow(clippy::result_large_err)]
-fn extract_object_format(outcome: &crate::handshake::Outcome) -> Result<gix_hash::Kind, Error> {
-    use bstr::ByteSlice;
-    let object_hash =
-        if let Some(object_format) = outcome.capabilities.capability("object-format").and_then(|c| c.value()) {
+        // Assume sha1 if server says nothing, otherwise configure anything beyond sha1 in the local repo configuration
+        let object_hash = if let Some(object_format) = capabilities.capability("object-format").and_then(|c| c.value())
+        {
             let object_format = object_format.to_str().map_err(|_| Error::UnknownObjectFormat {
                 format: object_format.into(),
             })?;
@@ -178,5 +150,37 @@ fn extract_object_format(outcome: &crate::handshake::Outcome) -> Result<gix_hash
         } else {
             gix_hash::Kind::Sha1
         };
-    Ok(object_hash)
+
+        Ok(Self {
+            mappings,
+            refspecs: fetch_refspecs,
+            extra_refspecs,
+            fixes,
+            remote_refs,
+            object_hash,
+        })
+    }
+}
+
+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/response/mod.rs

@@ -185,7 +185,7 @@ impl Response {
     }
 
     /// Append the given `updates` which may have been obtained from a
-    /// (handshake::Outcome)[crate::handshake::Outcome::v1_shallow_updates].
+    /// (handshake::Outcome)[crate::Handshake::v1_shallow_updates].
     ///
     /// In V2, these are received as part of the pack, but V1 sends them early, so we
     /// offer to re-integrate them here.

gix-protocol/src/fetch/types.rs

@@ -18,18 +18,18 @@ pub struct Options<'a> {
     pub reject_shallow_remote: bool,
 }
 
-/// For use in [`RefMap::new()`] and [`fetch`](crate::fetch()).
+/// For use in [`RefMap::fetch()`] and [`fetch`](crate::fetch()).
 #[cfg(feature = "handshake")]
 pub struct Context<'a, T> {
     /// The outcome of the handshake performed with the remote.
     ///
     /// Note that it's mutable as depending on the protocol, it may contain refs that have been sent unconditionally.
-    pub handshake: &'a mut crate::handshake::Outcome,
+    pub handshake: &'a mut crate::Handshake,
     /// The transport to use when making an `ls-refs` or `fetch` call.
     ///
     /// 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::new()`] or the `fetch` call in [`fetch()`](crate::fetch()).
+    /// How to self-identify during the `ls-refs` call in [`RefMap::fetch()`] 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>>),
@@ -39,10 +39,7 @@ pub struct Context<'a, T> {
 
 #[cfg(feature = "fetch")]
 mod with_fetch {
-    use crate::{
-        fetch,
-        fetch::{negotiate, refmap},
-    };
+    use crate::fetch::{self, negotiate, refmap};
 
     /// For use in [`fetch`](crate::fetch()).
     pub struct NegotiateContext<'a, 'b, 'c, Objects, Alternates, AlternatesOut, AlternatesErr, Find>
@@ -126,7 +123,7 @@ mod with_fetch {
         /// [`refmap::SpecIndex::ExplicitInRemote`] in [`refmap::Mapping`].
         pub refspecs: Vec<gix_refspec::RefSpec>,
         /// Refspecs which have been added implicitly due to settings of the `remote`, usually pre-initialized from
-        /// [`extra_refspecs` in RefMap options](refmap::init::Options).
+        /// [`extra_refspecs` in RefMap options](refmap::init::Context).
         /// They are referred to by [`refmap::SpecIndex::Implicit`] in [`refmap::Mapping`].
         ///
         /// They are never persisted nor are they typically presented to the user.