From 667de433f2cd769323ec5534944a6eba01a8ae40 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 23 Jan 2025 10:27:53 +0100 Subject: [PATCH 1/2] fix!: rename `with_shell()` to `command_may_be_shell_script()`, add `with_shell()` to enforce using a shell. That way it's clear the shell will only be used if the command actually is a shell script. This also renames `with_shell_*` variants to `command_may_be_shell_script_*` variants, without a respective `with_shell_*` alternative. --- gix-command/src/lib.rs | 44 ++++++++++++++------- gix-command/tests/command.rs | 74 +++++++++++++++++++++++++++--------- 2 files changed, 85 insertions(+), 33 deletions(-) diff --git a/gix-command/src/lib.rs b/gix-command/src/lib.rs index ae2d261149b..d92d366d58c 100644 --- a/gix-command/src/lib.rs +++ b/gix-command/src/lib.rs @@ -93,19 +93,33 @@ 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) -> Self { self.shell_program = Some(program.into()); self @@ -113,7 +127,7 @@ mod prepare { /// 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. diff --git a/gix-command/tests/command.rs b/gix-command/tests/command.rs index e44929ed149..8dde360a420 100644 --- a/gix-command/tests/command.rs +++ b/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()? From 28f712e97ebd805cbcffe184203c9089248b0ca8 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 23 Jan 2025 10:41:16 +0100 Subject: [PATCH 2/2] adapt to changes in `gix-command` --- gix-credentials/src/program/mod.rs | 4 ++-- gix-diff/src/blob/pipeline.rs | 2 +- gix-filter/src/driver/init.rs | 2 +- gix-merge/src/blob/platform/merge.rs | 2 +- gix-transport/src/client/blocking_io/ssh/mod.rs | 2 +- gix-transport/src/client/blocking_io/ssh/program_kind.rs | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/gix-credentials/src/program/mod.rs b/gix-credentials/src/program/mod.rs index 2d216044997..a966ca622f5 100644 --- a/gix-credentials/src/program/mod.rs +++ b/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(), }; diff --git a/gix-diff/src/blob/pipeline.rs b/gix-diff/src/blob/pipeline.rs index 9f7f342de11..6ad308e7223 100644 --- a/gix-diff/src/blob/pipeline.rs +++ b/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()) diff --git a/gix-filter/src/driver/init.rs b/gix-filter/src/driver/init.rs index 616d4d7193b..2083098f16b 100644 --- a/gix-filter/src/driver/init.rs +++ b/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()) diff --git a/gix-merge/src/blob/platform/merge.rs b/gix-merge/src/blob/platform/merge.rs index 45e6d052251..058a301287c 100644 --- a/gix-merge/src/blob/platform/merge.rs +++ b/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()) diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs index 737543b87ab..21680031712 100644 --- a/gix-transport/src/client/blocking_io/ssh/mod.rs +++ b/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, diff --git a/gix-transport/src/client/blocking_io/ssh/program_kind.rs b/gix-transport/src/client/blocking_io/ssh/program_kind.rs index 36801fa5fa7..a9a5898a127 100644 --- a/gix-transport/src/client/blocking_io/ssh/program_kind.rs +++ b/gix-transport/src/client/blocking_io/ssh/program_kind.rs @@ -29,7 +29,7 @@ impl ProgramKind { desired_version: Protocol, disallow_shell: bool, ) -> Result { - 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; }