fix: Posix compliant argument parsing for -c mode (#147)

clap does not handle `--` as an argument but rather as a separator so we need additional processing
for passing `--` as an operand to a `-c` script. This is because Posix interprets `--` as an operand. The first argument in `-c` is the name of the command and can be `--` just fine.

- Added functions `try_parse_known` and `parse_known` for splitting arguments into `before hyphen` and `hyphen and after hyphen`
- This approach allows manually deciding what to do with `--` and the remaining arguments

brush-core/src/builtins.rs

@@ -382,3 +382,75 @@ fn brush_help_styles() -> clap::builder::Styles {
         .literal(styling::AnsiColor::Magenta.on_default() | styling::Effects::BOLD)
         .placeholder(styling::AnsiColor::Cyan.on_default())
 }
+
+/// This function and the [`try_parse_known`] exists to deal with
+/// the Clap's limitation of treating `--` like a regular value
+/// `https://github.com/clap-rs/clap/issues/5055`
+///
+/// # Arguments
+///
+/// * `args` - An Iterator from [`std::env::args`]
+///
+/// # Returns
+///
+/// * a parsed struct T from [`clap::Parser::parse_from`]
+/// * the remain iterator `args` with `--` and the rest arguments if they present
+///   othervise None
+///
+/// # Examples
+/// ```
+///    use clap::{builder::styling, Parser};
+///    #[derive(Parser)]
+///    struct CommandLineArgs {
+///       #[clap(allow_hyphen_values = true, num_args=1..)]
+///       script_args: Vec<String>,
+///    }
+///
+///    let (mut parsed_args, raw_args) =
+///        brush_core::builtins::parse_known::<CommandLineArgs, _>(std::env::args());
+///    if raw_args.is_some() {
+///        parsed_args.script_args = raw_args.unwrap().collect();
+///    }
+/// ```
+pub fn parse_known<T: Parser, S>(
+    args: impl IntoIterator<Item = S>,
+) -> (T, Option<impl Iterator<Item = S>>)
+where
+    S: Into<std::ffi::OsString> + Clone + PartialEq<&'static str>,
+{
+    let mut args = args.into_iter();
+    // the best way to save `--` is to get it out with a side effect while `clap` iterates over the args
+    // this way we can be 100% sure that we have '--' and the remaining args
+    // and we will iterate only once
+    let mut hyphen = None;
+    let args_before_hyphen = args.by_ref().take_while(|a| {
+        let is_hyphen = *a == "--";
+        if is_hyphen {
+            hyphen = Some(a.clone());
+        }
+        !is_hyphen
+    });
+    let parsed_args = T::parse_from(args_before_hyphen);
+    let raw_args = hyphen.map(|hyphen| std::iter::once(hyphen).chain(args));
+    (parsed_args, raw_args)
+}
+
+/// Similar to [`parse_known`] but with [`clap::Parser::try_parse_from`]
+/// This function is used to parse arguments in builtins such as [`crate::builtins::echo::EchoCommand`]
+pub fn try_parse_known<T: Parser>(
+    args: impl IntoIterator<Item = String>,
+) -> Result<(T, Option<impl Iterator<Item = String>>), clap::Error> {
+    let mut args = args.into_iter();
+    let mut hyphen = None;
+    let args_before_hyphen = args.by_ref().take_while(|a| {
+        let is_hyphen = a == "--";
+        if is_hyphen {
+            hyphen = Some(a.clone());
+        }
+        !is_hyphen
+    });
+    let parsed_args = T::try_parse_from(args_before_hyphen)?;
+
+    let raw_args = hyphen.map(|hyphen| std::iter::once(hyphen).chain(args));
+    Ok((parsed_args, raw_args))
+}

brush-core/src/builtins/echo.rs

@@ -26,6 +26,20 @@ pub(crate) struct EchoCommand {
 
 #[async_trait::async_trait]
 impl builtins::Command for EchoCommand {
+    /// Override the default [builtins::Command::new] function to handle clap's limitation related to `--`.
+    /// See [crate::builtins::parse_known] for more information
+    /// TODO: we can safely remove this after the issue is resolved
+    fn new<I>(args: I) -> Result<Self, clap::Error>
+    where
+        I: IntoIterator<Item = String>,
+    {
+        let (mut this, rest_args) = crate::builtins::try_parse_known::<EchoCommand>(args)?;
+        if let Some(args) = rest_args {
+            this.args.extend(args);
+        }
+        Ok(this)
+    }
+
     async fn execute(
         &self,
         context: commands::ExecutionContext<'_>,

brush-shell/src/args.rs

@@ -114,9 +114,14 @@ pub struct CommandLineArgs {
     pub enabled_log_events: Vec<events::TraceEvent>,
 
     /// Path to script to execute.
+    // allow any string as command_name similar to sh
+    #[clap(allow_hyphen_values = true)]
     pub script_path: Option<String>,
 
     /// Arguments for script.
+    // `allow_hyphen_values`: do not strip `-` from flags
+    // `num_args=1..`: consume everything
+    #[clap(allow_hyphen_values = true, num_args=1..)]
     pub script_args: Vec<String>,
 }
 

brush-shell/src/main.rs

@@ -10,14 +10,41 @@ mod shell_factory;
 
 use crate::args::{CommandLineArgs, InputBackend};
 use brush_interactive::InteractiveShell;
-use clap::Parser;
 use std::{path::Path, sync::Arc};
 
 lazy_static::lazy_static! {
     static ref TRACE_EVENT_CONFIG: Arc<tokio::sync::Mutex<Option<events::TraceEventConfig>>> =
         Arc::new(tokio::sync::Mutex::new(None));
 }
 
+// WARN: this implementation shadows `clap::Parser::parse_from` one so it must be defined
+// after the `use clap::Parser`
+impl CommandLineArgs {
+    // Work around clap's limitation handling `--` like a regular value
+    // TODO: We can safely remove this `impl` after the issue is resolved
+    // https://github.com/clap-rs/clap/issues/5055
+    // This function takes precedence over [`clap::Parser::parse_from`]
+    fn parse_from<'a>(itr: impl IntoIterator<Item = &'a String>) -> Self {
+        let (mut this, script_args) = brush_core::builtins::parse_known::<CommandLineArgs, _>(itr);
+        // if we have `--` and unparsed raw args than
+        if let Some(mut args) = script_args {
+            // if script_path has not been parsed yet
+            // use the first script_args[0] (it is `--`)
+            // as script_path
+            let first = args.next();
+            if this.script_path.is_none() {
+                this.script_path = first.cloned();
+                this.script_args.extend(args.cloned());
+            } else {
+                // if script_path already exists, everyone goes into script_args
+                this.script_args
+                    .extend(first.into_iter().chain(args).cloned());
+            }
+        }
+        this
+    }
+}
+
 /// Main entry point for the `brush` shell.
 fn main() {
     //

brush-shell/tests/cases/arguments.yaml

@@ -0,0 +1,83 @@
+name: "Argument handling tests"
+common_test_files:
+  - path: "script.sh"
+    contents: |
+      echo \"$0\" \"$1\" \"$2\" \"$@\"
+
+cases:
+  - name: "-c mode arguments without --"
+    args:
+      - "-c"
+      - 'echo \"$0\" \"$1\" \"$2\" \"$@\"'
+      - 1
+      - "-2"
+      - "3"
+
+  - name: "-c mode arguments with --"
+    args:
+      - "-c"
+      - 'echo \"$0\" \"$1\" \"$2\" \"$@\"'
+      - "--"
+      - 1
+      - 2
+      - 3
+
+  - name: "-c mode and arguments with +O"
+    args:
+      - "+O"
+      - "nullglob"
+      - "-c"
+      - 'echo \"$0\" \"$1\" \"$2\" \"$@\"'
+      - "--"
+      - 1
+      - 2
+      - 3
+
+  - name: "-c mode -- torture"
+    args:
+      - "-c"
+      - 'echo \"$0\" \"$1\" \"$2\" \"$@\"'
+      - --
+      - --
+      - -&-1
+      - --!
+      - "\"-2\""
+      - "''--''"
+      - 3--*
+
+  - name: "-c modeonly one --"
+    args:
+      - "-c"
+      - 'echo \"$0\" \"$1\" \"$2\" \"$@\"'
+      - --
+
+  - name: "script arguments without --"
+    args:
+      - script.sh
+      - -1
+      - -2
+      - -3
+
+  - name: "script arguments with --"
+    args:
+      - script.sh
+      - --
+      - --1
+      - -2
+      - 3
+
+  - name: "script -- torture"
+    args:
+      - script.sh
+      - --
+      - "--"
+      - --
+      - -!-1*
+      - "\"-2\""
+      - --
+      - 3--
+
+  - name: "script only one --"
+    args:
+      - script.sh
+      - --

brush-shell/tests/cases/builtins/echo.yaml

@@ -0,0 +1,7 @@
+name: "Builtins: echo"
+cases:
+  - name: "echo with only --"
+    stdin: echo --
+
+  - name: "echo with -- and args"
+    stdin: echo -- -1 --"aaa" ?^1as-