Merge pull request #1731 from GitoxideLabs/fix-pack-receive

fix pack receive

Author: Byron

gitoxide-core/src/net.rs

@@ -36,4 +36,20 @@ mod impls {
 }
 
 #[cfg(any(feature = "async-client", feature = "blocking-client"))]
-pub use gix::protocol::transport::connect;
+#[gix::protocol::maybe_async::maybe_async]
+pub async fn connect<Url, E>(
+    url: Url,
+    options: gix::protocol::transport::client::connect::Options,
+) -> Result<
+    gix::protocol::SendFlushOnDrop<Box<dyn gix::protocol::transport::client::Transport + Send>>,
+    gix::protocol::transport::client::connect::Error,
+>
+where
+    Url: TryInto<gix::url::Url, Error = E>,
+    gix::url::parse::Error: From<E>,
+{
+    Ok(gix::protocol::SendFlushOnDrop::new(
+        gix::protocol::transport::connect(url, options).await?,
+        false,
+    ))
+}

gitoxide-core/src/pack/receive.rs

@@ -3,6 +3,7 @@ use crate::pack::receive::protocol::fetch::negotiate;
 use crate::OutputFormat;
 use gix::config::tree::Key;
 use gix::protocol::maybe_async;
+use gix::remote::fetch::Error;
 use gix::DynNestedProgress;
 pub use gix::{
     hash::ObjectId,
@@ -64,7 +65,7 @@ where
 
     let agent = gix::protocol::agent(gix::env::agent());
     let mut handshake = gix::protocol::fetch::handshake(
-        &mut transport,
+        &mut transport.inner,
         gix::protocol::credentials::builtin,
         vec![("agent".into(), Some(agent.clone()))],
         &mut progress,
@@ -85,13 +86,22 @@ where
         &fetch_refspecs,
         gix::protocol::fetch::Context {
             handshake: &mut handshake,
-            transport: &mut transport,
+            transport: &mut transport.inner,
             user_agent: user_agent.clone(),
             trace_packetlines,
         },
         gix::protocol::fetch::refmap::init::Options::default(),
     )
     .await?;
+
+    if refmap.mappings.is_empty() && !refmap.remote_refs.is_empty() {
+        return Err(Error::NoMapping {
+            refspecs: refmap.refspecs.clone(),
+            num_remote_refs: refmap.remote_refs.len(),
+        }
+        .into());
+    }
+
     let mut negotiate = Negotiate { refmap: &refmap };
     gix::protocol::fetch(
         &mut negotiate,
@@ -114,7 +124,7 @@ where
         &ctx.should_interrupt,
         gix::protocol::fetch::Context {
             handshake: &mut handshake,
-            transport: &mut transport,
+            transport: &mut transport.inner,
             user_agent,
             trace_packetlines,
         },
@@ -140,10 +150,13 @@ impl gix::protocol::fetch::Negotiate for Negotiate<'_> {
         })
     }
 
