[clone] Support for reading multi-step negoritaions, but…

…only for V2 this will work correctly. V1 may indeed hang in stateful
connections, unless we know exactly what caused that response.

For now that's acceptable.

Author: Byron

README.md

@@ -445,6 +445,8 @@ From there, we can derive a few rules to try adhere to:
 
 ## Shortcomings
 
+* **fetches using protocol V1 and stateful connections, i.e. ssh, git, file, may hang**
+  * This can be fixed by making response parsing.
 * **lean** and **light** and **small** builds don't support non-UTF-8 paths _in the CLI_
   * This is because they depend on `argh`, which [does not yet support parsing OsStrings](https://github.com/google/argh/issues/33). We however
     believe it eventually will do so and thus don't move on to [`pico-args`](https://github.com/RazrFalcon/pico-args/blob/master/examples/app.rs).

git-packetline/tests/packet_line/reader/read.rs

@@ -102,6 +102,39 @@ fn read_pack_with_progress_extraction() -> crate::Result {
     );
     Ok(())
 }
+#[test]
+fn peek_past_an_actual_eof_is_an_error() -> crate::Result {
+    let input = b"0009ERR e";
+    let mut rd = git_packetline::Provider::new(&input[..], &[]);
+    let mut reader = rd.as_read();
+    assert_eq!(reader.peek_data_line().expect("one line")??, b"ERR e");
+    let mut buf = String::new();
+    reader.read_line(&mut buf)?;
+
+    assert_eq!(
+        reader.peek_data_line().expect("an err").expect_err("foo").kind(),
+        std::io::ErrorKind::UnexpectedEof,
+        "peeking past the end is not an error as the caller should make sure we dont try 'invalid' reads"
+    );
+    Ok(())
+}
+
+#[test]
+fn peek_past_a_delimiter_is_no_error() -> crate::Result {
+    let input = b"0009hello0000";
+    let mut rd = git_packetline::Provider::new(&input[..], &[PacketLine::Flush]);
+    let mut reader = rd.as_read();
+    assert_eq!(reader.peek_data_line().expect("one line")??, b"hello");
+
+    let mut buf = String::new();
+    reader.read_line(&mut buf)?;
+
+    assert!(
+        reader.peek_data_line().is_none(),
+        "peeking past a flush packet is a 'natural' event that shold not cause an error"
+    );
+    Ok(())
+}
 
 #[test]
 fn handling_of_err_lines() {

git-protocol/src/fetch/response.rs

@@ -102,7 +102,17 @@ impl Response {
                 let (acks, has_pack) = 'lines: loop {
                     line.clear();
                     let peeked_line = match reader.peek_data_line() {
-                        Some(line) => String::from_utf8_lossy(line??),
+                        Some(Ok(Ok(line))) => String::from_utf8_lossy(line),
+                        // This special case (block) 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.
+                        // 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 (acks, false),
+                        Some(Err(err)) => return Err(err.into()),
+                        Some(Ok(Err(err))) => return Err(err.into()),
+
                         None => break 'lines (acks, false), // EOF
                     };
 

git-protocol/tests/fetch/response.rs

@@ -31,7 +31,22 @@ mod v1 {
         }
 
         #[test]
-        fn simple_fetch_acks_and_pack() -> crate::Result {
+        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())?;
+            assert_eq!(
+                r.acknowledgements(),
+                &[
+                    Acknowledgement::Common(id("47ee0b7fe4f3a7d776c78794873e6467e1c47e59")),
+                    Acknowledgement::Common(id("3f02c0ad360d96e8dbba92f97b42ebbaa4319db1")),
+                    Acknowledgement::NAK,
+                ]
+            );
+            Ok(())
+        }
+
+        #[test]
+        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)?;
@@ -74,7 +89,15 @@ mod v2 {
         }
 
         #[test]
-        fn simple_fetch_acks_and_pack() -> crate::Result {
+        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())?;
+            assert_eq!(r.acknowledgements(), &[Acknowledgement::NAK,]);
+            Ok(())
+        }
+
+        #[test]
+        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)?;

git-protocol/tests/fixtures/v1/fetch-no-pack.response

@@ -0,0 +1,3 @@
+0038ACK 47ee0b7fe4f3a7d776c78794873e6467e1c47e59 common
+0038ACK 3f02c0ad360d96e8dbba92f97b42ebbaa4319db1 common
+0008NAK

git-protocol/tests/fixtures/v2/fetch-no-pack.response

@@ -0,0 +1,3 @@
+0014acknowledgments
+0008NAK
+0000

gitoxide-core/src/protocol.rs

@@ -30,6 +30,8 @@ impl From<Protocol> for git_transport::Protocol {
 
 impl Default for Protocol {
     fn default() -> Self {
+        // Note that it's very important this remains V2, as V1 may block forver in stateful (i.e. non-http) connections when fetching
+        // as we chose not to complicate matters by counting which arguments where sent (just yet).
         Protocol::V2
     }
 }

tasks.md

@@ -1,13 +1,11 @@
 ### Cloning
-* **git-transport**
- * [ ] a way to support shallow lines during V1 handshake (doesnt' seem to happen in V2 at all)
 * **git-protocol**
   * [x] sideband-all support
-  * [ ] test for multi-stage fetch response (one without a pack file in first round)
-  * [ ] delegate interaction to support clone
+  * [x] test for multi-stage fetch response (one without a pack file in first round)
+  * [x] delegate interaction to support clone
     * [x] parse server negotiation response
     * [x] negotiation via delegate
-    * [ ] received pack file passed to delegate
+    * [x] received pack file passed to delegate
     * [x] assure there is a way to do fetches with have/want negotiation
 * **gixp-pack-receive**
   * [ ] hookup `git-protocol` with delegate to allow for receiving full packs
@@ -21,6 +19,10 @@
 
 ### NEXT ITERATION: Fetching _(more analysis needed after previous block)_
 
+* **git-transport**
+ * [ ] a way to support shallow lines during V1 handshake (doesnt' seem to happen in V2 at all)
+* **prodash**
+  * [ ] force sub-progress 
 * **git-ref**
   * [ ] create ref pointing to ID
       * _assure to keep the path towards symbolic refs open, and allow specifying if these should be followed or not_