git: only try to use ssh-agent once per connection

As reported in #1970, SSH authentication would sometimes run into a
loop where it repeatedly tries to use ssh-agent for authentication
without making progess. The problem can be reproduced by simply
removing `$SSH_AUTH_KEY` from your environment (and not having a Git
credentials helper configured, I think).

This seems to be a bug introduced by b104f8e154c21. That commit meant
to make it so we attempt to use ssh-agent and fall back to using
(password-less) keys after that. The problem is that
`git2::Cred::ssh_key_from_agent()` just returns an object that will be
used later for looking up the credentials from ssh-agent, so the call
will not fail because ssh-agent is not reachable.

This commit attempts to fix the problem by having the credentials
callback attempt to use ssh-agent only once.

CHANGELOG.md

@@ -48,6 +48,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed bugs
 
+* SSH authentication could hang when ssh-agent couldn't be reached 
+  [#1970](https://github.com/martinvonz/jj/issues/1970)
+
 ## [0.8.0] - 2023-07-09
 
 ### Breaking changes

lib/src/git.rs

@@ -878,6 +878,7 @@ impl<'a> RemoteCallbacks<'a> {
         }
         // TODO: We should expose the callbacks to the caller instead -- the library
         // crate shouldn't read environment variables.
+        let mut tried_ssh_agent = false;
         callbacks.credentials(move |url, username_from_url, allowed_types| {
             let span = tracing::debug_span!("RemoteCallbacks.credentials");
             let _ = span.enter();
@@ -890,28 +891,15 @@ impl<'a> RemoteCallbacks<'a> {
                 return Ok(creds);
             } else if let Some(username) = username_from_url {
                 if allowed_types.contains(git2::CredentialType::SSH_KEY) {
-                    // Try to get the SSH key from the agent by default, and report an error
-                    // only if it _seems_ like that's what the user wanted.
-                    //
-                    // Note that the env variables read below are **not** the only way to
-                    // communicate with the agent, which is why we request a key from it no
-                    // matter what.
-                    match git2::Cred::ssh_key_from_agent(username) {
-                        Ok(key) => {
-                            tracing::info!(username, "using ssh_key_from_agent");
-                            return Ok(key);
-                        }
-                        Err(err) => {
-                            if std::env::var("SSH_AUTH_SOCK").is_ok()
-                                || std::env::var("SSH_AGENT_PID").is_ok()
-                            {
-                                tracing::error!(err = %err);
-                                return Err(err);
-                            }
-                            // There is no agent-related env variable so we
-                            // consider that the user doesn't care about using
-                            // the agent and proceed.
-                        }
+                    // Try to get the SSH key from the agent once. We don't even check if
+                    // $SSH_AUTH_SOCK is set because Windows uses another mechanism.
+                    if !tried_ssh_agent {
+                        tracing::info!(username, "using ssh_key_from_agent");
+                        tried_ssh_agent = true;
+                        return git2::Cred::ssh_key_from_agent(username).map_err(|err| {
+                            tracing::error!(err = %err);
+                            err
+                        });
                     }
 
                     if let Some(ref mut cb) = self.get_ssh_key {