feat!: provide fetch::outcome::Negotiate for details on what happened during negotiation.

We also remove the `negotiation_rounds` field in favor of a far more detailed `fetch::outcome::Negotiate` struct.

Author: Byron

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

@@ -46,27 +46,25 @@ pub enum Status {
     ///
     /// As we could determine that nothing changed without remote interaction, there was no negotiation at all.
     NoPackReceived {
+        /// If `true`, we didn't receive a pack due to dry-run mode being enabled.
+        dry_run: bool,
+        /// Information about the pack negotiation phase if negotiation happened at all.
+        ///
+        /// It's possible that negotiation didn't have to happen as no reference of interest changed on the server.
+        negotiate: Option<outcome::Negotiate>,
         /// 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 about the pack negotiation phase.
+        negotiate: outcome::Negotiate,
         /// Information collected while writing the pack and its index.
         write_pack_bundle: gix_pack::bundle::write::Outcome,
         /// Information collected while updating references.
         update_refs: refs::update::Outcome,
     },
-    /// 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,
-    },
 }
 
 /// The outcome of receiving a pack via [`Prepare::receive()`].
@@ -78,6 +76,42 @@ pub struct Outcome {
     pub status: Status,
 }
 
+/// Additional types related to the outcome of a fetch operation.
+pub mod outcome {
+    /// Information about the negotiation phase of a fetch.
+    ///
+    /// Note that negotiation can happen even if no pack is ultimately produced.
+    #[derive(Default, Debug, Clone)]
+    pub struct Negotiate {
+        /// The negotiation graph indicating what kind of information 'the algorithm' collected in the end.
+        pub graph: gix_revision::graph::IdMap<gix_revision::graph::Commit<gix_negotiate::Metadata>>,
+        /// Additional information for each round of negotiation.
+        pub rounds: Vec<negotiate::Round>,
+    }
+
+    ///
+    pub mod negotiate {
+        /// Key information about each round in the pack-negotiation.
+        #[derive(Debug, Clone)]
+        pub struct Round {
+            /// The amount of `HAVE` lines sent this round.
+            ///
+            /// Each `HAVE` is an object that we tell the server about which would acknowledge each one it has as well.
+            pub haves_sent: usize,
+            /// A total counter, over all previous rounds, indicating how many `HAVE`s we sent without seeing a single acknowledgement,
+            /// i.e. the indication of a common object.
+            ///
+            /// This number maybe zero or be lower compared to the previous round if we have received at least one acknowledgement.
+            pub in_vain: usize,
+            /// The amount of haves we should send in this round.
+            ///
+            /// If the value is lower than `haves_sent` (the `HAVE` lines actually sent), the negotiation algorithm has run out of options
+            /// which typically indicates the end of the negotiation phase.
+            pub haves_to_send: usize,
+        }
+    }
+}
+
 /// The progress ids used in during various steps of the fetch operation.
 ///
 /// Note that tagged progress isn't very widely available yet, but support can be improved as needed.

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

@@ -19,7 +19,8 @@ use crate::{
         connection::fetch::config,
         fetch,
         fetch::{
-            negotiate, negotiate::Algorithm, refs, Error, Outcome, Prepare, ProgressId, RefLogMessage, Shallow, Status,
+            negotiate, negotiate::Algorithm, outcome, refs, Error, Outcome, Prepare, ProgressId, RefLogMessage,
+            Shallow, Status,
         },
     },
     Progress, Repository,
@@ -141,11 +142,10 @@ where
             negotiate::make_refmapping_ignore_predicate(con.remote.fetch_tags, &self.ref_map),
         )?;
         let mut previous_response = None::<gix_protocol::fetch::Response>;
