fix: simplify quoting logic (#313)

6 files changed, 143 insertions, 49 deletions

diff --git a/Cargo.lock b/Cargo.lock
index b4d644d..87b358f 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2030,7 +2030,6 @@ dependencies = [
  "serde_json",
  "serde_spanned",
  "serde_with",
- "shlex",
  "spdx",
  "strsim",
  "tempfile",
diff --git a/Cargo.toml b/Cargo.toml
index 1cd4e7e..c2cc964 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -48,7 +48,6 @@ serde = "1.0.171"
 serde_json = "1.0.103"
 serde_spanned = "0.6.3"
 serde_with = { version = "3.1.0", features = ["indexmap"] }
-shlex = "1.1.0"
 spdx = "0.10.2"
 strsim = "0.10.0"
 tempfile = "3.6.0"
diff --git a/src/cli/run.rs b/src/cli/run.rs
index ec485f5..a04190b 100644
--- a/src/cli/run.rs
+++ b/src/cli/run.rs
@@ -12,7 +12,7 @@ use rattler_conda_types::Platform;
 use crate::prefix::Prefix;
 use crate::progress::await_in_progress;
 use crate::project::environment::get_metadata_env;
-use crate::task::{CmdArgs, Execute, Task};
+use crate::task::{quote_arguments, CmdArgs, Execute, Task};
 use crate::{environment::get_up_to_date_prefix, Project};
 use rattler_shell::{
     activation::{ActivationVariables, Activator, PathModificationBehaviour},
@@ -78,10 +78,11 @@ pub fn order_tasks(
         .unwrap_or_else(|| {
             (
                 None,
-                Task::Execute(Execute {
-                    cmd: CmdArgs::Multiple(tasks),
+                Execute {
+                    cmd: CmdArgs::from(tasks),
                     depends_on: vec![],
-                }),
+                }
+                .into(),
                 Vec::new(),
             )
         });
@@ -100,11 +101,7 @@ pub fn order_tasks(
 
     while let Some((task, additional_args)) = s1.pop_back() {
         // Get the dependencies of the command
-        let depends_on = match &task {
-            Task::Execute(process) => process.depends_on.as_slice(),
-            Task::Alias(alias) => &alias.depends_on,
-            _ => &[],
-        };
+        let depends_on = task.depends_on();
 
         // Locate the dependencies in the project and add them to the stack
         for dependency in depends_on.iter() {
@@ -132,23 +129,12 @@ pub fn order_tasks(
 
 pub async fn create_script(task: Task, args: Vec<String>) -> miette::Result<SequentialList> {
     // Construct the script from the task
-    let task = match task {
-        Task::Execute(Execute {
-            cmd: CmdArgs::Single(cmd),
-            ..
-        })
-        | Task::Plain(cmd) => cmd,
-        Task::Execute(Execute {
-            cmd: CmdArgs::Multiple(args),
-            ..
-        }) => quote_arguments(args),
-        _ => {
-            return Err(miette::miette!("No command given"));
-        }
-    };
+    let task = task
+        .as_single_command()
+        .ok_or_else(|| miette::miette!("the task does not provide a runnable command"))?;
 
     // Append the command line arguments
-    let cli_args = quote_arguments(args);
+    let cli_args = quote_arguments(args.iter().map(|arg| arg.as_str()));
     let full_script = format!("{task} {cli_args}");
 
     // Parse the shell command
@@ -193,14 +179,6 @@ pub async fn execute_script_with_output(
     }
 }
 
-fn quote_arguments(args: impl IntoIterator<Item = impl AsRef<str>>) -> String {
-    args.into_iter()
-        // surround all the additional arguments in double quotes and santize any command
-        // substitution
-        .map(|a| format!("\"{}\"", a.as_ref().replace('"', "\\\"")))
-        .join(" ")
-}
-
 /// CLI entry point for `pixi run`
 /// When running the sigints are ignored and child can react to them. As it pleases.
 pub async fn execute(args: Args) -> miette::Result<()> {
diff --git a/src/cli/task.rs b/src/cli/task.rs
index 3378117..46817c9 100644
--- a/src/cli/task.rs
+++ b/src/cli/task.rs
@@ -1,4 +1,4 @@
-use crate::task::{Alias, CmdArgs, Execute, Task};
+use crate::task::{quote, Alias, CmdArgs, Execute, Task};
 use crate::Project;
 use clap::Parser;
 use itertools::Itertools;
@@ -74,19 +74,25 @@ impl From<AddArgs> for Task {
     fn from(value: AddArgs) -> Self {
         let depends_on = value.depends_on.unwrap_or_default();
 
+        // Convert the arguments into a single string representation
+        let cmd_args = if value.commands.len() == 1 {
+            value.commands.into_iter().next().unwrap()
+        } else {
+            // Simply concatenate all arguments
+            value
+                .commands
+                .into_iter()
+                .map(|arg| quote(&arg).into_owned())
+                .join(" ")
+        };
+
+        // Depending on whether the task should have depends_on or not we create a Plain or complex
+        // command.
         if depends_on.is_empty() {
-            Self::Plain(if value.commands.len() == 1 {
-                value.commands[0].clone()
-            } else {
-                shlex::join(value.commands.iter().map(AsRef::as_ref))
-            })
+            Self::Plain(cmd_args)
         } else {
             Self::Execute(Execute {
-                cmd: CmdArgs::Single(if value.commands.len() == 1 {
-                    value.commands[0].clone()
-                } else {
-                    shlex::join(value.commands.iter().map(AsRef::as_ref))
-                }),
+                cmd: CmdArgs::Single(cmd_args),
                 depends_on,
             })
         }
@@ -175,7 +181,7 @@ pub fn execute(args: Args) -> miette::Result<()> {
                 project.remove_task(name, platform)?;
                 eprintln!(
                     "{}Removed task {} ",
-                    console::style(console::Emoji("❌ ", "X")).yellow(),
+                    console::style(console::Emoji("✔ ", "+")).green(),
                     console::style(&name).bold(),
                 );
             }
diff --git a/src/task/mod.rs b/src/task/mod.rs
index 53e0517..275c3b9 100644
--- a/src/task/mod.rs
+++ b/src/task/mod.rs
@@ -1,6 +1,7 @@
 use itertools::Itertools;
 use serde::Deserialize;
 use serde_with::{formats::PreferMany, serde_as, OneOrMany};
+use std::borrow::Cow;
 use std::fmt::{Display, Formatter};
 
 /// Represents different types of scripts
@@ -13,6 +14,7 @@ pub enum Task {
 }
 
 impl Task {
+    /// Returns the names of the task that this task depends on
     pub fn depends_on(&self) -> &[String] {
         match self {
             Task::Plain(_) => &[],
@@ -52,6 +54,24 @@ impl Task {
             Task::Alias(_) => false,
         }
     }
+
+    /// Returns the command to execute.
+    pub fn as_command(&self) -> Option<CmdArgs> {
+        match self {
+            Task::Plain(cmd) => Some(CmdArgs::Single(cmd.clone())),
+            Task::Execute(exe) => Some(exe.cmd.clone()),
+            Task::Alias(_) => None,
+        }
+    }
+
+    /// Returns the command to execute as a single string.
+    pub fn as_single_command(&self) -> Option<Cow<str>> {
+        match self {
+            Task::Plain(cmd) => Some(Cow::Borrowed(cmd)),
+            Task::Execute(exe) => Some(exe.cmd.as_single()),
+            Task::Alias(_) => None,
+        }
+    }
 }
 
 /// A command script executes a single command from the environment
@@ -68,6 +88,12 @@ pub struct Execute {
     pub depends_on: Vec<String>,
 }
 
+impl From<Execute> for Task {
+    fn from(value: Execute) -> Self {
+        Task::Execute(value)
+    }
+}
+
 #[derive(Debug, Clone, Deserialize)]
 #[serde(untagged)]
 pub enum CmdArgs {
@@ -75,6 +101,36 @@ pub enum CmdArgs {
     Multiple(Vec<String>),
 }
 
+impl From<Vec<String>> for CmdArgs {
+    fn from(value: Vec<String>) -> Self {
+        CmdArgs::Multiple(value)
+    }
+}
+
+impl From<String> for CmdArgs {
+    fn from(value: String) -> Self {
+        CmdArgs::Single(value)
+    }
+}
+
+impl CmdArgs {
+    /// Returns a single string representation of the command arguments.
+    pub fn as_single(&self) -> Cow<str> {
+        match self {
+            CmdArgs::Single(cmd) => Cow::Borrowed(cmd),
+            CmdArgs::Multiple(args) => Cow::Owned(args.iter().map(|arg| quote(arg)).join(" ")),
+        }
+    }
+
+    /// Returns a single string representation of the command arguments.
+    pub fn into_single(self) -> String {
+        match self {
+            CmdArgs::Single(cmd) => cmd,
+            CmdArgs::Multiple(args) => args.iter().map(|arg| quote(arg)).join(" "),
+        }
+    }
+}
+
 #[derive(Debug, Clone, Deserialize)]
 #[serde(deny_unknown_fields)]
 #[serde_as]
@@ -114,3 +170,52 @@ impl Display for Task {
         }
     }
 }
+
+/// Quotes a string argument if it requires quotes to be able to be properly represented in our
+/// shell implementation.
+pub fn quote(in_str: &str) -> Cow<str> {
+    if in_str.is_empty() {
+        "\"\"".into()
+    } else if in_str
+        .bytes()
+        .any(|c| matches!(c as char, '\t' | '\r' | '\n' | ' '))
+    {
+        let mut out: Vec<u8> = Vec::new();
+        out.push(b'"');
+        for c in in_str.bytes() {
+            match c as char {
+                '"' | '\\' => out.push(b'\\'),
+                _ => (),
+            }
+            out.push(c);
+        }
+        out.push(b'"');
+        unsafe { String::from_utf8_unchecked(out) }.into()
+    } else {
+        in_str.into()
+    }
+}
+
+/// Quotes multiple string arguments and joins them together to form a single string.
+pub fn quote_arguments<'a>(args: impl IntoIterator<Item = &'a str>) -> String {
+    args.into_iter().map(quote).join(" ")
+}
+
+#[cfg(test)]
+mod test {
+    use super::quote;
+
+    #[test]
+    fn test_quote() {
+        assert_eq!(quote("foobar"), "foobar");
+        assert_eq!(quote("foo bar"), "\"foo bar\"");
+        assert_eq!(quote("\""), "\"");
+        assert_eq!(quote("foo \" bar"), "\"foo \\\" bar\"");
+        assert_eq!(quote(""), "\"\"");
+        assert_eq!(quote("$PATH"), "$PATH");
+        assert_eq!(
+            quote("PATH=\"$PATH;build/Debug\""),
+            "PATH=\"$PATH;build/Debug\""
+        );
+    }
+}
diff --git a/tests/task_tests.rs b/tests/task_tests.rs
index e725713..beecf1e 100644
--- a/tests/task_tests.rs
+++ b/tests/task_tests.rs
@@ -45,9 +45,16 @@ pub async fn add_command_types() {
         .unwrap();
 
     let project = pixi.project().unwrap();
-    let task = project.manifest.tasks.get("test2").unwrap();
-    assert!(matches!(task, Task::Execute(cmd) if matches!(cmd.cmd, CmdArgs::Single(_))));
-    assert!(matches!(task, Task::Execute(cmd) if !cmd.depends_on.is_empty()));
+    let task2 = project.manifest.tasks.get("test2").unwrap();
+    let task = project.manifest.tasks.get("test").unwrap();
+    assert!(matches!(task2, Task::Execute(cmd) if matches!(cmd.cmd, CmdArgs::Single(_))));
+    assert!(matches!(task2, Task::Execute(cmd) if !cmd.depends_on.is_empty()));
+
+    assert_eq!(task.as_single_command().as_deref(), Some("echo hello"));
+    assert_eq!(
+        task2.as_single_command().as_deref(),
+        Some("\"echo hello\" \"echo bonjour\"")
+    );
 
     // Create an alias
     pixi.tasks()