-    fn add_wants(&mut self, arguments: &mut Arguments, _remote_ref_target_known: &[bool]) {
+    fn add_wants(&mut self, arguments: &mut Arguments, _remote_ref_target_known: &[bool]) -> bool {
+        let mut has_want = false;
         for id in self.refmap.mappings.iter().filter_map(|m| m.remote.as_id()) {
             arguments.want(id);
+            has_want = true;
         }
+        has_want
     }
 
     fn one_round(

gix-protocol/src/fetch/error.rs

@@ -21,8 +21,8 @@ pub enum Error {
     LockShallowFile(#[from] gix_lock::acquire::Error),
     #[error("Receiving objects from shallow remotes is prohibited due to the value of `clone.rejectShallow`")]
     RejectShallowRemote,
-    #[error("Failed to consume the pack sent by the remove")]
-    ConsumePack(Box<dyn std::error::Error + Send + Sync + 'static>),
+    #[error("Failed to consume the pack sent by the remote")]
+    ConsumePack(#[source] Box<dyn std::error::Error + Send + Sync + 'static>),
     #[error("Failed to read remaining bytes in stream")]
     ReadRemainingBytes(#[source] std::io::Error),
 }

gix-protocol/src/fetch/function.rs

@@ -22,12 +22,12 @@ use std::sync::atomic::{AtomicBool, Ordering};
 /// * …update local refs
 /// * …end the interaction after the fetch
 ///
-/// Note that the interaction will never be ended, even on error or failure, leaving it up to the caller to do that, maybe
+/// **Note that the interaction will never be ended**, even on error or failure, leaving it up to the caller to do that, maybe
 /// with the help of [`SendFlushOnDrop`](crate::SendFlushOnDrop) which can wrap `transport`.
 /// Generally, the `transport` is left in a state that allows for more commands to be run.
 ///
-/// Return `Ok(None)` if there was nothing to do because all remote refs are at the same state as they are locally, or `Ok(Some(outcome))`
-/// to inform about all the changes that were made.
+/// Return `Ok(None)` if there was nothing to do because all remote refs are at the same state as they are locally,
+/// or there was nothing wanted, or `Ok(Some(outcome))` to inform about all the changes that were made.
 #[maybe_async::maybe_async]
 pub async fn fetch<P, T, E>(
     negotiate: &mut impl Negotiate,
@@ -91,14 +91,21 @@ where
         negotiate::Action::MustNegotiate {
             remote_ref_target_known,
         } => {
-            negotiate.add_wants(&mut arguments, remote_ref_target_known);
+            if !negotiate.add_wants(&mut arguments, remote_ref_target_known) {
+                return Ok(None);
+            }
             let mut rounds = Vec::new();
             let is_stateless = arguments.is_stateless(!transport.connection_persists_across_multiple_requests());
             let mut state = negotiate::one_round::State::new(is_stateless);
             let mut reader = 'negotiation: loop {
                 let _round = gix_trace::detail!("negotiate round", round = rounds.len() + 1);
                 progress.step();
                 progress.set_name(format!("negotiate (round {})", rounds.len() + 1));
+                if should_interrupt.load(Ordering::Relaxed) {
+                    return Err(Error::Negotiate(negotiate::Error::NegotiationFailed {
+                        rounds: rounds.len(),
+                    }));
+                }
 
                 let is_done = match negotiate.one_round(&mut state, &mut arguments, previous_response.as_ref()) {
                     Ok((round, is_done)) => {

gix-protocol/src/fetch/negotiate.rs

@@ -284,17 +284,21 @@ pub fn make_refmapping_ignore_predicate(fetch_tags: Tags, ref_map: &RefMap) -> i
 /// * `ref_map` is the state of refs as known on the remote.
 /// * `shallow` defines if the history should be shallow.
 /// * `mapping_is_ignored` is typically initialized with [`make_refmapping_ignore_predicate`].
+///
+/// Returns `true` if at least one [want](crate::fetch::Arguments::want()) was added, or `false` otherwise.
+/// Note that not adding a single want can make the remote hang, so it's avoided on the client side by ending the fetch operation.
 pub fn add_wants(
     objects: &impl gix_object::FindHeader,
     arguments: &mut crate::fetch::Arguments,
     ref_map: &RefMap,
     remote_ref_target_known: &[bool],
     shallow: &Shallow,
     mapping_is_ignored: impl Fn(&refmap::Mapping) -> bool,
-) {
+) -> bool {
     // When using shallow, we can't exclude `wants` as the remote won't send anything then. Thus, we have to resend everything
     // we have as want instead to get exactly the same graph, but possibly deepened.
     let is_shallow = !matches!(shallow, Shallow::NoChange);
+    let mut has_want = false;
     let wants = ref_map
         .mappings
         .iter()
@@ -306,13 +310,15 @@ pub fn add_wants(
         if !arguments.can_use_ref_in_want() || matches!(want.remote, refmap::Source::ObjectId(_)) {
             if let Some(id) = id_on_remote {
                 arguments.want(id);
+                has_want = true;
             }
         } else {
             arguments.want_ref(
                 want.remote
                     .as_name()
                     .expect("name available if this isn't an object id"),
             );
+            has_want = true;
         }
         let id_is_annotated_tag_we_have = id_on_remote
             .and_then(|id| objects.try_header(id).ok().flatten().map(|h| (id, h)))
@@ -324,6 +330,7 @@ pub fn add_wants(
             arguments.have(tag_on_remote);
         }
     }
+    has_want
 }
 
 /// Remove all commits that are more recent than the cut-off, which is the commit time of the oldest common commit we have with the server.

gix-protocol/src/fetch/types.rs

@@ -70,7 +70,9 @@ mod with_fetch {
         /// Typically invokes [`negotiate::mark_complete_and_common_ref()`].
         fn mark_complete_and_common_ref(&mut self) -> Result<negotiate::Action, negotiate::Error>;
         /// Typically invokes [`negotiate::add_wants()`].
-        fn add_wants(&mut self, arguments: &mut fetch::Arguments, remote_ref_target_known: &[bool]);
+        /// Returns `true` if wants were added, or `false` if the negotiation should be aborted.
+        #[must_use]
+        fn add_wants(&mut self, arguments: &mut fetch::Arguments, remote_ref_target_known: &[bool]) -> bool;
         /// Typically invokes [`negotiate::one_round()`].
         fn one_round(
             &mut self,

gix/src/remote/connection/fetch/receive_pack.rs

@@ -274,15 +274,15 @@ impl gix_protocol::fetch::Negotiate for Negotiate<'_, '_, '_> {
         )
     }
 
-    fn add_wants(&mut self, arguments: &mut Arguments, remote_ref_target_known: &[bool]) {
+    fn add_wants(&mut self, arguments: &mut Arguments, remote_ref_target_known: &[bool]) -> bool {
         negotiate::add_wants(
             self.objects,
             arguments,
             self.ref_map,
             remote_ref_target_known,
             self.shallow,
             negotiate::make_refmapping_ignore_predicate(self.tags, self.ref_map),
-        );
+        )
     }
 
     fn one_round(

tests/journey/gix.sh

@@ -109,7 +109,7 @@ title "gix (with repository)"
 
         # for some reason, on CI the daemon always shuts down before we can connect,
         # or isn't actually ready despite having accepted the first connection already.
-        (not_on_ci with "git:// protocol"
+        (with "git:// protocol"
           launch-git-daemon
           (with "version 1"
             it "generates the correct output" && {
@@ -278,7 +278,7 @@ title "gix commit-graph"
           )
         )
         fi
-        (not_on_ci with "git:// protocol"
+        (with "git:// protocol"
           launch-git-daemon
           (with "version 1"
             (with "NO output directory"