util: Fix shell builder quoting regressions (#44685)

Lukas Wirth created

Follow up to https://github.com/zed-industries/zed/pull/42382

Release Notes:

- N/A *or* Added/Fixed/Improved ...

Change summary

crates/agent_servers/src/acp.rs                        |  21 -
crates/context_server/src/transport/stdio_transport.rs |   6 
crates/project/src/debugger/locators/cargo.rs          |  23 +-
crates/terminal_view/src/terminal_panel.rs             |   2 
crates/util/src/shell.rs                               |   9 
crates/util/src/shell_builder.rs                       | 131 ++++++++++-
6 files changed, 137 insertions(+), 55 deletions(-)

Detailed changes

crates/agent_servers/src/acp.rs 🔗

@@ -11,8 +11,6 @@ use project::agent_server_store::AgentServerCommand;
 use serde::Deserialize;
 use settings::Settings as _;
 use task::ShellBuilder;
-#[cfg(windows)]
-use task::ShellKind;
 use util::ResultExt as _;
 
 use std::path::PathBuf;
@@ -92,23 +90,8 @@ impl AcpConnection {
     ) -> Result<Self> {
         let shell = cx.update(|cx| TerminalSettings::get(None, cx).shell.clone())?;
         let builder = ShellBuilder::new(&shell, cfg!(windows));
-        #[cfg(windows)]
-        let kind = builder.kind();
-        let (cmd, args) = builder.build(Some(command.path.display().to_string()), &command.args);
-
-        let mut child = util::command::new_smol_command(cmd);
-        #[cfg(windows)]
-        if kind == ShellKind::Cmd {
-            use smol::process::windows::CommandExt;
-            for arg in args {
-                child.raw_arg(arg);
-            }
-        } else {
-            child.args(args);
-        }
-        #[cfg(not(windows))]
-        child.args(args);
-
+        let mut child =
+            builder.build_command(Some(command.path.display().to_string()), &command.args);
         child
             .envs(command.env.iter().flatten())
             .stdin(std::process::Stdio::piped())

crates/context_server/src/transport/stdio_transport.rs 🔗

@@ -33,12 +33,10 @@ impl StdioTransport {
     ) -> Result<Self> {
         let shell = cx.update(|cx| TerminalSettings::get(None, cx).shell.clone())?;
         let builder = ShellBuilder::new(&shell, cfg!(windows));
-        let (command, args) =
-            builder.build(Some(binary.executable.display().to_string()), &binary.args);
+        let mut command =
+            builder.build_command(Some(binary.executable.display().to_string()), &binary.args);
 
-        let mut command = util::command::new_smol_command(command);
         command
-            .args(args)
             .envs(binary.env.unwrap_or_default())
             .stdin(std::process::Stdio::piped())
             .stdout(std::process::Stdio::piped())

crates/project/src/debugger/locators/cargo.rs 🔗

@@ -115,18 +115,17 @@ impl DapLocator for CargoLocator {
             .clone()
             .context("Couldn't get cwd from debug config which is needed for locators")?;
         let builder = ShellBuilder::new(&build_config.shell, cfg!(windows)).non_interactive();
-        let (program, args) = builder.build(
-            Some("cargo".into()),
-            &build_config
-                .args
-                .iter()
-                .cloned()
-                .take_while(|arg| arg != "--")
-                .chain(Some("--message-format=json".to_owned()))
-                .collect::<Vec<_>>(),
-        );
-        let mut child = util::command::new_smol_command(program)
-            .args(args)
+        let mut child = builder
+            .build_command(
+                Some("cargo".into()),
+                &build_config
+                    .args
+                    .iter()
+                    .cloned()
+                    .take_while(|arg| arg != "--")
+                    .chain(Some("--message-format=json".to_owned()))
+                    .collect::<Vec<_>>(),
+            )
             .envs(build_config.env.iter().map(|(k, v)| (k.clone(), v.clone())))
             .current_dir(cwd)
             .stdout(Stdio::piped())

crates/terminal_view/src/terminal_panel.rs 🔗

@@ -550,7 +550,7 @@ impl TerminalPanel {
 
         let builder = ShellBuilder::new(&shell, is_windows);
         let command_label = builder.command_label(task.command.as_deref().unwrap_or(""));
-        let (command, args) = builder.build(task.command.clone(), &task.args);
+        let (command, args) = builder.build_no_quote(task.command.clone(), &task.args);
 
         let task = SpawnInTerminal {
             command_label,

crates/util/src/shell.rs 🔗

@@ -702,7 +702,10 @@ impl ShellKind {
                     .map(|quoted| Cow::Owned(self.prepend_command_prefix(&quoted).into_owned()));
             }
         }
-        self.try_quote(arg)
+        self.try_quote(arg).map(|quoted| match quoted {
+            unquoted @ Cow::Borrowed(_) => unquoted,
+            Cow::Owned(quoted) => Cow::Owned(self.prepend_command_prefix(&quoted).into_owned()),
+        })
     }
 
     pub fn split(&self, input: &str) -> Option<Vec<String>> {
@@ -916,7 +919,7 @@ mod tests {
                 .try_quote_prefix_aware("'uname'")
                 .unwrap()
                 .into_owned(),
-            "\"'uname'\"".to_string()
+            "^\"'uname'\"".to_string()
         );
         assert_eq!(
             shell_kind.try_quote("^uname").unwrap().into_owned(),
@@ -949,7 +952,7 @@ mod tests {
                 .try_quote_prefix_aware("'uname a'")
                 .unwrap()
                 .into_owned(),
-            "\"'uname a'\"".to_string()
+            "^\"'uname a'\"".to_string()
         );
         assert_eq!(
             shell_kind.try_quote("^'uname a'").unwrap().into_owned(),

crates/util/src/shell_builder.rs 🔗

@@ -80,27 +80,23 @@ impl ShellBuilder {
         task_args: &[String],
     ) -> (String, Vec<String>) {
         if let Some(task_command) = task_command {
-            let task_command = self.kind.prepend_command_prefix(&task_command);
             let task_command = if !task_args.is_empty() {
                 match self.kind.try_quote_prefix_aware(&task_command) {
-                    Some(task_command) => task_command,
+                    Some(task_command) => task_command.into_owned(),
                     None => task_command,
                 }
             } else {
                 task_command
             };
-            let mut combined_command =
-                task_args
-                    .iter()
-                    .fold(task_command.into_owned(), |mut command, arg| {
-                        command.push(' ');
-                        let shell_variable = self.kind.to_shell_variable(arg);
-                        command.push_str(&match self.kind.try_quote(&shell_variable) {
-                            Some(shell_variable) => shell_variable,
-                            None => Cow::Owned(shell_variable),
-                        });
-                        command
-                    });
+            let mut combined_command = task_args.iter().fold(task_command, |mut command, arg| {
+                command.push(' ');
+                let shell_variable = self.kind.to_shell_variable(arg);
+                command.push_str(&match self.kind.try_quote(&shell_variable) {
+                    Some(shell_variable) => shell_variable,
+                    None => Cow::Owned(shell_variable),
+                });
+                command
+            });
             if self.redirect_stdin {
                 match self.kind {
                     ShellKind::Fish => {
@@ -134,6 +130,90 @@ impl ShellBuilder {
         (self.program, self.args)
     }
 
+    // This should not exist, but our task infra is broken beyond repair right now
+    #[doc(hidden)]
+    pub fn build_no_quote(
+        mut self,
+        task_command: Option<String>,
+        task_args: &[String],
+    ) -> (String, Vec<String>) {
+        if let Some(task_command) = task_command {
+            let mut combined_command = task_args.iter().fold(task_command, |mut command, arg| {
+                command.push(' ');
+                command.push_str(&self.kind.to_shell_variable(arg));
+                command
+            });
+            if self.redirect_stdin {
+                match self.kind {
+                    ShellKind::Fish => {
+                        combined_command.insert_str(0, "begin; ");
+                        combined_command.push_str("; end </dev/null");
+                    }
+                    ShellKind::Posix
+                    | ShellKind::Nushell
+                    | ShellKind::Csh
+                    | ShellKind::Tcsh
+                    | ShellKind::Rc
+                    | ShellKind::Xonsh
+                    | ShellKind::Elvish => {
+                        combined_command.insert(0, '(');
+                        combined_command.push_str(") </dev/null");
+                    }
+                    ShellKind::PowerShell | ShellKind::Pwsh => {
+                        combined_command.insert_str(0, "$null | & {");
+                        combined_command.push_str("}");
+                    }
+                    ShellKind::Cmd => {
+                        combined_command.push_str("< NUL");
+                    }
+                }
+            }
+
+            self.args
+                .extend(self.kind.args_for_shell(self.interactive, combined_command));
+        }
+
+        (self.program, self.args)
+    }
+
+    /// Builds a command with the given task command and arguments.
+    ///
+    /// Prefer this over manually constructing a command with the output of `Self::build`,
+    /// as this method handles `cmd` weirdness on windows correctly.
+    pub fn build_command(
+        self,
+        mut task_command: Option<String>,
+        task_args: &[String],
+    ) -> smol::process::Command {
+        #[cfg(windows)]
+        let kind = self.kind;
+        if task_args.is_empty() {
+            task_command = task_command
+                .as_ref()
+                .map(|cmd| self.kind.try_quote_prefix_aware(&cmd).map(Cow::into_owned))
+                .unwrap_or(task_command);
+        }
+        let (program, args) = self.build(task_command, task_args);
+
+        let mut child = crate::command::new_smol_command(program);
+
+        #[cfg(windows)]
+        if kind == ShellKind::Cmd {
+            use smol::process::windows::CommandExt;
+
+            for arg in args {
+                child.raw_arg(arg);
+            }
+        } else {
+            child.args(args);
+        }
+
+        #[cfg(not(windows))]
+        child.args(args);
+
+        child
+    }
+
     pub fn kind(&self) -> ShellKind {
         self.kind
     }
@@ -166,7 +246,7 @@ mod test {
             vec![
                 "-i",
                 "-c",
-                "^echo '$env.hello' '$env.world' nothing '--($env.something)' '$' '${test'"
+                "echo '$env.hello' '$env.world' nothing '--($env.something)' '$' '${test'"
             ]
         );
     }
@@ -181,7 +261,7 @@ mod test {
             .build(Some("echo".into()), &["nothing".to_string()]);
 
         assert_eq!(program, "nu");
-        assert_eq!(args, vec!["-i", "-c", "(^echo nothing) </dev/null"]);
+        assert_eq!(args, vec!["-i", "-c", "(echo nothing) </dev/null"]);
     }
 
     #[test]
@@ -196,4 +276,23 @@ mod test {
         assert_eq!(program, "fish");
         assert_eq!(args, vec!["-i", "-c", "begin; echo test; end </dev/null"]);
     }
+
+    #[test]
+    fn does_not_quote_sole_command_only() {
+        let shell = Shell::Program("fish".to_owned());
+        let shell_builder = ShellBuilder::new(&shell, false);
+
+        let (program, args) = shell_builder.build(Some("echo".into()), &[]);
+
+        assert_eq!(program, "fish");
+        assert_eq!(args, vec!["-i", "-c", "echo"]);
+
+        let shell = Shell::Program("fish".to_owned());
+        let shell_builder = ShellBuilder::new(&shell, false);
+
+        let (program, args) = shell_builder.build(Some("echo oo".into()), &[]);
+
+        assert_eq!(program, "fish");
+        assert_eq!(args, vec!["-i", "-c", "echo oo"]);
+    }
 }