change connection API to be consuming, otherwise async mode doesn't work due to lack of async drop (#450)

Author: Byron

Diff for: git-repository/src/remote/connect.rs

@@ -1,5 +1,5 @@
-use crate::remote::{connection, Connection};
-use crate::{remote, Progress, Remote};
+use crate::remote::Connection;
+use crate::{Progress, Remote};
 use git_protocol::transport::client::Transport;
 
 mod error {
@@ -37,7 +37,6 @@ impl<'repo> Remote<'repo> {
             remote: self,
             transport,
             progress,
-            state: connection::State::Connected,
         }
     }
 
@@ -46,13 +45,27 @@ impl<'repo> Remote<'repo> {
     #[git_protocol::maybe_async::maybe_async]
     pub async fn connect<P>(
         &self,
-        direction: remote::Direction,
+        direction: crate::remote::Direction,
         progress: P,
     ) -> Result<Connection<'_, 'repo, Box<dyn Transport + Send>, P>, Error>
     where
         P: Progress,
     {
         use git_protocol::transport::Protocol;
+        fn sanitize(mut url: git_url::Url) -> Result<git_url::Url, Error> {
+            if url.scheme == git_url::Scheme::File {
+                let mut dir = git_path::from_bstr(url.path.as_ref());
+                let kind = git_discover::is_git(dir.as_ref()).or_else(|_| {
+                    dir.to_mut().push(git_discover::DOT_GIT_DIR);
+                    git_discover::is_git(dir.as_ref())
+                })?;
+                let (git_dir, _work_dir) = git_discover::repository::Path::from_dot_git_dir(dir.into_owned(), kind)
+                    .into_repository_and_work_tree_directories();
+                url.path = git_path::into_bstr(git_dir).into_owned();
+            }
+            Ok(url)
+        }
+
         let protocol = self
             .repo
             .config
@@ -72,23 +85,8 @@ impl<'repo> Remote<'repo> {
                 })
             })?;
 
-        let url = self.processed_url(direction)?;
-        let transport = git_protocol::transport::connect(url, protocol).await?;
+        let url = self.url(direction).ok_or(Error::MissingUrl { direction })?.to_owned();
+        let transport = git_protocol::transport::connect(sanitize(url)?, protocol).await?;
         Ok(self.to_connection_with_transport(transport, progress))
     }
-
-    fn processed_url(&self, direction: remote::Direction) -> Result<git_url::Url, Error> {
-        let mut url = self.url(direction).ok_or(Error::MissingUrl { direction })?.to_owned();
-        if url.scheme == git_url::Scheme::File {
-            let mut dir = git_path::from_bstr(url.path.as_ref());
-            let kind = git_discover::is_git(dir.as_ref()).or_else(|_| {
-                dir.to_mut().push(git_discover::DOT_GIT_DIR);
-                git_discover::is_git(dir.as_ref())
-            })?;
-            let (git_dir, _work_dir) = git_discover::repository::Path::from_dot_git_dir(dir.into_owned(), kind)
-                .into_repository_and_work_tree_directories();
-            url.path = git_path::into_bstr(git_dir).into_owned();
-        }
-        Ok(url)
-    }
 }

Diff for: git-repository/src/remote/connection/list_refs.rs

@@ -1,4 +1,4 @@
-use crate::remote::connection::State;
+use crate::remote::connection::HandshakeWithRefs;
 use crate::remote::{Connection, Direction};
 use git_features::progress::Progress;
 use git_protocol::transport::client::Transport;
