Merge branch 'fix-1103'

Author: Byron

Cargo.lock

@@ -34,7 +34,8 @@ mod prepare {
 
     /// Builder
     impl Prepare {
-        /// If called, the command will not be executed directly, but with `sh`.
+        /// If called, the command will not be executed directly, but with `sh`, but ony if the
+        /// command passed to [`prepare`](super::prepare()) requires this.
         ///
         /// This also allows to pass shell scripts as command, or use commands that contain arguments which are subsequently
         /// parsed by `sh`.

gix-command/src/lib.rs

@@ -1,14 +1,65 @@
 use gix_testtools::Result;
 
 mod prepare {
+    #[cfg(windows)]
+    const SH: &str = "sh";
+    #[cfg(not(windows))]
+    const SH: &str = "/bin/sh";
+
     fn quoted(input: &[&str]) -> String {
         input.iter().map(|s| format!("\"{s}\"")).collect::<Vec<_>>().join(" ")
     }
+
+    #[test]
+    fn empty() {
+        let cmd = std::process::Command::from(gix_command::prepare(""));
+        assert_eq!(format!("{cmd:?}"), "\"\"");
+    }
     #[test]
     fn single_and_multiple_arguments() {
         let cmd = std::process::Command::from(gix_command::prepare("ls").arg("first").args(["second", "third"]));
         assert_eq!(format!("{cmd:?}"), quoted(&["ls", "first", "second", "third"]));
     }
+
+    #[test]
+    fn single_and_multiple_arguments_as_part_of_command() {
+        let cmd = std::process::Command::from(gix_command::prepare("ls first second third"));
+        assert_eq!(
+            format!("{cmd:?}"),
+            quoted(&["ls first second third"]),
+            "without shell, this is an invalid command"
+        );
+    }
+
+    #[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());
+        assert_eq!(
+            format!("{cmd:?}"),
+            quoted(&[SH, "-c", "ls first second third", "--"]),
+            "with shell, this works as it performs word splitting"
+        );
+    }
+
+    #[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());
+        assert_eq!(
+            format!("{cmd:?}"),
+            format!("\"{SH}\" \"-c\" \"ls --foo \\\"a b\\\" \\\"$@\\\"\" \"--\" \"additional\""),
+            "with shell, this works as it performs word splitting"
+        );
+    }
+
+    #[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());
+        assert_eq!(
+            format!("{cmd:?}"),
+            format!("\"{SH}\" \"-c\" \"~/bin/exe --foo \\\"a b\\\"\" \"--\""),
+            "this always needs a shell as we need tilde expansion"
+        );
+    }
 }
 
 mod spawn {

gix-command/tests/command.rs

@@ -28,6 +28,7 @@ gix-trace = { version = "^0.1.3", path = "../gix-trace" }
 thiserror = "1.0.32"
 serde = { version = "1.0.114", optional = true, default-features = false, features = ["derive"] }
 bstr = { version = "1.3.0", default-features = false, features = ["std"]}
+shell-words = "1.0"
 
 
 

gix-credentials/Cargo.toml

@@ -12,7 +12,8 @@ pub enum Kind {
     /// A custom credentials helper, as identified just by the name with optional arguments
     ExternalName {
         /// The name like `foo` along with optional args, like `foo --arg --bar="a b"`, with arguments using `sh` shell quoting rules.
-        /// The program executed will be `git-credential-foo` if `name_and_args` starts with `foo`.
+        /// The program executed will be `git-credential-foo [args]` if `name_and_args` starts with `foo [args]`.
+        /// Note that a shell is only used if it's needed.
         name_and_args: BString,
     },
     /// A custom credentials helper, as identified just by the absolute path to the program and optional arguments. The program is executed through a shell.
@@ -53,7 +54,6 @@ impl Program {
                 if gix_path::is_absolute(path) {
                     Kind::ExternalPath { path_and_args: input }
                 } else {
-                    input.insert_str(0, "git credential-");
                     Kind::ExternalName { name_and_args: input }
                 }
             };
@@ -65,33 +65,41 @@ impl Program {
         }
         from_custom_definition_inner(input.into())
     }
-}
-
-/// Builder
-impl Program {
-    /// By default `stderr` of programs is inherited and typically displayed in the terminal.
-    pub fn suppress_stderr(mut self) -> Self {
-        self.stderr = false;
-        self
-    }
-}
 
