Skip to content

Commit

Permalink
Factor client feature check from connect to helpers
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
EliahKagan committed Jan 26, 2025
1 parent a445b72 commit 9255ccc
Showing 1 changed file with 50 additions and 33 deletions.
83 changes: 50 additions & 33 deletions gix-transport/src/client/blocking_io/ssh/mod.rs
Original file line number Diff line number Diff line change
@@ -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)]
Expand Down Expand Up @@ -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,
Expand All @@ -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;

0 comments on commit 9255ccc

Please sign in to comment.