@@ -10,6 +10,8 @@ mod error {
         Handshake(#[from] git_protocol::fetch::handshake::Error),
         #[error(transparent)]
         ListRefs(#[from] git_protocol::fetch::refs::Error),
+        #[error(transparent)]
+        Transport(#[from] git_protocol::transport::client::Error),
     }
 }
 pub use error::Error;
@@ -22,40 +24,36 @@ where
     /// List all references on the remote.
     ///
     /// Note that this doesn't fetch the objects mentioned in the tips nor does it make any change to underlying repository.
-    ///
-    /// This method is idempotent and only runs once.
     #[git_protocol::maybe_async::maybe_async]
-    pub async fn list_refs(&mut self) -> Result<&[git_protocol::fetch::Ref], Error> {
-        match self.state {
-            State::Connected => {
-                let mut outcome = git_protocol::fetch::handshake(
+    pub async fn list_refs(mut self) -> Result<Vec<git_protocol::fetch::Ref>, Error> {
+        let res = self.fetch_refs().await?;
+        git_protocol::fetch::indicate_end_of_interaction(&mut self.transport).await?;
+        Ok(res.refs)
+    }
+
+    #[git_protocol::maybe_async::maybe_async]
+    async fn fetch_refs(&mut self) -> Result<HandshakeWithRefs, Error> {
+        let mut outcome = git_protocol::fetch::handshake(
+            &mut self.transport,
+            git_protocol::credentials::helper,
+            Vec::new(),
+            &mut self.progress,
+        )
+        .await?;
+        let refs = match outcome.refs.take() {
+            Some(refs) => refs,
+            None => {
+                git_protocol::fetch::refs(
                     &mut self.transport,
-                    git_protocol::credentials::helper,
-                    Vec::new(),
+                    outcome.server_protocol_version,
+                    &outcome.capabilities,
+                    |_a, _b, _c| Ok(git_protocol::fetch::delegate::LsRefsAction::Continue),
                     &mut self.progress,
                 )
-                .await?;
-                let refs = match outcome.refs.take() {
-                    Some(refs) => refs,
-                    None => {
-                        git_protocol::fetch::refs(
-                            &mut self.transport,
-                            outcome.server_protocol_version,
-                            &outcome.capabilities,
-                            |_a, _b, _c| Ok(git_protocol::fetch::delegate::LsRefsAction::Continue),
-                            &mut self.progress,
-                        )
-                        .await?
-                    }
-                };
-                self.state = State::HandshakeWithRefs { outcome, refs };
-                match &self.state {
-                    State::HandshakeWithRefs { refs, .. } => Ok(refs),
-                    _ => unreachable!(),
-                }
+                .await?
             }
-            State::HandshakeWithRefs { ref refs, .. } => Ok(refs),
-        }
+        };
+        Ok(HandshakeWithRefs { outcome, refs })
     }
 
     /// List all references on the remote that have been filtered through our remote's [`refspecs`][crate::Remote::refspecs()]

Diff for: git-repository/src/remote/connection/mod.rs

@@ -1,51 +1,27 @@
 use crate::Remote;
 
-pub(crate) enum State {
-    Connected,
-    HandshakeWithRefs {
-        #[allow(dead_code)]
-        outcome: git_protocol::fetch::handshake::Outcome,
-        refs: Vec<git_protocol::fetch::Ref>,
-    },
+pub(crate) struct HandshakeWithRefs {
+    #[allow(dead_code)]
+    outcome: git_protocol::fetch::handshake::Outcome,
+    refs: Vec<git_protocol::fetch::Ref>,
 }
 
 /// A type to represent an ongoing connection to a remote host, typically with the connection already established.
 ///
 /// It can be used to perform a variety of operations with the remote without worrying about protocol details,
 /// much like a remote procedure call.
-pub struct Connection<'a, 'repo, T, P>
-where
-    T: git_protocol::transport::client::Transport,
-{
+pub struct Connection<'a, 'repo, T, P> {
     pub(crate) remote: &'a Remote<'repo>,
     pub(crate) transport: T,
     pub(crate) progress: P,
-    pub(crate) state: State,
-}
-
-mod impls {
-    use crate::remote::Connection;
-    use git_protocol::transport::client::Transport;
-
-    impl<'a, 'repo, T, P> Drop for Connection<'a, 'repo, T, P>
-    where
-        T: Transport,
-    {
-        fn drop(&mut self) {
-            git_protocol::fetch::indicate_end_of_interaction(&mut self.transport).ok();
-        }
-    }
 }
 
 mod access {
     use crate::remote::Connection;
     use crate::Remote;
 
     /// Access and conversion
-    impl<'a, 'repo, T, P> Connection<'a, 'repo, T, P>
-    where
-        T: git_protocol::transport::client::Transport,
-    {
+    impl<'a, 'repo, T, P> Connection<'a, 'repo, T, P> {
         /// Drop the transport and additional state to regain the original remote.
         pub fn remote(&self) -> &Remote<'repo> {
             self.remote

Diff for: git-repository/tests/remote/list_refs.rs

@@ -8,7 +8,7 @@ mod blocking_io {
     fn all() {
         let repo = remote::repo("clone");
         let remote = repo.find_remote("origin").unwrap();
-        let mut connection = remote.connect(Fetch, progress::Discard).unwrap();
+        let connection = remote.connect(Fetch, progress::Discard).unwrap();
         let refs = connection.list_refs().unwrap();
         assert_eq!(refs.len(), 14, "it gets all remote refs, independently of the refspec.");
     }