From 9255ccc8e043513329f3e78d9b0a2616e807121e Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Sun, 26 Jan 2025 09:24:36 -0500
Subject: [PATCH] Factor client feature check from `connect` to helpers

This creates two helper functions and moves the logic for the SSH
client feature check (determining the `ProgramKind`) to them.

Further refactoring is possible, especially since some of the use
of mutability might be replaceable by early return with no loss of
readability. But for now this maintains the preexisting structure
of the code covering the case where it is not necessary to pre-run
the SSH client.
---
 .../src/client/blocking_io/ssh/mod.rs         | 83 +++++++++++--------
 1 file changed, 50 insertions(+), 33 deletions(-)

diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs
index 2b5967dcc48..83849b1fca7 100644
--- a/gix-transport/src/client/blocking_io/ssh/mod.rs
+++ b/gix-transport/src/client/blocking_io/ssh/mod.rs
@@ -1,8 +1,14 @@
-use std::process::Stdio;
+use std::{
+    ffi::OsStr,
+    process::{Command, Stdio},
+};
 
-use gix_url::ArgumentSafety::*;
+use gix_url::{ArgumentSafety::*, Url};
 
-use crate::{client::blocking_io, Protocol};
+use crate::{
+    client::{self, blocking_io::file::SpawnProcessOnDemand},
+    Protocol,
+};
 
 /// The error used in [`connect()`].
 #[derive(Debug, thiserror::Error)]
@@ -100,44 +106,18 @@ pub mod connect {
 /// If `trace` is `true`, all packetlines received or sent will be passed to the facilities of the `gix-trace` crate.
 #[allow(clippy::result_large_err)]
 pub fn connect(
-    url: gix_url::Url,
+    url: Url,
     desired_version: Protocol,
     options: connect::Options,
     trace: bool,
-) -> Result<blocking_io::file::SpawnProcessOnDemand, Error> {
+) -> Result<SpawnProcessOnDemand, Error> {
     if url.scheme != gix_url::Scheme::Ssh || url.host().is_none() {
         return Err(Error::UnsupportedScheme(url));
     }
     let ssh_cmd = options.ssh_command();
-    let mut kind = options.kind.unwrap_or_else(|| ProgramKind::from(ssh_cmd));
-    if options.kind.is_none() && kind == ProgramKind::Simple {
-        let mut cmd = std::process::Command::from({
-            let mut prepare = gix_command::prepare(ssh_cmd)
-                .stderr(Stdio::null())
-                .stdout(Stdio::null())
-                .stdin(Stdio::null())
-                .command_may_be_shell_script()
-                .arg("-G")
-                .arg(match url.host_as_argument() {
-                    Usable(host) => host,
-                    Dangerous(host) => Err(Error::AmbiguousHostName { host: host.into() })?,
-                    Absent => panic!("BUG: host should always be present in SSH URLs"),
-                });
-            if options.disallow_shell {
-                prepare.use_shell = false;
-            }
-            prepare
-        });
-        gix_features::trace::debug!(cmd = ?cmd, "invoking `ssh` for feature check");
-        kind = if cmd.status().ok().is_some_and(|status| status.success()) {
-            ProgramKind::Ssh
-        } else {
-            ProgramKind::Simple
-        };
-    }
-
+    let kind = determine_client_kind(options.kind, ssh_cmd, &url, options.disallow_shell)?;
     let path = gix_url::expand_path::for_shell(url.path.clone());
-    Ok(blocking_io::file::SpawnProcessOnDemand::new_ssh(
+    Ok(SpawnProcessOnDemand::new_ssh(
         url,
         ssh_cmd,
         path,
@@ -148,5 +128,42 @@ pub fn connect(
     ))
 }
 
+fn determine_client_kind(
+    known_kind: Option<ProgramKind>,
+    ssh_cmd: &OsStr,
+    url: &Url,
+    disallow_shell: bool,
+) -> Result<ProgramKind, Error> {
+    let mut kind = known_kind.unwrap_or_else(|| ProgramKind::from(ssh_cmd));
+    if known_kind.is_none() && kind == ProgramKind::Simple {
+        let mut cmd = build_client_feature_check_command(ssh_cmd, url, disallow_shell)?;
+        gix_features::trace::debug!(cmd = ?cmd, "invoking `ssh` for feature check");
+        kind = if cmd.status().ok().is_some_and(|status| status.success()) {
+            ProgramKind::Ssh
+        } else {
+            ProgramKind::Simple
+        };
+    }
+    Ok(kind)
+}
+
+fn build_client_feature_check_command(ssh_cmd: &OsStr, url: &Url, disallow_shell: bool) -> Result<Command, Error> {
+    let mut prepare = gix_command::prepare(ssh_cmd)
+        .stderr(Stdio::null())
+        .stdout(Stdio::null())
+        .stdin(Stdio::null())
+        .command_may_be_shell_script()
+        .arg("-G")
+        .arg(match url.host_as_argument() {
+            Usable(host) => host,
+            Dangerous(host) => Err(Error::AmbiguousHostName { host: host.into() })?,
+            Absent => panic!("BUG: host should always be present in SSH URLs"),
+        });
+    if disallow_shell {
+        prepare.use_shell = false;
+    }
+    Ok(prepare.into())
+}
+
 #[cfg(test)]
 mod tests;