Merge pull request #1800 from GitoxideLabs/improvements

various improvements

Author: Byron

gix-command/src/lib.rs

@@ -93,27 +93,41 @@ mod prepare {
 
     /// Builder
     impl Prepare {
-        /// If called, the command will not be executed directly, but with `sh`, but only if the
-        /// command passed to [`prepare`](super::prepare()) requires this.
+        /// If called, the command will be checked for characters that are typical for shell scripts, and if found will use `sh` to execute it
+        /// or whatever is set as [`with_shell_program()`](Self::with_shell_program()).
+        /// If the command isn't valid UTF-8, a shell will always be used.
         ///
-        /// This also allows to pass shell scripts as command, or use commands that contain arguments which are subsequently
-        /// parsed by `sh`.
-        pub fn with_shell(mut self) -> Self {
+        /// If a shell is used, then arguments given here with [arg()](Self::arg) or [args()](Self::args) will be substituted via `"$@"` if it's not
+        /// already present in the command.
+        ///
+        /// If this method is not called, commands are always executed verbatim, without the use of a shell.
+        pub fn command_may_be_shell_script(mut self) -> Self {
             self.use_shell = self.command.to_str().map_or(true, |cmd| {
                 cmd.as_bytes().find_byteset(b"|&;<>()$`\\\"' \t\n*?[#~=%").is_some()
             });
             self
         }
 
-        /// Set the name or path to the shell `program` to use, to avoid using the default shell which is `sh`.
+        /// If called, unconditionally use a shell to execute the command and its arguments, and `sh` to execute it,
+        /// or whatever is set as [`with_shell_program()`](Self::with_shell_program()).
+        ///
+        /// If a shell is used, then arguments given here with [arg()](Self::arg) or [args()](Self::args) will be substituted via `"$@"` if it's not
+        /// already present in the command.
+        ///
+        /// If this method is not called, commands are always executed verbatim, without the use of a shell.
+        pub fn with_shell(mut self) -> Self {
+            self.use_shell = true;
+            self
+        }
+        /// Set the name or path to the shell `program` to use if a shell is to be used, to avoid using the default shell which is `sh`.
         pub fn with_shell_program(mut self, program: impl Into<OsString>) -> Self {
             self.shell_program = Some(program.into());
             self
         }
 
         /// Unconditionally turn off using the shell when spawning the command.
         /// Note that not using the shell is the default so an effective use of this method
-        /// is some time after [`with_shell()`][Prepare::with_shell()] was called.
+        /// is some time after [`command_may_be_shell_script()`](Prepare::command_may_be_shell_script()) was called.
         pub fn without_shell(mut self) -> Self {
             self.use_shell = false;
             self
@@ -128,18 +142,20 @@ mod prepare {
             self
         }
 
-        /// Use a shell, but try to split arguments by hand if this can be safely done without a shell.
+        /// Like [`command_may_be_shell_script()`](Prepare::command_may_be_shell_script()), but try to split arguments by hand if this can be safely done without a shell.
         ///
-        /// If that's not the case, use a shell instead.
-        pub fn with_shell_allow_manual_argument_splitting(mut self) -> Self {
+        /// This is useful on platforms where spawning processes is slow, or where many processes have to be spawned in a raw which should be sped up.
+        /// Manual argument splitting is enabled by default on Windows only.
+        pub fn command_may_be_shell_script_allow_manual_argument_splitting(mut self) -> Self {
             self.allow_manual_arg_splitting = true;
-            self.with_shell()
+            self.command_may_be_shell_script()
         }
 
-        /// Use a shell, but prohibit splitting arguments by hand even if this could be safely done without a shell.
-        pub fn with_shell_disallow_manual_argument_splitting(mut self) -> Self {
+        /// Like [`command_may_be_shell_script()`](Prepare::command_may_be_shell_script()), but don't allow to bypass the shell even if manual argument splitting
+        /// can be performed safely.
+        pub fn command_may_be_shell_script_disallow_manual_argument_splitting(mut self) -> Self {
             self.allow_manual_arg_splitting = false;
-            self.with_shell()
+            self.command_may_be_shell_script()
         }
 
         /// Configure the process to use `stdio` for _stdin.

gix-command/tests/command.rs

@@ -246,7 +246,8 @@ mod prepare {
     #[test]
     fn multiple_arguments_in_one_line_with_auto_split() {
         let cmd = std::process::Command::from(
-            gix_command::prepare("echo first second third").with_shell_allow_manual_argument_splitting(),
+            gix_command::prepare("echo first second third")
+                .command_may_be_shell_script_allow_manual_argument_splitting(),
         );
         assert_eq!(
             format!("{cmd:?}"),
@@ -267,7 +268,8 @@ mod prepare {
 
     #[test]
     fn single_and_multiple_arguments_as_part_of_command_with_shell() {
-        let cmd = std::process::Command::from(gix_command::prepare("ls first second third").with_shell());
+        let cmd =
+            std::process::Command::from(gix_command::prepare("ls first second third").command_may_be_shell_script());
         assert_eq!(
             format!("{cmd:?}"),
             if cfg!(windows) {
@@ -283,7 +285,7 @@ mod prepare {
     fn single_and_multiple_arguments_as_part_of_command_with_given_shell() {
         let cmd = std::process::Command::from(
             gix_command::prepare("ls first second third")
-                .with_shell()
+                .command_may_be_shell_script()
                 .with_shell_program("/somepath/to/bash"),
         );
         assert_eq!(
@@ -299,7 +301,11 @@ mod prepare {
 
     #[test]
     fn single_and_complex_arguments_as_part_of_command_with_shell() {
-        let cmd = std::process::Command::from(gix_command::prepare("ls --foo \"a b\"").arg("additional").with_shell());
+        let cmd = std::process::Command::from(
+            gix_command::prepare("ls --foo \"a b\"")
+                .arg("additional")
+                .command_may_be_shell_script(),
+        );
         assert_eq!(
             format!("{cmd:?}"),
             if cfg!(windows) {
@@ -314,7 +320,7 @@ mod prepare {
     #[test]
     fn single_and_complex_arguments_with_auto_split() {
         let cmd = std::process::Command::from(
-            gix_command::prepare("ls --foo=\"a b\"").with_shell_allow_manual_argument_splitting(),
+            gix_command::prepare("ls --foo=\"a b\"").command_may_be_shell_script_allow_manual_argument_splitting(),
         );
         assert_eq!(
             format!("{cmd:?}"),
@@ -326,15 +332,29 @@ mod prepare {
     #[test]
     fn single_and_complex_arguments_without_auto_split() {
         let cmd = std::process::Command::from(
-            gix_command::prepare("ls --foo=\"a b\"").with_shell_disallow_manual_argument_splitting(),
+            gix_command::prepare("ls --foo=\"a b\"").command_may_be_shell_script_disallow_manual_argument_splitting(),
         );
         assert_eq!(format!("{cmd:?}"), quoted(&[SH, "-c", r#"ls --foo=\"a b\""#, "--"]));
     }
 
+    #[test]
+    fn single_and_simple_arguments_without_auto_split_with_shell() {
+        let cmd = std::process::Command::from(
+            gix_command::prepare("ls")
+                .arg("--foo=a b")
+                .command_may_be_shell_script_disallow_manual_argument_splitting()
+                .with_shell(),
+        );
+        assert_eq!(
+            format!("{cmd:?}"),
+            quoted(&[SH, "-c", "ls \\\"$@\\\"", "--", "--foo=a b"])
+        );
+    }
+
     #[test]
     fn single_and_complex_arguments_will_not_auto_split_on_special_characters() {
         let cmd = std::process::Command::from(
-            gix_command::prepare("ls --foo=~/path").with_shell_allow_manual_argument_splitting(),
+            gix_command::prepare("ls --foo=~/path").command_may_be_shell_script_allow_manual_argument_splitting(),
         );
         assert_eq!(
             format!("{cmd:?}"),
@@ -345,7 +365,8 @@ mod prepare {
 
     #[test]
     fn tilde_path_and_multiple_arguments_as_part_of_command_with_shell() {
-        let cmd = std::process::Command::from(gix_command::prepare("~/bin/exe --foo \"a b\"").with_shell());
+        let cmd =
+            std::process::Command::from(gix_command::prepare("~/bin/exe --foo \"a b\"").command_may_be_shell_script());
         assert_eq!(
             format!("{cmd:?}"),
             format!(r#""{SH}" "-c" "~/bin/exe --foo \"a b\"" "--""#),
@@ -355,7 +376,11 @@ mod prepare {
 
     #[test]
     fn script_with_dollar_at() {
-        let cmd = std::process::Command::from(gix_command::prepare("echo \"$@\" >&2").with_shell().arg("store"));
+        let cmd = std::process::Command::from(
+            gix_command::prepare("echo \"$@\" >&2")
+                .command_may_be_shell_script()
+                .arg("store"),
+        );
         assert_eq!(
             format!("{cmd:?}"),
             format!(r#""{SH}" "-c" "echo \"$@\" >&2" "--" "store""#),
@@ -374,7 +399,7 @@ mod spawn {
         let out = gix_command::prepare("echo $FIRST $SECOND")
             .env("FIRST", "first")
             .env("SECOND", "second")
-            .with_shell()
+            .command_may_be_shell_script()
             .spawn()?
             .wait_with_output()?;
         assert_eq!(out.stdout.as_bstr(), "first second\n");
@@ -385,12 +410,15 @@ mod spawn {
     #[cfg(unix)]
     fn disallow_shell() -> crate::Result {
         let out = gix_command::prepare("PATH= echo hi")
-            .with_shell()
+            .command_may_be_shell_script()
             .spawn()?
             .wait_with_output()?;
         assert_eq!(out.stdout.as_bstr(), "hi\n");
 
-        let mut cmd: std::process::Command = gix_command::prepare("echo hi").with_shell().without_shell().into();
+        let mut cmd: std::process::Command = gix_command::prepare("echo hi")
+            .command_may_be_shell_script()
+            .without_shell()
+            .into();
         assert!(
             cmd.env_remove("PATH").spawn().is_err(),
             "no command named 'echo hi' exists"
@@ -400,9 +428,13 @@ mod spawn {
 
     #[test]
     fn script_with_dollar_at() -> crate::Result {
-        let out = std::process::Command::from(gix_command::prepare("echo \"$@\"").with_shell().arg("arg"))
-            .spawn()?
-            .wait_with_output()?;
+        let out = std::process::Command::from(
+            gix_command::prepare("echo \"$@\"")
+                .command_may_be_shell_script()
+                .arg("arg"),
+        )
+        .spawn()?
+        .wait_with_output()?;
         assert_eq!(
             out.stdout.to_str_lossy().trim(),
             "arg",
@@ -433,7 +465,7 @@ mod spawn {
         #[test]
         fn command_in_path_with_args() -> crate::Result {
             assert!(gix_command::prepare(if cfg!(unix) { "ls -l" } else { "dir.exe -a" })
-                .with_shell()
+                .command_may_be_shell_script()
                 .spawn()?
                 .wait()?
                 .success());
@@ -442,14 +474,18 @@ mod spawn {
 
         #[test]
         fn sh_shell_specific_script_code() -> crate::Result {
-            assert!(gix_command::prepare(":;:;:").with_shell().spawn()?.wait()?.success());
+            assert!(gix_command::prepare(":;:;:")
+                .command_may_be_shell_script()
+                .spawn()?
+                .wait()?
+                .success());
             Ok(())
         }
 
         #[test]
         fn sh_shell_specific_script_code_with_single_extra_arg() -> crate::Result {
             let out = gix_command::prepare("printf")
-                .with_shell()
+                .command_may_be_shell_script()
                 .arg("1")
                 .spawn()?
                 .wait_with_output()?;
@@ -461,7 +497,7 @@ mod spawn {
         #[test]
         fn sh_shell_specific_script_code_with_multiple_extra_args() -> crate::Result {
             let out = gix_command::prepare("printf")
-                .with_shell()
+                .command_may_be_shell_script()
                 .arg("%s")
                 .arg("arg")
                 .spawn()?

gix-credentials/src/program/mod.rs

@@ -82,14 +82,14 @@ impl Program {
                 args.insert_str(0, git_program.to_string_lossy().as_ref());
                 gix_command::prepare(gix_path::from_bstr(args.as_bstr()).into_owned())
                     .arg(action.as_arg(true))
-                    .with_shell_allow_manual_argument_splitting()
+                    .command_may_be_shell_script_allow_manual_argument_splitting()
                     .into()
             }
             Kind::ExternalShellScript(for_shell)
             | Kind::ExternalPath {
                 path_and_args: for_shell,
             } => gix_command::prepare(gix_path::from_bstr(for_shell.as_bstr()).as_ref())
-                .with_shell()
+                .command_may_be_shell_script()
                 .arg(action.as_arg(true))
                 .into(),
         };

gix-diff/src/blob/pipeline.rs

@@ -543,7 +543,7 @@ impl Driver {
         let cmd = gix_command::prepare(gix_path::from_bstr(command).into_owned())
             // TODO: Add support for an actual Context, validate it *can* match Git
             .with_context(Default::default())
-            .with_shell()
+            .command_may_be_shell_script()
             .stdin(Stdio::null())
             .stdout(Stdio::piped())
             .stderr(Stdio::piped())

gix-filter/src/driver/init.rs

@@ -92,7 +92,7 @@ fn spawn_driver(
     context: &gix_command::Context,
 ) -> Result<(std::process::Child, std::process::Command), Error> {
     let mut cmd: std::process::Command = gix_command::prepare(gix_path::from_bstr(cmd).into_owned())
-        .with_shell()
+        .command_may_be_shell_script()
         .with_context(context.clone())
         .stdin(Stdio::piped())
         .stdout(Stdio::piped())

gix-merge/src/blob/platform/merge.rs

@@ -214,7 +214,7 @@ pub(super) mod inner {
                 Ok(merge::Command {
                     cmd: gix_command::prepare(gix_path::from_bstring(cmd))
                         .with_context(context)
-                        .with_shell()
+                        .command_may_be_shell_script()
                         .stdin(Stdio::null())
                         .stdout(Stdio::inherit())
                         .stderr(Stdio::inherit())

gix-transport/src/client/blocking_io/ssh/mod.rs

@@ -116,7 +116,7 @@ pub fn connect(
                 .stderr(Stdio::null())
                 .stdout(Stdio::null())
                 .stdin(Stdio::null())
-                .with_shell()
+                .command_may_be_shell_script()
                 .arg("-G")
                 .arg(match url.host_as_argument() {
                     Usable(host) => host,

gix-transport/src/client/blocking_io/ssh/program_kind.rs

@@ -29,7 +29,7 @@ impl ProgramKind {
         desired_version: Protocol,
         disallow_shell: bool,
     ) -> Result<gix_command::Prepare, ssh::invocation::Error> {
-        let mut prepare = gix_command::prepare(ssh_cmd).with_shell();
+        let mut prepare = gix_command::prepare(ssh_cmd).command_may_be_shell_script();
         if disallow_shell {
             prepare.use_shell = false;
         }