-        let mut round = 1;
-        let mut write_pack_bundle = match &action {
+        let (mut write_pack_bundle, negotiate) = match &action {
             negotiate::Action::NoChange | negotiate::Action::SkipToRefUpdate => {
                 gix_protocol::indicate_end_of_interaction(&mut con.transport).await.ok();
-                None
+                (None, None)
             }
             negotiate::Action::MustNegotiate {
                 remote_ref_target_known,
@@ -158,6 +158,7 @@ where
                     &self.shallow,
                     negotiate::make_refmapping_ignore_predicate(con.remote.fetch_tags, &self.ref_map),
                 );
+                let mut rounds = Vec::new();
                 let is_stateless =
                     arguments.is_stateless(!con.transport.connection_persists_across_multiple_requests());
                 let mut haves_to_send = gix_negotiate::window_size(is_stateless, None);
@@ -166,7 +167,7 @@ where
                 let mut common = is_stateless.then(Vec::new);
                 let reader = 'negotiation: loop {
                     progress.step();
-                    progress.set_name(format!("negotiate (round {round})"));
+                    progress.set_name(format!("negotiate (round {})", rounds.len() + 1));
 
                     let is_done = match negotiate::one_round(
                         negotiator.deref_mut(),
@@ -182,6 +183,11 @@ where
                             }
                             seen_ack |= ack_seen;
                             in_vain += haves_sent;
+                            rounds.push(outcome::negotiate::Round {
+                                haves_sent,
+                                in_vain,
+                                haves_to_send,
+                            });
                             let is_done = haves_sent != haves_to_send || (seen_ack && in_vain >= 256);
                             haves_to_send = gix_negotiate::window_size(is_stateless, haves_to_send);
                             is_done
@@ -206,11 +212,9 @@ where
                             setup_remote_progress(progress, &mut reader, should_interrupt);
                         }
                         break 'negotiation reader;
-                    } else {
-                        round += 1;
                     }
                 };
-                drop(graph);
+                let graph = graph.detach();
                 drop(graph_repo);
                 let previous_response = previous_response.expect("knowledge of a pack means a response was received");
                 if !previous_response.shallow_updates().is_empty() && shallow_lock.is_none() {
@@ -267,7 +271,7 @@ where
                         crate::shallow::write(shallow_lock, shallow_commits, previous_response.shallow_updates())?;
                     }
                 }
-                write_pack_bundle
+                (write_pack_bundle, Some(outcome::Negotiate { graph, rounds }))
             }
         };
 
@@ -294,21 +298,17 @@ where
 
         let out = Outcome {
             ref_map: std::mem::take(&mut self.ref_map),
-            status: if matches!(self.dry_run, fetch::DryRun::Yes) {
-                assert!(write_pack_bundle.is_none(), "in dry run we never read a bundle");
-                Status::DryRun {
+            status: match write_pack_bundle {
+                Some(write_pack_bundle) => Status::Change {
+                    write_pack_bundle,
                     update_refs,
-                    negotiation_rounds: round,
-                }
-            } else {
-                match write_pack_bundle {
-                    Some(write_pack_bundle) => Status::Change {
-                        write_pack_bundle,
-                        update_refs,
-                        negotiation_rounds: round,
-                    },
-                    None => Status::NoPackReceived { update_refs },
-                }
+                    negotiate: negotiate.expect("if we have a pack, we always negotiated it"),
+                },
+                None => Status::NoPackReceived {
+                    dry_run: matches!(self.dry_run, fetch::DryRun::Yes),
+                    negotiate,
+                    update_refs,
+                },
             },
         };
         Ok(out)

gix/src/remote/fetch.rs

@@ -11,7 +11,9 @@ pub mod negotiate {
 }
 
 #[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))]
-pub use super::connection::fetch::{prepare, refs, Error, Outcome, Prepare, ProgressId, RefLogMessage, Status};
+pub use super::connection::fetch::{
+    outcome, prepare, refs, Error, Outcome, Prepare, ProgressId, RefLogMessage, Status,
+};
 
 /// If `Yes`, don't really make changes but do as much as possible to get an idea of what would be done.
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]

