-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Remove clap for echo #7603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove clap for echo #7603
Changes from 6 commits
ca07881
2ead244
2f1a9d1
c1c127e
79a90fd
ab95cf8
fcca66a
5fc2c34
bb16513
17c6c21
e8c3e42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,122 @@ 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 parsed flags for echo command: | ||
/// -n (disable newline) | ||
/// -e/-E (escape handling), | ||
/// '-' (stop processing flags). | ||
struct EchoFlags { | ||
/// -n flag: if true, don't output trailing newline | ||
/// Default: false | ||
pub disable_newline: bool, | ||
|
||
/// -e enables escape interpretation, -E disables it | ||
/// Default: false (escape interpretation disabled) | ||
pub escape: bool, | ||
|
||
/// Single hyphen '-' argument (stops flag processing) | ||
/// Default: false | ||
pub is_single_hyphen: 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(); | ||
/// Checks if an argument is a valid echo flag | ||
/// Returns EchoFlags if valid, None otherwise | ||
fn is_echo_flag(arg: &OsString) -> Option<EchoFlags> { | ||
let bytes = arg.as_encoded_bytes(); | ||
if bytes.first() == Some(&b'-') { | ||
let mut flags = EchoFlags { | ||
disable_newline: false, | ||
escape: false, | ||
is_single_hyphen: false, | ||
}; | ||
// Single hyphen case: "-" (stops flag processing, no other effect) | ||
if arg.len() == 1 { | ||
flags.is_single_hyphen = true; | ||
return Some(flags); | ||
} | ||
|
||
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("--")); | ||
} | ||
// Process characters after the '-' | ||
for c in &bytes[1..] { | ||
match c { | ||
b'e' => flags.escape = true, | ||
b'E' => flags.escape = false, | ||
b'n' => flags.disable_newline = true, | ||
// 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 None, | ||
} | ||
result.push(arg); | ||
} | ||
|
||
return Some(flags); | ||
} | ||
|
||
result.into_iter() | ||
// argument doesn't start with '-' - no flag | ||
None | ||
} | ||
|
||
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 trailing_newline = true; | ||
let mut escape = false; | ||
let mut args_iter = args.into_iter(); | ||
|
||
// Process arguments until first non-flag is found | ||
for arg in &mut args_iter { | ||
if let Some(echo_flags) = is_echo_flag(&arg) { | ||
// Single hyphen stops flag processing | ||
if echo_flags.is_single_hyphen { | ||
result.push(arg); | ||
break; | ||
} | ||
if echo_flags.disable_newline { | ||
trailing_newline = false; | ||
} | ||
escape = echo_flags.escape; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is too aggressive since it means There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great observation. Indeed this was an issue. I fixed this and also added some test as there was no test in the test suite so far to check for this behaviour. |
||
} else { | ||
// First non-flag argument stops flag processing | ||
result.push(arg); | ||
break; | ||
} | ||
} | ||
// Collect remaining arguments | ||
for arg in args_iter { | ||
result.push(arg); | ||
} | ||
(result, trailing_newline, 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 = if let Ok(posixly_correct) = env::var("POSIXLY_CORRECT") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain this change. As far as I know GNU checks whether the env var is defined not whether it's 1. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wasn't sure about this. I checked in the test files and there it was set to be equal to one.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reverted the change to now only check if it is defined at all |
||
posixly_correct == "1" | ||
} else { | ||
false | ||
}; | ||
|
||
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) | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work? I think the
is_single_hyphen
case is identical to theNone
caseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also correct. Changed this part too. Thanks!