Skip to content
115 changes: 78 additions & 37 deletions src/uu/echo/src/echo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -23,63 +23,104 @@ mod options {
pub const DISABLE_BACKSLASH_ESCAPE: &str = "disable_backslash_escape";
}

fn is_echo_flag(arg: &OsString) -> bool {
matches!(arg.to_str(), Some("-e" | "-E" | "-n"))
/// Holds the options for echo command:
/// -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
// follows. In order to use '--' as a value, we have to inject an additional '--'
fn handle_double_hyphens(args: impl uucore::Args) -> impl uucore::Args {
let mut result = Vec::new();
let mut is_first_argument = true;
let mut args_iter = args.into_iter();

if let Some(first_val) = args_iter.next() {
// the first argument ('echo') gets pushed before we start with the checks for flags/'--'
result.push(first_val);
// We need to skip any possible Flag arguments until we find the first argument to echo that
// is not a flag. If the first argument is double hyphen we inject an additional '--'
// otherwise we switch is_first_argument boolean to skip the checks for any further arguments
for arg in args_iter {
if is_first_argument && !is_echo_flag(&arg) {
is_first_argument = false;
if arg == "--" {
result.push(OsString::from("--"));
}
/// Checks if an argument is a valid echo flag
/// Returns true if valid echo flag found
fn is_echo_flag(arg: &OsString, echo_options: &mut EchoOptions) -> bool {
let bytes = arg.as_encoded_bytes();
if bytes.first() == Some(&b'-') && arg != "-" {
// 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;

// Process characters after the '-'
for c in &bytes[1..] {
match c {
b'e' => escape = true,
b'E' => escape = false,
b'n' => trailing_newline = false,
// if there is any char in an argument starting with '-' that doesn't match e/E/n
// present means that this argument is not a flag
_ => return false,
}
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> {
matches
.get_many::<OsString>(options::STRING)
.map_or_else(Vec::new, |values| values.cloned().collect())
/// Processes command line arguments, separating flags from normal arguments
/// Returns:
/// - Vector of non-flag arguments
/// - 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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a subtle change in behavior here: POSIXLY_CORRECT=<non-utf8-sequence> is now interpreted as true, whereas before it was interpreted as false. I assume this is a correct change. (I can't test GNU easily right now.)

$ POSIXLY_CORRECT=$'\377asd' cargo run echo 'asd\nqwe' # current main
asd\nqwe
$ POSIXLY_CORRECT=$'\377asd' cargo run echo 'asd\nqwe' # your PR
asd
qwe

Nitpick: If you fix subtleties like this, it would be extra awesome if you also add a test, so that we don't lose it later :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested that change upthread when I saw this code at the edge of a diff 😅

The following test (in test_echo.rs inside mod posixly_correct {}) should do the trick:

    #[cfg(unix)]
    #[test]
    fn non_utf8_variable() {
        use std::{ffi::OsStr, os::unix::ffi::OsStrExt};

        new_ucmd!()
            .env("POSIXLY_CORRECT", OsStr::from_bytes(b"\xFF"))
            .arg("-e")
            .succeeds()
            .stdout_only("-e\n");
    }

(I can confirm that this matches GNU, GNU activates POSIX mode regardless of whether the variable's value is 1 or 0 or empty string or \xFF.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! But I would argue that if you change the behavior, then an old test cannot possibly correctly test for the new behavior :D

In particular, this test only tests whether -e is interpreted or considered as part of the output. It does not test whether escape sequences are interpreted or taken literally, i.e. whether "escape mode" is initially set or not.

Like I said, it's not a major thing, and doesn't block this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not an old test, I meant that was where I'd put it. It fails if env::var(...).is_ok() is used.


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));
let args = collect_args(&matches);
// if POSIXLY_CORRECT is set and the first argument is the "-n" flag
// 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()));
let trailing_newline = !matches.get_flag(options::NO_NEWLINE);
let escaped = matches.get_flag(options::ENABLE_BACKSLASH_ESCAPE);
let args = collect_args(&matches);
// if POSIXLY_CORRECT is not set we filter the flags normally
let (args, trailing_newline, escaped) = filter_echo_flags(args_iter);
(args, trailing_newline, escaped)
};

Expand Down
107 changes: 106 additions & 1 deletion tests/by-util/test_echo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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!()
Expand All @@ -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("--")
Expand Down
Loading