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.

Sebastian Thiel committed 3 years ago

Commit 682def01 parent d0ff6bc4 files · +94 −49Browse files

Unified · Split

4 files

gix/src/remote/connection/fetch/mod.rs +48 −10
gix/src/remote/connection/fetch/receive_pack.rs +24 −23
gix/src/remote/fetch.rs +3 −1
gix/tests/remote/fetch.rs +19 −15

gix/src/remote/connection/fetch/mod.rs+48−10

@@ -46,27 +46,25 @@ pub enum Status { · 45 unmodified lines
46	///	46	///
47	/// As we could determine that nothing changed without remote interaction, there was no negotiation at all.	47	/// As we could determine that nothing changed without remote interaction, there was no negotiation at all.
48	NoPackReceived {	48	NoPackReceived {
		49	/// If `true`, we didn't receive a pack due to dry-run mode being enabled.
		50	dry_run: bool,
		51	/// Information about the pack negotiation phase if negotiation happened at all.
		52	///
		53	/// It's possible that negotiation didn't have to happen as no reference of interest changed on the server.
		54	negotiate: Option<outcome::Negotiate>,
49	/// However, depending on the refspecs, references might have been updated nonetheless to point to objects as	55	/// However, depending on the refspecs, references might have been updated nonetheless to point to objects as
50	/// reported by the remote.	56	/// reported by the remote.
51	update_refs: refs::update::Outcome,	57	update_refs: refs::update::Outcome,
52	},	58	},
53	/// There was at least one tip with a new object which we received.	59	/// There was at least one tip with a new object which we received.
54	Change {	60	Change {
55	/// The number of rounds it took to minimize the pack to contain only the objects we don't have.	61	/// Information about the pack negotiation phase.
56	negotiation_rounds: usize,	62	negotiate: outcome::Negotiate,
57	/// Information collected while writing the pack and its index.	63	/// Information collected while writing the pack and its index.
58	write_pack_bundle: gix_pack::bundle::write::Outcome,	64	write_pack_bundle: gix_pack::bundle::write::Outcome,
59	/// Information collected while updating references.	65	/// Information collected while updating references.
60	update_refs: refs::update::Outcome,	66	update_refs: refs::update::Outcome,
61	},	67	},
62	/// A dry run was performed which leaves the local repository without any change
63	/// nor will a pack have been received.
64	DryRun {
65	/// The number of rounds it took to minimize the would-be-sent-pack to contain only the objects we don't have.
66	negotiation_rounds: usize,
67	/// Information about what updates to refs would have been done.
68	update_refs: refs::update::Outcome,
69	},
70	}	68	}
71		69
72	/// The outcome of receiving a pack via [`Prepare::receive()`].	70	/// The outcome of receiving a pack via [`Prepare::receive()`].
@@ -78,6 +76,46 @@ pub struct Outcome { · 5 unmodified lines
78	pub status: Status,	76	pub status: Status,
79	}	77	}
80		78
		79	/// Additional types related to the outcome of a fetch operation.
		80	pub mod outcome {
		81	/// Information about the negotiation phase of a fetch.
		82	///
		83	/// Note that negotiation can happen even if no pack is ultimately produced.
		84	#[derive(Default, Debug, Clone)]
		85	pub struct Negotiate {
		86	/// The negotiation graph indicating what kind of information 'the algorithm' collected in the end.
		87	pub graph: gix_negotiate::IdMap,
		88	/// Additional information for each round of negotiation.
		89	pub rounds: Vec<negotiate::Round>,
		90	}
		91
		92	///
		93	pub mod negotiate {
		94	/// Key information about each round in the pack-negotiation.
		95	#[derive(Debug, Clone)]
		96	pub struct Round {
		97	/// The amount of `HAVE` lines sent this round.
		98	///
		99	/// Each `HAVE` is an object that we tell the server about which would acknowledge each one it has as well.
		100	pub haves_sent: usize,
		101	/// A total counter, over all previous rounds, indicating how many `HAVE`s we sent without seeing a single acknowledgement,
		102	/// i.e. the indication of a common object.
		103	///
		104	/// This number maybe zero or be lower compared to the previous round if we have received at least one acknowledgement.
		105	pub in_vain: usize,
		106	/// The amount of haves we should send in this round.
		107	///
		108	/// If the value is lower than `haves_sent` (the `HAVE` lines actually sent), the negotiation algorithm has run out of options
		109	/// which typically indicates the end of the negotiation phase.
		110	pub haves_to_send: usize,
		111	/// If `true`, the server reported, as response to our previous `HAVE`s, that at least one of them is in common by acknowledging it.
		112	///
		113	/// This may also lead to the server responding with a pack.
		114	pub previous_response_had_at_least_one_in_common: bool,
		115	}
		116	}
		117	}
		118
81	/// The progress ids used in during various steps of the fetch operation.	119	/// The progress ids used in during various steps of the fetch operation.
82	///	120	///
83	/// Note that tagged progress isn't very widely available yet, but support can be improved as needed.	121	/// 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+24−23

         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,
             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, negotiate) = match &action {
-        let mut write_pack_bundle = 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,
                     &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);
                 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(),
                             }
                             seen_ack |= ack_seen;
                             in_vain += haves_sent;
+                            rounds.push(outcome::negotiate::Round {
+                                haves_sent,
+                                in_vain,
+                                haves_to_send,
+                                previous_response_had_at_least_one_in_common: ack_seen,
+                            });
                             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
                             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() {
                         crate::shallow::write(shallow_lock, shallow_commits, previous_response.shallow_updates())?;
                     }
                 }
-                write_pack_bundle
+                (write_pack_bundle, Some(outcome::Negotiate { graph, rounds }))
             }
         };
         let out = Outcome {
             ref_map: std::mem::take(&mut self.ref_map),
-            status: if matches!(self.dry_run, fetch::DryRun::Yes) {
+            status: match write_pack_bundle {
-                assert!(write_pack_bundle.is_none(), "in dry run we never read a bundle");
+                Some(write_pack_bundle) => Status::Change {
-                Status::DryRun {
+                    write_pack_bundle,
                     update_refs,
-                    negotiation_rounds: round,
+                    negotiate: negotiate.expect("if we have a pack, we always negotiated it"),
-                }
+                },
-            } else {
+                None => Status::NoPackReceived {
-                match write_pack_bundle {
+                    dry_run: matches!(self.dry_run, fetch::DryRun::Yes),
-                    Some(write_pack_bundle) => Status::Change {
+                    negotiate,
-                        write_pack_bundle,
+                    update_refs,
-                        update_refs,
+                },
-                        negotiation_rounds: round,
-                    },
-                    None => Status::NoPackReceived { update_refs },
-                }
             },
         };
         Ok(out)

gix/src/remote/fetch.rs+3−1

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

gix/tests/remote/fetch.rs+19−15

         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!(
                 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.
                         .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"
                 .await?;
             match res.status {
-                gix::remote::fetch::Status::Change { write_pack_bundle, update_refs, negotiation_rounds } => {
+                gix::remote::fetch::Status::Change { write_pack_bundle, update_refs, negotiate } => {
-                    assert_eq!(negotiation_rounds, 1);
+                    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));
                     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.");
                         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!(