Merge branch 'fix-v1-negotiation'

Author: Byron

gix-protocol/src/fetch/response/async_io.rs

@@ -37,10 +37,16 @@ impl Response {
     /// and if `true` we will keep parsing until we get a pack as the client already signalled to the server that it's done.
     /// This way of doing things allows us to exploit knowledge about more recent versions of the protocol, which keeps code easier
     /// and more localized without having to support all the cruft that there is.
+    ///
+    /// `wants_to_negotiate` should be `false` for clones which is when we don't have sent any haves. The reason for this flag to exist
+    /// is to predict how to parse V1 output only, and neither `client_expects_pack` nor `wants_to_negotiate` are relevant for V2.
+    /// This ugliness is in place to avoid having to resort to an [an even more complex ugliness](https://github.com/git/git/blob/9e49351c3060e1fa6e0d2de64505b7becf157f28/fetch-pack.c#L583-L594)
+    /// that `git` has to use to predict how many acks are supposed to be read. We also genuinely hope that this covers it all….
     pub async fn from_line_reader(
         version: Protocol,
         reader: &mut (impl client::ExtendedBufRead + Unpin),
         client_expects_pack: bool,
+        wants_to_negotiate: bool,
     ) -> Result<Response, response::Error> {
         match version {
             Protocol::V0 | Protocol::V1 => {
@@ -89,7 +95,10 @@ impl Response {
                     );
                     // When the server sends ready, we know there is going to be a pack so no need to stop early.
                     saw_ready |= matches!(acks.last(), Some(Acknowledgement::Ready));
-                    if let Some(Acknowledgement::Nak) = acks.last().filter(|_| !client_expects_pack && !saw_ready) {
+                    if let Some(Acknowledgement::Nak) = acks.last().filter(|_| !client_expects_pack || !saw_ready) {
+                        if !wants_to_negotiate {
+                            continue;
+                        }
                         break 'lines false;
                     }
                 };

gix-protocol/src/fetch/response/blocking_io.rs

@@ -37,10 +37,16 @@ impl Response {
     /// and if `true` we will keep parsing until we get a pack as the client already signalled to the server that it's done.
     /// This way of doing things allows us to exploit knowledge about more recent versions of the protocol, which keeps code easier
     /// and more localized without having to support all the cruft that there is.
+    ///
+    /// `wants_to_negotiate` should be `false` for clones which is when we don't have sent any haves. The reason for this flag to exist
+    /// is to predict how to parse V1 output only, and neither `client_expects_pack` nor `wants_to_negotiate` are relevant for V2.
+    /// This ugliness is in place to avoid having to resort to an [an even more complex ugliness](https://github.com/git/git/blob/9e49351c3060e1fa6e0d2de64505b7becf157f28/fetch-pack.c#L583-L594)
+    /// that `git` has to use to predict how many acks are supposed to be read. We also genuinely hope that this covers it all….
     pub fn from_line_reader(
         version: Protocol,
         reader: &mut impl client::ExtendedBufRead,
         client_expects_pack: bool,
+        wants_to_negotiate: bool,
     ) -> Result<Response, response::Error> {
         match version {
             Protocol::V0 | Protocol::V1 => {
@@ -85,7 +91,10 @@ impl Response {
                     assert_ne!(reader.readline_str(&mut line)?, 0, "consuming a peeked line works");
                     // When the server sends ready, we know there is going to be a pack so no need to stop early.
                     saw_ready |= matches!(acks.last(), Some(Acknowledgement::Ready));
-                    if let Some(Acknowledgement::Nak) = acks.last().filter(|_| !client_expects_pack && !saw_ready) {
+                    if let Some(Acknowledgement::Nak) = acks.last().filter(|_| !client_expects_pack || !saw_ready) {
+                        if !wants_to_negotiate {
+                            continue;
+                        }
                         break 'lines false;
                     }
                 };

gix-protocol/src/fetch_fn.rs

@@ -44,6 +44,10 @@ pub enum FetchConnection {
 /// * If `trace` is `true`, all packetlines received or sent will be passed to the facilities of the `gix-trace` crate.
 ///
 /// _Note_ that depending on the `delegate`, the actual action performed can be `ls-refs`, `clone` or `fetch`.
+///
+/// # WARNING - Do not use!
+///
+/// As it will hang when having multiple negotiation rounds.
 #[allow(clippy::result_large_err)]
 #[maybe_async]
 // TODO: remove this without losing test coverage - we have the same but better in `gix` and it's
@@ -134,7 +138,8 @@ where
         let response = Response::from_line_reader(
             protocol_version,
             &mut reader,
-            true, /* hack, telling us we don't want this delegate approach anymore */
+            true,  /* hack, telling us we don't want this delegate approach anymore */
+            false, /* just as much of a hack which causes us to expect a pack immediately */
         )
         .await?;
         previous_response = if response.has_pack() {

gix-protocol/tests/fetch/response.rs

@@ -29,7 +29,7 @@ mod v1 {
         async fn clone() -> crate::Result {
             let mut provider = mock_reader("v1/clone-only.response");
             let mut reader = provider.as_read_without_sidebands();
-            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true, false).await?;
             assert_eq!(r.acknowledgements(), &[Acknowledgement::Nak]);
             assert!(r.has_pack());
             let mut buf = Vec::new();
@@ -42,7 +42,7 @@ mod v1 {
         async fn shallow_clone() -> crate::Result {
             let mut provider = mock_reader("v1/clone-deepen-1.response");
             let mut reader = provider.as_read_without_sidebands();
-            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true, false).await?;
             assert_eq!(
                 r.shallow_updates(),
                 &[ShallowUpdate::Shallow(id("808e50d724f604f69ab93c6da2919c014667bedb"))]
@@ -59,7 +59,7 @@ mod v1 {
         async fn empty_shallow_clone_due_to_depth_being_too_high() -> crate::Result {
             let mut provider = mock_reader("v1/clone-deepen-5.response");
             let mut reader = provider.as_read_without_sidebands();
-            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true, false).await?;
             assert!(r.shallow_updates().is_empty());
             assert_eq!(r.acknowledgements(), &[Acknowledgement::Nak]);
             assert!(r.has_pack());
@@ -73,7 +73,7 @@ mod v1 {
         async fn unshallow_fetch() -> crate::Result {
             let mut provider = mock_reader("v1/fetch-unshallow.response");
             let mut reader = provider.as_read_without_sidebands();
-            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true, true).await?;
             assert_eq!(
                 r.acknowledgements(),
                 &[
@@ -103,8 +103,9 @@ mod v1 {
         #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))]
         async fn fetch_acks_without_pack() -> crate::Result {
             let mut provider = mock_reader("v1/fetch-no-pack.response");
-            let r = fetch::Response::from_line_reader(Protocol::V1, &mut provider.as_read_without_sidebands(), true)
-                .await?;
+            let r =
+                fetch::Response::from_line_reader(Protocol::V1, &mut provider.as_read_without_sidebands(), true, true)
+                    .await?;
             assert_eq!(
                 r.acknowledgements(),
                 &[
@@ -120,7 +121,7 @@ mod v1 {
         async fn fetch_acks_and_pack() -> crate::Result {
             let mut provider = mock_reader("v1/fetch.response");
             let mut reader = provider.as_read_without_sidebands();
-            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true, true).await?;
             assert_eq!(
                 r.acknowledgements(),
                 &[
@@ -223,7 +224,7 @@ mod v2 {
                 );
                 let mut provider = mock_reader(&fixture);
                 let mut reader = provider.as_read_without_sidebands();
-                let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true).await?;
+                let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true, true).await?;
                 assert!(r.acknowledgements().is_empty(), "it should go straight to the packfile");
                 assert!(r.has_pack());
                 reader.set_progress_handler(Some(Box::new(|_is_err, _text| {
@@ -240,7 +241,7 @@ mod v2 {
         async fn shallow_clone() -> crate::Result {
             let mut provider = mock_reader("v2/clone-deepen-1.response");
             let mut reader = provider.as_read_without_sidebands();
-            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true, true).await?;
             assert!(r.acknowledgements().is_empty(), "it should go straight to the packfile");
             assert_eq!(
                 r.shallow_updates(),
@@ -257,7 +258,7 @@ mod v2 {
         async fn unshallow_fetch() -> crate::Result {
             let mut provider = mock_reader("v2/fetch-unshallow.response");
             let mut reader = provider.as_read_without_sidebands();
-            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true, true).await?;
             assert_eq!(
                 r.acknowledgements(),
                 &[
@@ -289,7 +290,7 @@ mod v2 {
         async fn empty_shallow_clone() -> crate::Result {
             let mut provider = mock_reader("v2/clone-deepen-5.response");
             let mut reader = provider.as_read_without_sidebands();
-            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true, true).await?;
             assert!(r.acknowledgements().is_empty(), "it should go straight to the packfile");
             assert!(r.shallow_updates().is_empty(), "it should go straight to the packfile");
             assert!(r.has_pack());
@@ -303,7 +304,7 @@ mod v2 {
         async fn clone_with_sidebands() -> crate::Result {
             let mut provider = mock_reader("v2/clone-only-2.response");
             let mut reader = provider.as_read_without_sidebands();
-            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true, true).await?;
             assert!(r.acknowledgements().is_empty(), "it should go straight to the packfile");
             assert!(r.has_pack());
 
@@ -325,8 +326,9 @@ mod v2 {
         #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))]
         async fn fetch_acks_without_pack() -> crate::Result {
             let mut provider = mock_reader("v2/fetch-no-pack.response");
-            let r = fetch::Response::from_line_reader(Protocol::V2, &mut provider.as_read_without_sidebands(), true)
-                .await?;
+            let r =
+                fetch::Response::from_line_reader(Protocol::V2, &mut provider.as_read_without_sidebands(), true, true)
+                    .await?;
             assert_eq!(r.acknowledgements(), &[Acknowledgement::Nak]);
             Ok(())
         }
@@ -336,7 +338,7 @@ mod v2 {
             let mut provider = mock_reader("v2/fetch-err-line.response");
             provider.fail_on_err_lines(true);
             let mut sidebands = provider.as_read_without_sidebands();
-            match fetch::Response::from_line_reader(Protocol::V2, &mut sidebands, true).await {
+            match fetch::Response::from_line_reader(Protocol::V2, &mut sidebands, true, true).await {
                 Ok(_) => panic!("need error response"),
                 Err(err) => match err {
                     fetch::response::Error::UploadPack(err) => {
@@ -351,7 +353,7 @@ mod v2 {
         async fn fetch_acks_and_pack() -> crate::Result {
             let mut provider = mock_reader("v2/fetch.response");
             let mut reader = provider.as_read_without_sidebands();
-            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true, true).await?;
             assert_eq!(
                 r.acknowledgements(),
                 &[

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

@@ -221,8 +221,13 @@ where
                     if sideband_all {
                         setup_remote_progress(progress, &mut reader, should_interrupt);
                     }
-                    let response =
-                        gix_protocol::fetch::Response::from_line_reader(protocol_version, &mut reader, is_done).await?;
+                    let response = gix_protocol::fetch::Response::from_line_reader(
+                        protocol_version,
+                        &mut reader,
+                        is_done,
+                        !is_done,
+                    )
+                    .await?;
                     let has_pack = response.has_pack();
                     previous_response = Some(response);
                     if has_pack {