[ref-ls] don't do anything on drop

This is not actually required and may even be a problem as it would
trigger even though we are aborting, quitting.

Author: Byron

git-packetline/tests/packet_line/write.rs

@@ -46,7 +46,7 @@ fn empty_writes_fail_with_error() {
 }
 
 #[test]
-fn nothing_happens_on_drop() {
+fn nothing_happens_on_into_read() {
     let mut out = Vec::new();
     let w = Writer::new(&mut out);
     drop(w);

git-protocol/src/fetch/arguments.rs

@@ -121,14 +121,15 @@ impl Arguments {
         }
         match version {
             git_transport::Protocol::V1 => {
-                let mut on_drop = if add_done_argument {
-                    vec![client::MessageKind::Text(&b"done"[..])]
+                let on_into_read = if add_done_argument {
+                    client::MessageKind::Text(&b"done"[..])
                 } else {
-                    vec![client::MessageKind::Flush]
+                    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)?;
+                let mut line_writer =
+                    transport.request(client::WriteMode::OneLFTerminatedLinePerWriteCall, on_into_read)?;
 
                 for arg in self.args.drain(..) {
                     line_writer.write_all(&arg)?;
@@ -140,7 +141,7 @@ impl Arguments {
                 if let Some(next_args) = retained_state {
                     self.args = next_args;
                 }
-                Ok(line_writer.into_read())
+                Ok(line_writer.into_read()?)
             }
             git_transport::Protocol::V2 => {
                 let mut arguments = std::mem::replace(&mut self.args, self.base_args.clone());

git-transport/src/client/connect.rs

@@ -45,8 +45,8 @@ mod box_impl {
             self.deref_mut().set_identity(identity)
         }
 
-        fn request(&mut self, write_mode: WriteMode, on_drop: Vec<MessageKind>) -> Result<RequestWriter, Error> {
-            self.deref_mut().request(write_mode, on_drop)
+        fn request(&mut self, write_mode: WriteMode, on_into_read: MessageKind) -> Result<RequestWriter, Error> {
+            self.deref_mut().request(write_mode, on_into_read)
         }
 
         fn close(&mut self) -> Result<(), Error> {

git-transport/src/client/file.rs

@@ -114,11 +114,11 @@ impl client::Transport for SpawnProcessOnDemand {
         c.handshake(service)
     }
 
-    fn request(&mut self, write_mode: WriteMode, on_drop: Vec<MessageKind>) -> Result<RequestWriter, client::Error> {
+    fn request(&mut self, write_mode: WriteMode, on_into_read: MessageKind) -> Result<RequestWriter, client::Error> {
         self.connection
             .as_mut()
             .expect("handshake() to have been called first")
-            .request(write_mode, on_drop)
+            .request(write_mode, on_into_read)
     }
 
     fn close(&mut self) -> Result<(), client::Error> {

git-transport/src/client/git.rs

@@ -93,13 +93,13 @@ where
     fn request(
         &mut self,
         write_mode: client::WriteMode,
-        on_drop: Vec<client::MessageKind>,
+        on_into_read: client::MessageKind,
     ) -> Result<client::RequestWriter, client::Error> {
         Ok(client::RequestWriter::new_from_bufread(
             &mut self.writer,
             Box::new(self.line_provider.as_read_without_sidebands()),
             write_mode,
-            on_drop,
+            on_into_read,
         ))
     }
 

git-transport/src/client/http/mod.rs

@@ -142,7 +142,7 @@ impl<H: Http> client::Transport for Transport<H> {
     fn request(
         &mut self,
         write_mode: client::WriteMode,
-        on_drop: Vec<client::MessageKind>,
+        on_into_read: client::MessageKind,
     ) -> Result<client::RequestWriter, client::Error> {
         let service = self.service.expect("handshake() must have been called first");
         let url = append_url(&self.url, service.as_str());
@@ -179,7 +179,7 @@ impl<H: Http> client::Transport for Transport<H> {
                 body: line_provider.as_read_without_sidebands(),
             }),
             write_mode,
-            on_drop,
+            on_into_read,
         ))
     }
 

git-transport/src/client/mod.rs

@@ -91,7 +91,8 @@ pub enum Identity {
 
 /// A type implementing `Write`, which when done can be transformed into a `Read` for obtaining the response.
 pub struct RequestWriter<'a> {
-    pub(crate) writer: WritePacketOnDrop<Box<dyn io::Write + 'a>>,
+    on_into_read: MessageKind,
+    pub(crate) writer: git_packetline::Writer<Box<dyn io::Write + 'a>>,
     pub(crate) reader: Box<dyn ExtendedBufRead + 'a>,
 }
 
@@ -110,24 +111,32 @@ impl<'a> RequestWriter<'a> {
         writer: W,
         reader: Box<dyn ExtendedBufRead + 'a>,
         write_mode: WriteMode,
-        on_drop: Vec<MessageKind>,
+        on_into_read: MessageKind,
     ) -> Self {
         let mut writer = git_packetline::Writer::new(Box::new(writer) as Box<dyn io::Write>);
         match write_mode {
             WriteMode::Binary => writer.enable_binary_mode(),
             WriteMode::OneLFTerminatedLinePerWriteCall => writer.enable_text_mode(),
         }
         RequestWriter {
-            writer: WritePacketOnDrop::new(writer, on_drop),
+            on_into_read,
+            writer,
             reader,
         }
     }
-    pub fn into_read(self) -> Box<dyn ExtendedBufRead + 'a> {
-        self.reader
+    pub fn into_read(mut self) -> io::Result<Box<dyn ExtendedBufRead + 'a>> {
+        self.write_message(self.on_into_read)?;
+        Ok(self.reader)
     }
 
-    pub fn write_message(&mut self, kind: MessageKind) -> io::Result<()> {
-        self.writer.write_message(kind)
+    pub fn write_message(&mut self, message: MessageKind) -> io::Result<()> {
+        match message {
+            MessageKind::Flush => git_packetline::PacketLine::Flush.to_write(&mut self.writer.inner),
+            MessageKind::Delimiter => git_packetline::PacketLine::Delimiter.to_write(&mut self.writer.inner),
+            MessageKind::Text(t) => git_packetline::borrowed::Text::from(t).to_write(&mut self.writer.inner),
+        }
+        .map(|_| ())
+        .map_err(|err| io::Error::new(io::ErrorKind::Other, err))
     }
 }
 
@@ -143,47 +152,6 @@ impl<'a, T: io::Read> ExtendedBufRead for git_packetline::provider::ReadWithSide
 
 pub type HandleProgress = Box<dyn FnMut(bool, &[u8])>;
 
-pub(crate) struct WritePacketOnDrop<W: io::Write> {
-    inner: git_packetline::Writer<W>,
-    on_drop: Vec<MessageKind>,
-}
-
-impl<W: io::Write> WritePacketOnDrop<W> {
-    pub fn new(inner: git_packetline::Writer<W>, on_drop: Vec<MessageKind>) -> Self {
-        WritePacketOnDrop { inner, on_drop }
-    }
-
-    pub fn write_message(&mut self, message: MessageKind) -> io::Result<()> {
-        match message {
-            MessageKind::Flush => git_packetline::PacketLine::Flush.to_write(&mut self.inner.inner),
-            MessageKind::Delimiter => git_packetline::PacketLine::Delimiter.to_write(&mut self.inner.inner),
-            MessageKind::Text(t) => git_packetline::borrowed::Text::from(t).to_write(&mut self.inner.inner),
-        }
-        .map(|_| ())
-        .map_err(|err| io::Error::new(io::ErrorKind::Other, err))
-    }
-}
-
-impl<W: io::Write> io::Write for WritePacketOnDrop<W> {
-    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
-        self.inner.write(buf)
-    }
-
-    fn flush(&mut self) -> io::Result<()> {
-        self.inner.flush()
-    }
-}
-
-impl<W: io::Write> Drop for WritePacketOnDrop<W> {
-    fn drop(&mut self) {
-        let mut on_drop = std::mem::take(&mut self.on_drop);
-        for msg in on_drop.drain(..) {
-            self.write_message(msg)
-                .expect("packet line write on drop must work or we may as well panic to prevent weird surprises");
-        }
-    }
-}
-
 /// All methods provided here must be called in the correct order according to the communication protocol used to connect to them.
 /// It does, however, know just enough to be able to provide a higher-level interface than would otherwise be possible.
 /// Thus the consumer of this trait will not have to deal with packet lines at all.
@@ -208,10 +176,10 @@ pub trait Transport {
     }
     /// Obtain a writer for sending data and obtaining the response. It can be configured in various ways,
     /// and should to support with the task at hand.
-    /// `send_mode` determines how calls to the `write(…)` method are interpreted, and `on_drop` determines
-    /// which messages to write when the writer is dropped. This happens naturally when switching to reading the response with `into_read()`.
+    /// `send_mode` determines how calls to the `write(…)` method are interpreted, and `on_into_read` determines
+    /// which message to write when the writer is turned into the response reader using `into_read()`.
     /// If `handle_progress` is not None, it's function passed a text line without trailing LF from which progress information can be parsed.
-    fn request(&mut self, write_mode: WriteMode, on_drop: Vec<MessageKind>) -> Result<RequestWriter, Error>;
+    fn request(&mut self, write_mode: WriteMode, on_into_read: MessageKind) -> Result<RequestWriter, Error>;
 
     /// Closes the connection to indicate no further requests will be made.
     fn close(&mut self) -> Result<(), Error>;
@@ -253,7 +221,7 @@ impl<T: Transport> TransportV2Ext for T {
         capabilities: impl IntoIterator<Item = (&'a str, Option<&'a str>)>,
         arguments: Option<impl IntoIterator<Item = BString>>,
     ) -> Result<Box<dyn ExtendedBufRead + '_>, Error> {
-        let mut writer = self.request(WriteMode::OneLFTerminatedLinePerWriteCall, vec![MessageKind::Flush])?;
+        let mut writer = self.request(WriteMode::OneLFTerminatedLinePerWriteCall, MessageKind::Flush)?;
         writer.write_all(format!("command={}", command).as_bytes())?;
         for (name, value) in capabilities {
             match value {
@@ -267,6 +235,6 @@ impl<T: Transport> TransportV2Ext for T {
                 writer.write_all(argument.as_ref())?;
             }
         }
-        Ok(writer.into_read())
+        Ok(writer.into_read()?)
     }
 }

git-transport/tests/client/git.rs

@@ -72,18 +72,18 @@ fn handshake_v1_and_request() -> crate::Result {
     );
     drop(res);
 
-    let writer = c.request(client::WriteMode::Binary, Vec::new())?;
-    assert_eq!(writer.into_read().lines().next().expect("exactly one line")?, "NAK");
+    let writer = c.request(client::WriteMode::Binary, client::MessageKind::Flush)?;
+    assert_eq!(writer.into_read()?.lines().next().expect("exactly one line")?, "NAK");
 
     let mut writer = c.request(
         client::WriteMode::OneLFTerminatedLinePerWriteCall,
-        vec![client::MessageKind::Flush, client::MessageKind::Text(b"done")],
+        client::MessageKind::Text(b"done"),
     )?;
 
     writer.write_all(b"hello")?;
     writer.write_all(b"world")?;
 
-    let mut reader = writer.into_read();
+    let mut reader = writer.into_read()?;
     let messages = Rc::new(RefCell::new(Vec::<String>::new()));
     reader.set_progress_handler(Some(Box::new({
         let sb = messages.clone();
@@ -105,9 +105,9 @@ fn handshake_v1_and_request() -> crate::Result {
 
     assert_eq!(
         out.as_slice().as_bstr(),
-        b"002egit-upload-pack /foo.git\0host=example.org\0000ahello\n\
+        b"002egit-upload-pack /foo.git\0host=example.org\00000000ahello\n\
         000aworld\n\
-        00000009done\n0000"
+        0009done\n0000"
             .as_bstr(),
         "it sends the correct request"
     );

git-transport/tests/client/http/mod.rs

@@ -64,7 +64,7 @@ Authorization: Basic dXNlcjpwYXNzd29yZA==
     );
 
     server.next_read_and_respond_with(fixture_bytes("v1/http-handshake.response"));
-    client.request(client::WriteMode::Binary, Vec::new())?;
+    client.request(client::WriteMode::Binary, client::MessageKind::Flush)?;
 
     assert_eq!(
         server.received_as_string().lines().collect::<Vec<_>>(),
@@ -238,12 +238,12 @@ fn clone_v1() -> crate::Result {
     server.next_read_and_respond_with(fixture_bytes("v1/http-clone.response"));
     let mut writer = c.request(
         client::WriteMode::OneLFTerminatedLinePerWriteCall,
-        vec![client::MessageKind::Flush, client::MessageKind::Text(b"done")],
+        client::MessageKind::Text(b"done"),
     )?;
     writer.write_all(b"hello")?;
     writer.write_all(b"world")?;
 
-    let mut reader = writer.into_read();
+    let mut reader = writer.into_read()?;
     let mut line = String::new();
     reader.read_line(&mut line)?;
     assert_eq!(line, "NAK\n", "we receive a NAK in text mode before the PACK is sent");
@@ -275,10 +275,10 @@ User-Agent: git/oxide-{}
 Content-Type: application/x-git-upload-pack-request
 Accept: application/x-git-upload-pack-result
 
-21
+1d
 000ahello
 000aworld
-00000009done
+0009done
 
 0