fix!: make V1 stateless negotations work.

This is done by working around another V1 negotiation oddity
by exploiting client-side knowledge. For very old servers, we probably
wouldn't be able to do multi-rounds without dead-locking, but with
recent-enough (probably 10 years or so) old git servers all should
work fine.

All this to not actually have to implement the V1 strangeness, allowing
our code to work smoothly with all permutations of stateless/stateful connections
and V1/V2 interactions, with a single high-level implementation essentially.

Author: Byron

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

@@ -32,9 +32,15 @@ async fn parse_v2_section<T>(
 
 impl Response {
     /// Parse a response of the given `version` of the protocol from `reader`.
+    ///
+    /// `client_expects_pack` is only relevant for V1 stateful connections, and if `false`, causes us to stop parsing when seeing `NAK`,
+    /// 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.
     pub async fn from_line_reader(
         version: Protocol,
         reader: &mut (impl client::ExtendedBufRead + Unpin),
+        client_expects_pack: bool,
     ) -> Result<Response, response::Error> {
         match version {
             Protocol::V0 | Protocol::V1 => {
@@ -48,8 +54,8 @@ impl Response {
                         // This special case (hang/block forever) deals with a single NAK being a legitimate EOF sometimes
                         // Note that this might block forever in stateful connections as there it's not really clear
                         // if something will be following or not by just looking at the response. Instead you have to know
-                        // the arguments sent to the server and count response lines based on intricate knowledge on how the
-                        // server works.
+                        // [a lot](https://github.com/git/git/blob/9e49351c3060e1fa6e0d2de64505b7becf157f28/fetch-pack.c#L583-L594)
+                        // to deal with this correctly.
                         // For now this is acceptable, as V2 can be used as a workaround, which also is the default.
                         Some(Err(err)) if err.kind() == io::ErrorKind::UnexpectedEof => break 'lines false,
                         Some(Err(err)) => return Err(err.into()),
@@ -75,6 +81,9 @@ impl Response {
                     if Response::parse_v1_ack_or_shallow_or_assume_pack(&mut acks, &mut shallows, &peeked_line) {
                         break 'lines true;
                     }
+                    if let Some(Acknowledgement::Nak) = acks.last().filter(|_| !client_expects_pack) {
+                        break 'lines false;
+                    }
                     assert_ne!(
                         reader.readline_str(&mut line).await?,
                         0,

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

@@ -32,9 +32,15 @@ fn parse_v2_section<T>(
 
 impl Response {
     /// Parse a response of the given `version` of the protocol from `reader`.
+    ///
+    /// `client_expects_pack` is only relevant for V1 stateful connections, and if `false`, causes us to stop parsing when seeing `NAK`,
+    /// 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.
     pub fn from_line_reader(
         version: Protocol,
         reader: &mut impl client::ExtendedBufRead,
+        client_expects_pack: bool,
     ) -> Result<Response, response::Error> {
         match version {
             Protocol::V0 | Protocol::V1 => {
@@ -48,8 +54,8 @@ impl Response {
                         // This special case (hang/block forever) deals with a single NAK being a legitimate EOF sometimes
                         // Note that this might block forever in stateful connections as there it's not really clear
                         // if something will be following or not by just looking at the response. Instead you have to know
-                        // the arguments sent to the server and count response lines based on intricate knowledge on how the
-                        // server works.
+                        // [a lot](https://github.com/git/git/blob/9e49351c3060e1fa6e0d2de64505b7becf157f28/fetch-pack.c#L583-L594)
+                        // to deal with this correctly.
                         // For now this is acceptable, as V2 can be used as a workaround, which also is the default.
                         Some(Err(err)) if err.kind() == io::ErrorKind::UnexpectedEof => break 'lines false,
                         Some(Err(err)) => return Err(err.into()),
@@ -75,6 +81,9 @@ impl Response {
                     if Response::parse_v1_ack_or_shallow_or_assume_pack(&mut acks, &mut shallows, &peeked_line) {
                         break 'lines true;
                     }
+                    if let Some(Acknowledgement::Nak) = acks.last().filter(|_| !client_expects_pack) {
+                        break 'lines false;
+                    }
                     assert_ne!(reader.readline_str(&mut line)?, 0, "consuming a peeked line works");
                 };
                 Ok(Response {

gix-protocol/src/fetch_fn.rs

@@ -128,7 +128,12 @@ where
         if sideband_all {
             setup_remote_progress(&mut progress, &mut reader);
         }
-        let response = Response::from_line_reader(protocol_version, &mut reader).await?;
+        let response = Response::from_line_reader(
+            protocol_version,
+            &mut reader,
+            true, /* hack, telling us we don't want this delegate approach anymore */
+        )
+        .await?;
         previous_response = if response.has_pack() {
             progress.step();
             progress.set_name("receiving 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).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true).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).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true).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).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true).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).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true).await?;
             assert_eq!(
                 r.acknowledgements(),
                 &[
@@ -103,7 +103,8 @@ 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()).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V1, &mut provider.as_read_without_sidebands(), true)
+                .await?;
             assert_eq!(
                 r.acknowledgements(),
                 &[
@@ -119,7 +120,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).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V1, &mut reader, true).await?;
             assert_eq!(
                 r.acknowledgements(),
                 &[
@@ -218,7 +219,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).await?;
+                let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, 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| {
@@ -235,7 +236,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).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true).await?;
             assert!(r.acknowledgements().is_empty(), "it should go straight to the packfile");
             assert_eq!(
                 r.shallow_updates(),
@@ -252,7 +253,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).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true).await?;
             assert_eq!(
                 r.acknowledgements(),
                 &[
@@ -284,7 +285,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).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, 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());
@@ -298,7 +299,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).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true).await?;
             assert!(r.acknowledgements().is_empty(), "it should go straight to the packfile");
             assert!(r.has_pack());
 
@@ -320,7 +321,8 @@ 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()).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V2, &mut provider.as_read_without_sidebands(), true)
+                .await?;
             assert_eq!(r.acknowledgements(), &[Acknowledgement::Nak]);
             Ok(())
         }
@@ -330,7 +332,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).await {
+            match fetch::Response::from_line_reader(Protocol::V2, &mut sidebands, true).await {
                 Ok(_) => panic!("need error response"),
                 Err(err) => match err {
                     fetch::response::Error::UploadPack(err) => {
@@ -345,7 +347,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).await?;
+            let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader, true).await?;
             assert_eq!(
                 r.acknowledgements(),
                 &[