refactor: allow passing OsStr as-is to exec_cmd (#2997)

src/context.rs

@@ -9,7 +9,8 @@ use git2::{ErrorCode::UnbornBranch, Repository, RepositoryState};
 use once_cell::sync::OnceCell;
 use std::collections::{HashMap, HashSet};
 use std::env;
-use std::ffi::OsString;
+use std::ffi::{OsStr, OsString};
+use std::fmt::Debug;
 use std::fs;
 use std::path::{Path, PathBuf};
 use std::string::String;
@@ -266,18 +267,19 @@ impl<'a> Context<'a> {
 
     /// Execute a command and return the output on stdout and stderr if successful
     #[inline]
-    pub fn exec_cmd(&self, cmd: &str, args: &[&str]) -> Option<CommandOutput> {
+    pub fn exec_cmd<T: AsRef<OsStr> + Debug, U: AsRef<OsStr> + Debug>(
+        &self,
+        cmd: T,
+        args: &[U],
+    ) -> Option<CommandOutput> {
         #[cfg(test)]
         {
-            let command = match args.len() {
+            let command = crate::utils::display_command(&cmd, args);
-                0 => cmd.to_owned(),
-                _ => format!("{} {}", cmd, args.join(" ")),
-            };
             if let Some(output) = self.cmd.get(command.as_str()) {
                 return output.clone();
             }
         }
-        exec_cmd(cmd, args, self.cmd_timeout)
+        exec_cmd(&cmd, args, self.cmd_timeout)
     }
 }
 

src/modules/git_metrics.rs

@@ -1,4 +1,5 @@
 use regex::Regex;
+use std::ffi::OsStr;
 
 use crate::{
     config::RootModuleConfig, configs::git_metrics::GitMetricsConfig, formatter::StringFormatter,
@@ -26,11 +27,11 @@ pub fn module<'a>(context: &'a Context) -> Option<Module<'a>> {
         .exec_cmd(
             "git",
             &[
-                "-C",
+                OsStr::new("-C"),
-                &repo_root.to_string_lossy(),
+                repo_root.as_os_str(),
-                "--no-optional-locks",
+                OsStr::new("--no-optional-locks"),
-                "diff",
+                OsStr::new("diff"),
-                "--shortstat",
+                OsStr::new("--shortstat"),
             ],
         )?
         .stdout;

src/modules/git_status.rs

@@ -6,6 +6,7 @@ use super::{Context, Module, RootModuleConfig};
 use crate::configs::git_status::GitStatusConfig;
 use crate::formatter::StringFormatter;
 use crate::segment::Segment;
+use std::ffi::OsStr;
 use std::sync::Arc;
 
 const ALL_STATUS_FORMAT: &str = "$conflicted$stashed$deleted$renamed$modified$staged$untracked";
@@ -184,12 +185,12 @@ fn get_repo_status(context: &Context) -> Option<RepoStatus> {
     let status_output = context.exec_cmd(
         "git",
         &[
-            "-C",
+            OsStr::new("-C"),
-            &context.current_dir.to_string_lossy(),
+            context.current_dir.as_os_str(),
-            "--no-optional-locks",
+            OsStr::new("--no-optional-locks"),
-            "status",
+            OsStr::new("status"),
-            "--porcelain=2",
+            OsStr::new("--porcelain=2"),
-            "--branch",
+            OsStr::new("--branch"),
         ],
     )?;
     let statuses = status_output.stdout.lines();
@@ -209,11 +210,11 @@ fn get_stashed_count(context: &Context) -> Option<usize> {
     let stash_output = context.exec_cmd(
         "git",
         &[
-            "-C",
+            OsStr::new("-C"),
-            &context.current_dir.to_string_lossy(),
+            context.current_dir.as_os_str(),
-            "--no-optional-locks",
+            OsStr::new("--no-optional-locks"),
-            "stash",
+            OsStr::new("stash"),
-            "list",
+            OsStr::new("list"),
         ],
     )?;
 
@@ -320,6 +321,7 @@ fn format_symbol(format_str: &str, config_path: &str) -> Option<Vec<Segment>> {
 #[cfg(test)]
 mod tests {
     use ansi_term::{ANSIStrings, Color};
+    use std::ffi::OsStr;
     use std::fs::{self, File};
     use std::io::{self, prelude::*};
     use std::path::Path;
@@ -829,9 +831,9 @@ mod tests {
 
         create_command("git")?
             .args(&[
-                "config",
+                OsStr::new("config"),
-                "core.worktree",
+                OsStr::new("core.worktree"),
-                &worktree_dir.path().to_string_lossy(),
+                worktree_dir.path().as_os_str(),
             ])
             .current_dir(repo_dir.path())
             .output()?;

src/utils.rs

@@ -72,18 +72,35 @@ impl PartialEq for CommandOutput {
     }
 }
 
+#[cfg(test)]
+pub fn display_command<T: AsRef<OsStr> + Debug, U: AsRef<OsStr> + Debug>(
+    cmd: T,
+    args: &[U],
+) -> String {
+    std::iter::once(cmd.as_ref())
+        .chain(args.iter().map(|i| i.as_ref()))
+        .map(|i| i.to_string_lossy().into_owned())
+        .collect::<Vec<String>>()
+        .join(" ")
+}
+
 /// Execute a command and return the output on stdout and stderr if successful
 #[cfg(not(test))]
-pub fn exec_cmd(cmd: &str, args: &[&str], time_limit: Duration) -> Option<CommandOutput> {
+pub fn exec_cmd<T: AsRef<OsStr> + Debug, U: AsRef<OsStr> + Debug>(
+    cmd: T,
+    args: &[U],
+    time_limit: Duration,
+) -> Option<CommandOutput> {
     internal_exec_cmd(cmd, args, time_limit)
 }
 
 #[cfg(test)]
-pub fn exec_cmd(cmd: &str, args: &[&str], time_limit: Duration) -> Option<CommandOutput> {
+pub fn exec_cmd<T: AsRef<OsStr> + Debug, U: AsRef<OsStr> + Debug>(
-    let command = match args.len() {
+    cmd: T,
-        0 => String::from(cmd),
+    args: &[U],
-        _ => format!("{} {}", cmd, args.join(" ")),
+    time_limit: Duration,
-    };
+) -> Option<CommandOutput> {
+    let command = display_command(&cmd, args);
     match command.as_str() {
         "crystal --version" => Some(CommandOutput {
             stdout: String::from(
@@ -280,7 +297,7 @@ CMake suite maintained and supported by Kitware (kitware.com/cmake).\n",
             stderr: String::default(),
         }),
         // If we don't have a mocked command fall back to executing the command
-        _ => internal_exec_cmd(cmd, args, time_limit),
+        _ => internal_exec_cmd(&cmd, args, time_limit),
     }
 }
 
@@ -345,10 +362,14 @@ pub fn wrap_seq_for_shell(
     final_string
 }
 
-fn internal_exec_cmd(cmd: &str, args: &[&str], time_limit: Duration) -> Option<CommandOutput> {
+fn internal_exec_cmd<T: AsRef<OsStr> + Debug, U: AsRef<OsStr> + Debug>(
+    cmd: T,
+    args: &[U],
+    time_limit: Duration,
+) -> Option<CommandOutput> {
     log::trace!("Executing command {:?} with args {:?}", cmd, args);
 
-    let full_path = match which::which(cmd) {
+    let full_path = match which::which(&cmd) {
         Ok(full_path) => {
             log::trace!("Using {:?} as {:?}", full_path, cmd);
             full_path
@@ -483,7 +504,11 @@ mod tests {
 
     #[test]
     fn exec_mocked_command() {
-        let result = exec_cmd("dummy_command", &[], Duration::from_millis(500));
+        let result = exec_cmd(
+            "dummy_command",
+            &[] as &[&OsStr],
+            Duration::from_millis(500),
+        );
         let expected = Some(CommandOutput {
             stdout: String::from("stdout ok!\n"),
             stderr: String::from("stderr ok!\n"),
@@ -498,7 +523,7 @@ mod tests {
     #[test]
     #[cfg(not(windows))]
     fn exec_no_output() {
-        let result = internal_exec_cmd("true", &[], Duration::from_millis(500));
+        let result = internal_exec_cmd("true", &[] as &[&OsStr], Duration::from_millis(500));
         let expected = Some(CommandOutput {
             stdout: String::from(""),
             stderr: String::from(""),
@@ -555,7 +580,7 @@ mod tests {
     #[test]
     #[cfg(not(windows))]
     fn exec_with_non_zero_exit_code() {
-        let result = internal_exec_cmd("false", &[], Duration::from_millis(500));
+        let result = internal_exec_cmd("false", &[] as &[&OsStr], Duration::from_millis(500));
         let expected = None;
 
         assert_eq!(result, expected)