gix/tests/remote/fetch.rs

@@ -137,12 +137,13 @@ mod blocking_and_async_io {
 
         match out.status {
             Status::Change {
-                negotiation_rounds,
+                negotiate,
                 write_pack_bundle,
                 ..
             } => {
                 assert_eq!(
-                    negotiation_rounds, 1,
+                    negotiate.rounds.len(),
+                    1,
                     "we don't really have a way to see that tips from alternates were added, I think"
                 );
                 assert_eq!(
@@ -215,11 +216,12 @@ mod blocking_and_async_io {
                 match changes.status {
                     Status::Change {
                         write_pack_bundle,
-                        negotiation_rounds,
+                        negotiate,
                         ..
                     } => {
                         assert_eq!(
-                            negotiation_rounds, expected_negotiation_rounds,
+                            negotiate.rounds.len(),
+                            expected_negotiation_rounds,
                             "we need multiple rounds"
                         );
                         // the server only has our `b1` and an extra commit or two.
@@ -389,8 +391,9 @@ mod blocking_and_async_io {
                         .await?;
 
                     match res.status {
-                    fetch::Status::NoPackReceived { update_refs } => {
+                    fetch::Status::NoPackReceived { update_refs, negotiate: _, dry_run } => {
                         assert_eq!(update_refs.edits.len(), expected_ref_count, "{shallow_args:?}|{fetch_tags:?}");
+                        assert!(!dry_run, "we actually perform the operation");
                     },
                     _ => unreachable!(
                         "{shallow_args:?}|{fetch_tags:?}: default negotiation is able to realize nothing is required and doesn't get to receiving a pack"
@@ -446,8 +449,8 @@ mod blocking_and_async_io {
                 .await?;
 
             match res.status {
-                gix::remote::fetch::Status::Change { write_pack_bundle, update_refs, negotiation_rounds } => {
-                    assert_eq!(negotiation_rounds, 1);
+                gix::remote::fetch::Status::Change { write_pack_bundle, update_refs, negotiate } => {
+                    assert_eq!(negotiate.rounds.len(), 1);
                     assert_eq!(write_pack_bundle.index.data_hash, hex_to_id(expected_data_hash), );
                     assert_eq!(write_pack_bundle.index.num_objects, 3 + num_objects_offset, "{fetch_tags:?}");
                     assert!(write_pack_bundle.data_path.as_deref().map_or(false, std::path::Path::is_file));
@@ -530,9 +533,9 @@ mod blocking_and_async_io {
                     fetch::Status::Change {
                         write_pack_bundle,
                         update_refs,
-                        negotiation_rounds,
+                        negotiate,
                     } => {
-                        assert_eq!(negotiation_rounds, 1);
+                        assert_eq!(negotiate.rounds.len(), 1);
                         assert_eq!(write_pack_bundle.pack_version, gix::odb::pack::data::Version::V2);
                         assert_eq!(write_pack_bundle.object_hash, repo.object_hash());
                         assert_eq!(write_pack_bundle.index.num_objects, 4, "{dry_run}: this value is 4 when git does it with 'consecutive' negotiation style, but could be 33 if completely naive.");
@@ -568,16 +571,17 @@ mod blocking_and_async_io {
 
                         update_refs
                     }
-                    fetch::Status::DryRun {
+                    fetch::Status::NoPackReceived {
+                        dry_run,
                         update_refs,
-                        negotiation_rounds,
+                        negotiate: _,
                     } => {
-                        assert_eq!(negotiation_rounds, 1);
+                        assert!(
+                            dry_run,
+                            "the only reason we receive no pack is if we are in dry-run mode"
+                        );
                         update_refs
                     }
-                    fetch::Status::NoPackReceived { .. } => {
-                        unreachable!("we firmly expect changes here, as the other origin has changes")
-                    }
                 };
 
                 assert_eq!(