-impl Program {
-    pub(crate) fn start(
-        &mut self,
-        action: &helper::Action,
-    ) -> std::io::Result<(std::process::ChildStdin, Option<std::process::ChildStdout>)> {
-        assert!(self.child.is_none(), "BUG: must not call `start()` twice");
+    /// Convert the program into the respective command, suitable to invoke `action`.
+    pub fn to_command(&self, action: &helper::Action) -> std::process::Command {
+        let git_program = cfg!(windows).then(|| "git.exe").unwrap_or("git");
         let mut cmd = match &self.kind {
             Kind::Builtin => {
-                let mut cmd = Command::new(cfg!(windows).then(|| "git.exe").unwrap_or("git"));
+                let mut cmd = Command::new(git_program);
                 cmd.arg("credential").arg(action.as_arg(false));
                 cmd
             }
-            Kind::ExternalShellScript(for_shell)
-            | Kind::ExternalName {
-                name_and_args: for_shell,
+            Kind::ExternalName { name_and_args } => {
+                let mut args = name_and_args.clone();
+                args.insert_str(0, "credential-");
+                args.insert_str(0, " ");
+                args.insert_str(0, git_program);
+                let split_args = if args.find_byteset(b"\\|&;<>()$`\n*?[#~%").is_none() {
+                    args.to_str()
+                        .ok()
+                        .and_then(|args| shell_words::split(args).ok().map(Vec::into_iter))
+                } else {
+                    None
+                };
+                match split_args {
+                    Some(mut args) => {
+                        let mut cmd = Command::new(args.next().expect("non-empty input"));
+                        cmd.args(args).arg(action.as_arg(true));
+                        cmd
+                    }
+                    None => gix_command::prepare(gix_path::from_bstr(args.as_ref()).into_owned())
+                        .arg(action.as_arg(true))
+                        .with_shell()
+                        .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())
@@ -106,6 +114,26 @@ impl Program {
                 Stdio::null()
             })
             .stderr(if self.stderr { Stdio::inherit() } else { Stdio::null() });
+        cmd
+    }
+}
+
+/// Builder
+impl Program {
+    /// By default `stderr` of programs is inherited and typically displayed in the terminal.
+    pub fn suppress_stderr(mut self) -> Self {
+        self.stderr = false;
+        self
+    }
+}
+
+impl Program {
+    pub(crate) fn start(
+        &mut self,
+        action: &helper::Action,
+    ) -> std::io::Result<(std::process::ChildStdin, Option<std::process::ChildStdout>)> {
+        assert!(self.child.is_none(), "BUG: must not call `start()` twice");
+        let mut cmd = self.to_command(action);
         gix_trace::debug!(cmd = ?cmd, "launching credential helper");
         let mut child = cmd.spawn()?;
         let stdin = child.stdin.take().expect("stdin to be configured");

gix-credentials/src/program/mod.rs

@@ -1,42 +1,102 @@
-use gix_credentials::{program::Kind, Program};
+use gix_credentials::{helper, program::Kind, Program};
+
+#[cfg(windows)]
+const GIT: &str = "git.exe";
+#[cfg(not(windows))]
+const GIT: &str = "git";
+
+#[cfg(windows)]
+const SH: &str = "sh";
+#[cfg(not(windows))]
+const SH: &str = "/bin/sh";
 
 #[test]
-fn script() {
-    assert!(
-        matches!(Program::from_custom_definition("!exe").kind, Kind::ExternalShellScript(script) if script == "exe")
+fn empty() {
+    let prog = Program::from_custom_definition("");
+    assert!(matches!(&prog.kind, Kind::ExternalName { name_and_args } if name_and_args == ""));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{GIT}" "credential-" "store""#),
+        "not useful, but allowed, would have to be caught elsewhere"
+    );
+}
+
+#[test]
+fn simple_script_in_path() {
+    let prog = Program::from_custom_definition("!exe");
+    assert!(matches!(&prog.kind, Kind::ExternalShellScript(script) if script == "exe"));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        r#""exe" "store""#,
+        "it didn't detect anything shell-scripty, and thus doesn't use a shell"
     );
 }
 
 #[test]
 fn name_with_args() {
     let input = "name --arg --bar=\"a b\"";
-    let expected = "git credential-name --arg --bar=\"a b\"";
-    assert!(
-        matches!(Program::from_custom_definition(input).kind, Kind::ExternalName{name_and_args} if name_and_args == expected)
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalName{name_and_args} if name_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{GIT}" "credential-name" "--arg" "--bar=a b" "store""#)
+    );
+}
+
+#[test]
+fn name_with_special_args() {
+    let input = "name --arg --bar=~/folder/in/home";
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalName{name_and_args} if name_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{SH}" "-c" "{GIT} credential-name --arg --bar=~/folder/in/home \"$@\"" "--" "store""#)
     );
 }
 
 #[test]
 fn name() {
     let input = "name";
-    let expected = "git credential-name";
-    assert!(
-        matches!(Program::from_custom_definition(input).kind, Kind::ExternalName{name_and_args} if name_and_args == expected)
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalName{name_and_args} if name_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{GIT}" "credential-name" "store""#),
+        "we detect that this can run without shell, which is also more portable on windows"
     );
 }
 
 #[test]
-fn path_with_args() {
+fn path_with_args_that_definitely_need_shell() {
     let input = "/abs/name --arg --bar=\"a b\"";
-    assert!(
-        matches!(Program::from_custom_definition(input).kind, Kind::ExternalPath{path_and_args} if path_and_args == input)
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{SH}" "-c" "/abs/name --arg --bar=\"a b\" \"$@\"" "--" "store""#)
     );
 }
 
 #[test]
-fn path() {
+fn path_without_args() {
     let input = "/abs/name";
-    assert!(
-        matches!(Program::from_custom_definition(input).kind, Kind::ExternalPath{path_and_args} if path_and_args == input)
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        r#""/abs/name" "store""#,
+        "no shell is used"
+    );
+}
+
+#[test]
+fn path_with_simple_args() {
+    let input = "/abs/name a b";
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{SH}" "-c" "/abs/name a b \"$@\"" "--" "store""#),
+        "a shell is used as well because there are arguments, and we don't do splitting ourselves. On windows, this can be a problem."
     );
 }

gix-credentials/tests/program/from_custom_definition.rs

@@ -81,10 +81,7 @@ mod baseline {
             .programs
             .iter()
             .map(|p| match &p.kind {
-                gix_credentials::program::Kind::ExternalName { name_and_args } => name_and_args
-                    .strip_prefix(b"git credential-")
-                    .expect("resolved name")
-                    .into(),
+                gix_credentials::program::Kind::ExternalName { name_and_args } => name_and_args.to_owned(),
                 _ => panic!("need name helper"),
             })
             .collect();