[ref-ls] A step towards getting the negotiation right, really need tests

Author: Byron

git-protocol/src/fetch/arguments.rs

@@ -7,10 +7,10 @@ use std::fmt;
 use std::io::Write;
 
 pub struct Arguments {
-    base_features: Vec<String>,
     base_args: Vec<BString>,
 
     args: Vec<BString>,
+    haves: Vec<BString>,
 
     filter: bool,
     shallow: bool,
@@ -48,7 +48,7 @@ impl Arguments {
         }
     }
     pub fn have(&mut self, id: borrowed::Id) {
-        self.prefixed("have ", id);
+        self.haves.push(format!("have {}", id).into());
     }
     pub fn deepen(&mut self, depth: usize) {
         assert!(self.shallow, "'shallow' feature required for deepen");
@@ -78,7 +78,7 @@ impl Arguments {
         let mut deepen_since = shallow;
         let mut deepen_not = shallow;
         let mut deepen_relative = shallow;
-        let (initial_arguments, base_features, features_for_first_want) = match version {
+        let (initial_arguments, features_for_first_want) = match version {
             Protocol::V1 => {
                 deepen_since = has("deepen-since");
                 deepen_not = has("deepen-not");
@@ -90,15 +90,15 @@ impl Arguments {
                         None => n.to_string(),
                     })
                     .collect::<Vec<_>>();
-                (Vec::new(), baked_features.clone(), Some(baked_features))
+                (Vec::new(), Some(baked_features))
             }
-            Protocol::V2 => (Command::Fetch.initial_arguments(&features), Vec::new(), None),
+            Protocol::V2 => (Command::Fetch.initial_arguments(&features), None),
         };
 
         Arguments {
-            base_features,
             base_args: initial_arguments.clone(),
             args: initial_arguments,
+            haves: Vec::new(),
             filter,
             shallow,
             deepen_not,
@@ -121,17 +121,24 @@ impl Arguments {
         }
         match version {
             git_transport::Protocol::V1 => {
-                let mut on_drop = vec![client::MessageKind::Flush];
-                if add_done_argument {
-                    on_drop.push(client::MessageKind::Text(&b"done"[..]));
-                }
+                let mut on_drop = if add_done_argument {
+                    vec![client::MessageKind::Text(&b"done"[..])]
+                } else {
+                    vec![client::MessageKind::Flush]
+                };
+                let is_stateful = transport.is_stateful();
+                let retained_state = if is_stateful { None } else { Some(self.args.clone()) };
                 let mut line_writer = transport.request(client::WriteMode::OneLFTerminatedLinePerWriteCall, on_drop)?;
+
                 for arg in self.args.drain(..) {
                     line_writer.write_all(&arg)?;
                 }
-                if !is_done {
-                    // re-install features for the next round
-                    self.features_for_first_want = Some(self.base_features.clone());
+                line_writer.write_message(client::MessageKind::Flush)?;
+                for line in self.haves.drain(..) {
+                    line_writer.write_all(&line)?;
+                }
+                if let Some(next_args) = retained_state {
+                    self.args = next_args;
                 }
                 Ok(line_writer.into_read())
             }

git-protocol/src/fetch/delegate.rs

@@ -46,10 +46,23 @@ pub trait Delegate {
         Action::Continue
     }
 
-    /// Given a list of `arguments` to populate to be send to the remote to start negotiating wants and haves.
-    /// `refs` are passed in each round, unchanged.
-    /// `previous_result` will be populated after the first server response has been received and can be used to figure
-    /// out whether to continue negotiating, or be done by returning `Action::Close`. The latter will be used to cause the
-    /// server to send a pack, if possible.
-    fn negotiate(&mut self, refs: &[Ref], arguments: &mut Arguments, previous_result: Option<&Response>) -> Action;
+    /// ### `previous` is None
+    /// Given a list of `arguments` to populate to be send to the remote to start negotiation with wants, shallows, filters and
+    /// other contextual information. This method is called once.
+    /// Send the objects you `have` have afterwards based on your tips, in preparation to walk down their parents with each call
+    /// to `negotiate` in find the common base.
+    /// Note that you should not `want` and object that you already have.
+    /// `refs` are the the tips of on the server side, effectively the latest objects they have.
+    ///
+    /// Return `Action::Close` if you know that there are no `haves` on your end to allow the server to send all of its objects,
+    /// as done during clone.
+    ///
+    /// ### `previous` is Some
+    /// Populate `arguments` with the objects you `have` starting from the tips, taking into consideration the `previous` response of the server to see
+    /// which objects they acknowledged to have. You have to maintain enough state to be able to walk down from your tips on each call,
+    /// if they are not in common, and keep setting `have` for those which are in common if that helps teaching the server about our state.
+    /// This method is called until the other side signals they are ready to send a pack.
+    /// Return `Action::Close` if you want to give up before finding a common base. This can happen if the remote repository has radically changed
+    /// so there are no bases, or they are very far in the past.
+    fn negotiate(&mut self, refs: &[Ref], arguments: &mut Arguments, previous: Option<&Response>) -> Action;
 }

git-protocol/src/fetch/mod.rs

@@ -156,7 +156,8 @@ pub fn fetch<F: FnMut(credentials::Action) -> credentials::Result>(
     }
     let mut arguments = Arguments::new(protocol_version, &fetch_features);
     let previous_response = None::<Response>;
-    // 16? Git does it that way, limiting the amount of lines sent at a time
+    // 16? Git does it that way, limiting the amount of iterations we take.
+    // TODO: Make this a loop and abort after having exchanged a certain amount of objects instead
     for round in 1..=16 {
         progress.step();
         progress.set_name(format!("negotiate (round {})", round));
@@ -167,7 +168,7 @@ pub fn fetch<F: FnMut(credentials::Action) -> credentials::Result>(
             &fetch_features,
             action == Action::Close,
         )?;
-        // TODO: read result in a protocol independent way
+        // TODO: read server response in a protocol independent way
         // match action {
         //     Action::Close {
         //