Remove clap for echo (#7603)

* Parsing echo flags manually without clap as clap introduced various problematic interactions with hyphens

* fixed error where multiple flags would parse wrong

* Spelling & formatting fixes

* docu for EchoFlag struct

* more extensive comment/documentation

* revert POSIXLY_CORRECT check to only check if it is set

* Fixed problem of overwriting flags. Added test for same issue

* cargo fmt

* cspell

* Update src/uu/echo/src/echo.rs

Enabling POSIXLY_CORRECT flag if value is not UTF-8

Co-authored-by: Jan Verbeek <jan.verbeek@posteo.nl>

---------

Co-authored-by: Jan Verbeek <jan.verbeek@posteo.nl>

src/uu/echo/src/echo.rs

@@ -4,7 +4,7 @@
 // file that was distributed with this source code.
 
 use clap::builder::ValueParser;
-use clap::{Arg, ArgAction, ArgMatches, Command};
+use clap::{Arg, ArgAction, Command};
 use std::env;
 use std::ffi::{OsStr, OsString};
 use std::io::{self, StdoutLock, Write};
@@ -23,63 +23,104 @@ mod options {
     pub const DISABLE_BACKSLASH_ESCAPE: &str = "disable_backslash_escape";
 }
 
-fn is_echo_flag(arg: &OsString) -> bool {
+/// Holds the options for echo command:
-    matches!(arg.to_str(), Some("-e" | "-E" | "-n"))
+/// -n (disable newline)
+/// -e/-E (escape handling),
+struct EchoOptions {
+    /// -n flag option: if true, output a trailing newline (-n disables it)
+    /// Default: true
+    pub trailing_newline: bool,
+
+    /// -e enables escape interpretation, -E disables it
+    /// Default: false (escape interpretation disabled)
+    pub escape: bool,
 }
 
-// A workaround because clap interprets the first '--' as a marker that a value
+/// Checks if an argument is a valid echo flag
-// follows. In order to use '--' as a value, we have to inject an additional '--'
+/// Returns true if valid echo flag found
-fn handle_double_hyphens(args: impl uucore::Args) -> impl uucore::Args {
+fn is_echo_flag(arg: &OsString, echo_options: &mut EchoOptions) -> bool {
-    let mut result = Vec::new();
+    let bytes = arg.as_encoded_bytes();
-    let mut is_first_argument = true;
+    if bytes.first() == Some(&b'-') && arg != "-" {
-    let mut args_iter = args.into_iter();
+        // we initialize our local variables to the "current" options so we don't override
+        // previous found flags
+        let mut escape = echo_options.escape;
+        let mut trailing_newline = echo_options.trailing_newline;
 
-    if let Some(first_val) = args_iter.next() {
+        // Process characters after the '-'
-        // the first argument ('echo') gets pushed before we start with the checks for flags/'--'
+        for c in &bytes[1..] {
-        result.push(first_val);
+            match c {
-        // We need to skip any possible Flag arguments until we find the first argument to echo that
+                b'e' => escape = true,
-        // is not a flag. If the first argument is double hyphen we inject an additional '--'
+                b'E' => escape = false,
-        // otherwise we switch is_first_argument boolean to skip the checks for any further arguments
+                b'n' => trailing_newline = false,
-        for arg in args_iter {
+                // if there is any char in an argument starting with '-' that doesn't match e/E/n
-            if is_first_argument && !is_echo_flag(&arg) {
+                // present means that this argument is not a flag
-                is_first_argument = false;
+                _ => return false,
-                if arg == "--" {
-                    result.push(OsString::from("--"));
-                }
             }
-            result.push(arg);
         }
+
+        // we only override the options with flags being found once we parsed the whole argument
+        echo_options.escape = escape;
+        echo_options.trailing_newline = trailing_newline;
+        return true;
     }
 
-    result.into_iter()
+    // argument doesn't start with '-' or is "-" => no flag
+    false
 }
 
-fn collect_args(matches: &ArgMatches) -> Vec<OsString> {
+/// Processes command line arguments, separating flags from normal arguments
-    matches
+/// Returns:
-        .get_many::<OsString>(options::STRING)
+/// - Vector of non-flag arguments
-        .map_or_else(Vec::new, |values| values.cloned().collect())
+/// - trailing_newline: whether to print a trailing newline
+/// - escape: whether to process escape sequences
+fn filter_echo_flags(args: impl uucore::Args) -> (Vec<OsString>, bool, bool) {
+    let mut result = Vec::new();
+    let mut echo_options = EchoOptions {
+        trailing_newline: true,
+        escape: false,
+    };
+    let mut args_iter = args.into_iter();
+
+    // Process arguments until first non-flag is found
+    for arg in &mut args_iter {
+        // we parse flags and store options found in "echo_option". First is_echo_flag
+        // call to return false will break the loop and we will collect the remaining arguments
+        if !is_echo_flag(&arg, &mut echo_options) {
+            // First non-flag argument stops flag processing
+            result.push(arg);
+            break;
+        }
+    }
+    // Collect remaining arguments
+    for arg in args_iter {
+        result.push(arg);
+    }
+    (result, echo_options.trailing_newline, echo_options.escape)
 }
 
 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
-    let is_posixly_correct = env::var("POSIXLY_CORRECT").is_ok();
+    // Check POSIX compatibility mode
+    let is_posixly_correct = env::var_os("POSIXLY_CORRECT").is_some();
 
+    let args_iter = args.skip(1);
     let (args, trailing_newline, escaped) = if is_posixly_correct {
-        let mut args_iter = args.skip(1).peekable();
+        let mut args_iter = args_iter.peekable();
 
         if args_iter.peek() == Some(&OsString::from("-n")) {
-            let matches = uu_app().get_matches_from(handle_double_hyphens(args_iter));
+            // if POSIXLY_CORRECT is set and the first argument is the "-n" flag
-            let args = collect_args(&matches);
+            // we filter flags normally but 'escaped' is activated nonetheless
+            let (args, _, _) = filter_echo_flags(args_iter);
             (args, false, true)
         } else {
-            let args: Vec<_> = args_iter.collect();
+            // if POSIXLY_CORRECT is set and the first argument is not the "-n" flag
+            // we just collect all arguments as every argument is considered an argument
+            let args: Vec<OsString> = args_iter.collect();
             (args, true, true)
         }
     } else {
-        let matches = uu_app().get_matches_from(handle_double_hyphens(args.into_iter()));
+        // if POSIXLY_CORRECT is not set we filter the flags normally
-        let trailing_newline = !matches.get_flag(options::NO_NEWLINE);
+        let (args, trailing_newline, escaped) = filter_echo_flags(args_iter);
-        let escaped = matches.get_flag(options::ENABLE_BACKSLASH_ESCAPE);
-        let args = collect_args(&matches);
         (args, trailing_newline, escaped)
     };
 

tests/by-util/test_echo.rs

@@ -2,7 +2,7 @@
 //
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
-// spell-checker:ignore (words) araba merci mright
+// spell-checker:ignore (words) araba merci efjkow
 
 use uutests::new_ucmd;
 use uutests::util::TestScenario;
@@ -126,6 +126,16 @@ fn test_escape_override() {
         .args(&["-E", "-e", "\\na"])
         .succeeds()
         .stdout_only("\na\n");
+
+    new_ucmd!()
+        .args(&["-E", "-e", "-n", "\\na"])
+        .succeeds()
+        .stdout_only("\na");
+
+    new_ucmd!()
+        .args(&["-e", "-E", "-n", "\\na"])
+        .succeeds()
+        .stdout_only("\\na");
 }
 
 #[test]
@@ -276,6 +286,89 @@ fn test_double_hyphens_at_start() {
         .stdout_only("-- a b --\n");
 }
 
+#[test]
+fn test_double_hyphens_after_single_hyphen() {
+    new_ucmd!()
+        .arg("-")
+        .arg("--")
+        .succeeds()
+        .stdout_only("- --\n");
+
+    new_ucmd!()
+        .arg("-")
+        .arg("-n")
+        .arg("--")
+        .succeeds()
+        .stdout_only("- -n --\n");
+
+    new_ucmd!()
+        .arg("-n")
+        .arg("-")
+        .arg("--")
+        .succeeds()
+        .stdout_only("- --");
+}
+
+#[test]
+fn test_flag_like_arguments_which_are_no_flags() {
+    new_ucmd!()
+        .arg("-efjkow")
+        .arg("--")
+        .succeeds()
+        .stdout_only("-efjkow --\n");
+
+    new_ucmd!()
+        .arg("--")
+        .arg("-efjkow")
+        .succeeds()
+        .stdout_only("-- -efjkow\n");
+
+    new_ucmd!()
+        .arg("-efjkow")
+        .arg("-n")
+        .arg("--")
+        .succeeds()
+        .stdout_only("-efjkow -n --\n");
+
+    new_ucmd!()
+        .arg("-n")
+        .arg("--")
+        .arg("-efjkow")
+        .succeeds()
+        .stdout_only("-- -efjkow");
+}
+
+#[test]
+fn test_backslash_n_last_char_in_last_argument() {
+    new_ucmd!()
+        .arg("-n")
+        .arg("-e")
+        .arg("--")
+        .arg("foo\n")
+        .succeeds()
+        .stdout_only("-- foo\n");
+
+    new_ucmd!()
+        .arg("-e")
+        .arg("--")
+        .arg("foo\\n")
+        .succeeds()
+        .stdout_only("-- foo\n\n");
+
+    new_ucmd!()
+        .arg("-n")
+        .arg("--")
+        .arg("foo\n")
+        .succeeds()
+        .stdout_only("-- foo\n");
+
+    new_ucmd!()
+        .arg("--")
+        .arg("foo\n")
+        .succeeds()
+        .stdout_only("-- foo\n\n");
+}
+
 #[test]
 fn test_double_hyphens_after_flags() {
     new_ucmd!()
@@ -292,6 +385,18 @@ fn test_double_hyphens_after_flags() {
         .succeeds()
         .stdout_only("-- foo\n");
 
+    new_ucmd!()
+        .arg("-ne")
+        .arg("--")
+        .succeeds()
+        .stdout_only("--");
+
+    new_ucmd!()
+        .arg("-neE")
+        .arg("--")
+        .succeeds()
+        .stdout_only("--");
+
     new_ucmd!()
         .arg("-e")
         .arg("--")