diff --git a/src/uu/nl/locales/en-US.ftl b/src/uu/nl/locales/en-US.ftl index 7e2a73e7336..ec11a7d7cde 100644 --- a/src/uu/nl/locales/en-US.ftl +++ b/src/uu/nl/locales/en-US.ftl @@ -28,10 +28,14 @@ nl-help-starting-line-number = first line number on each logical page nl-help-number-width = use NUMBER columns for line numbers # Error messages -nl-error-invalid-arguments = Invalid arguments supplied. nl-error-could-not-read-line = could not read line nl-error-could-not-write = could not write output nl-error-line-number-overflow = line number overflow -nl-error-invalid-regex = invalid regular expression -nl-error-invalid-numbering-style = invalid numbering style: '{ $style }' +nl-error-invalid-regex = Invalid regular expression +nl-error-invalid-numbering-style = invalid { $kind } numbering style: '{ $value }' +nl-error-invalid-number-format = invalid line numbering format: '{ $value }' +nl-error-invalid-number = invalid { $kind }: '{ $value }' +nl-error-number-out-of-range = invalid { $kind }: '{ $value }': Numerical result out of range +nl-error-number-too-large = invalid { $kind }: '{ $value }': Value too large for defined data type +nl-error-try-help = Try 'nl --help' for more information. nl-error-is-directory = { $path }: Is a directory diff --git a/src/uu/nl/locales/fr-FR.ftl b/src/uu/nl/locales/fr-FR.ftl index 1a9a9ecdd6c..fe43402e5da 100644 --- a/src/uu/nl/locales/fr-FR.ftl +++ b/src/uu/nl/locales/fr-FR.ftl @@ -28,10 +28,14 @@ nl-help-starting-line-number = premier numéro de ligne sur chaque page logique nl-help-number-width = utiliser NUMBER colonnes pour les numéros de ligne # Messages d'erreur -nl-error-invalid-arguments = Arguments fournis invalides. nl-error-could-not-read-line = impossible de lire la ligne nl-error-could-not-write = impossible d'écrire la sortie nl-error-line-number-overflow = débordement du numéro de ligne -nl-error-invalid-regex = expression régulière invalide -nl-error-invalid-numbering-style = style de numérotation invalide : '{ $style }' +nl-error-invalid-regex = Expression régulière invalide +nl-error-invalid-numbering-style = style de numérotation { $kind } invalide : '{ $value }' +nl-error-invalid-number-format = format de numérotation de ligne invalide : '{ $value }' +nl-error-invalid-number = { $kind } invalide : '{ $value }' +nl-error-number-out-of-range = { $kind } invalide : '{ $value }' : Résultat numérique hors limite +nl-error-number-too-large = { $kind } invalide : '{ $value }' : Valeur trop grande pour le type de données défini +nl-error-try-help = Essayez 'nl --help' pour plus d'informations. nl-error-is-directory = { $path } : Est un répertoire diff --git a/src/uu/nl/src/helper.rs b/src/uu/nl/src/helper.rs index 1f71e403ed5..bd093b867b1 100644 --- a/src/uu/nl/src/helper.rs +++ b/src/uu/nl/src/helper.rs @@ -5,17 +5,100 @@ // spell-checker:ignore (ToDO) conv use std::ffi::OsString; +use std::num::IntErrorKind; -use crate::options; +use uucore::translate; -// parse_options loads the options into the settings, returning an array of -// error messages. +use crate::{NumberingStyle, NumberingStyleError, options}; + +/// GNU reports a plain (no unit suffix) numeric option's own value with its +/// own wording, and immediately, fatally, exits on the first one it finds +/// invalid -- it does not keep parsing the rest of the command line the way +/// it does for the numbering-style options below. +/// +/// GNU distinguishes two kinds of "doesn't fit": a value that doesn't fit +/// in `overflow_at`'s own underlying integer width at all (e.g. `-w`'s +/// value is stored in a plain C `int`, so anything outside `i32` overflows +/// it even though it parses as an `i64` just fine) is reported as "Value +/// too large for defined data type"; one that fits that width but falls +/// outside the option's own accepted `range` (e.g. `-w` additionally +/// requires a *positive* value) is reported as "Numerical result out of +/// range" instead. `kind` names which option this is, for the message. +fn parse_nl_number( + value: &str, + kind: &'static str, + overflow_at: std::ops::RangeInclusive, + range: std::ops::RangeInclusive, +) -> Result { + match value.parse::() { + Ok(n) if range.contains(&n) => Ok(n), + Ok(n) if overflow_at.contains(&n) => Err( + translate!("nl-error-number-out-of-range", "kind" => kind, "value" => value.to_owned()), + ), + Ok(_) => Err( + translate!("nl-error-number-too-large", "kind" => kind, "value" => value.to_owned()), + ), + Err(e) + if matches!( + e.kind(), + IntErrorKind::PosOverflow | IntErrorKind::NegOverflow + ) => + { + Err( + translate!("nl-error-number-too-large", "kind" => kind, "value" => value.to_owned()), + ) + } + Err(_) => { + Err(translate!("nl-error-invalid-number", "kind" => kind, "value" => value.to_owned())) + } + } +} + +// parse_options loads the options into the settings, returning either the +// first immediately-fatal error found (matching GNU, which stops parsing the +// rest of the command line at that point), or the list of any non-fatal +// numbering-style/format errors collected along the way. #[allow(clippy::cognitive_complexity)] -pub fn parse_options(settings: &mut crate::Settings, opts: &clap::ArgMatches) -> Vec { - // This vector holds error messages encountered. +pub fn parse_options( + settings: &mut crate::Settings, + opts: &clap::ArgMatches, +) -> Result, String> { let mut errs: Vec = vec![]; settings.renumber = opts.get_flag(options::NO_RENUMBER); + if let Some(value) = opts.get_one::(options::LINE_INCREMENT) { + settings.line_increment = parse_nl_number( + value, + "line number increment", + i64::MIN..=i64::MAX, + i64::MIN..=i64::MAX, + )?; + } + if let Some(value) = opts.get_one::(options::JOIN_BLANK_LINES) { + settings.join_blank_lines = parse_nl_number( + value, + "line number of blank lines", + i64::MIN..=i64::MAX, + 0..=i64::MAX, + )? as u64; + } + if let Some(value) = opts.get_one::(options::STARTING_LINE_NUMBER) { + settings.starting_line_number = parse_nl_number( + value, + "starting line number", + i64::MIN..=i64::MAX, + i64::MIN..=i64::MAX, + )?; + } + if let Some(value) = opts.get_one::(options::NUMBER_WIDTH) { + settings.number_width = parse_nl_number( + value, + "line number field width", + i64::from(i32::MIN)..=i64::from(i32::MAX), + 1..=i64::from(i32::MAX), + )? as usize; + } + if let Some(mut delimiter) = opts .get_one::(options::SECTION_DELIMITER) .cloned() @@ -35,48 +118,55 @@ pub fn parse_options(settings: &mut crate::Settings, opts: &clap::ArgMatches) -> if let Some(val) = opts.get_one::(options::NUMBER_SEPARATOR) { settings.number_separator.clone_from(val); } - settings.number_format = opts - .get_one::(options::NUMBER_FORMAT) - .map(Into::into) - .unwrap_or_default(); - match opts - .get_one::(options::HEADER_NUMBERING) - .map(String::as_str) - .map(TryInto::try_into) - { - None => {} - Some(Ok(style)) => settings.header_numbering = style, - Some(Err(message)) => errs.push(message), - } - match opts - .get_one::(options::BODY_NUMBERING) - .map(String::as_str) - .map(TryInto::try_into) - { - None => {} - Some(Ok(style)) => settings.body_numbering = style, - Some(Err(message)) => errs.push(message), - } - match opts - .get_one::(options::FOOTER_NUMBERING) - .map(String::as_str) - .map(TryInto::try_into) - { - None => {} - Some(Ok(style)) => settings.footer_numbering = style, - Some(Err(message)) => errs.push(message), - } - if let Some(&num) = opts.get_one::(options::NUMBER_WIDTH) { - settings.number_width = num as usize; - } - if let Some(num) = opts.get_one::(options::JOIN_BLANK_LINES) { - settings.join_blank_lines = *num; - } - if let Some(num) = opts.get_one::(options::LINE_INCREMENT) { - settings.line_increment = *num; + + // GNU reports these four in the order they were actually given on the + // command line, not in any fixed order, so each is paired with its own + // position for a final sort. + let mut style_errs: Vec<(usize, String)> = vec![]; + + if let Some(format) = opts.get_one::(options::NUMBER_FORMAT) { + match format.as_str() { + "ln" | "rn" | "rz" => settings.number_format = format.clone().into(), + _ => style_errs.push(( + opts.index_of(options::NUMBER_FORMAT).unwrap_or(0), + translate!("nl-error-invalid-number-format", "value" => format.clone()), + )), + } } - if let Some(num) = opts.get_one::(options::STARTING_LINE_NUMBER) { - settings.starting_line_number = *num; + + for (opt, kind, field) in [ + ( + options::HEADER_NUMBERING, + "header", + &mut settings.header_numbering, + ), + ( + options::BODY_NUMBERING, + "body", + &mut settings.body_numbering, + ), + ( + options::FOOTER_NUMBERING, + "footer", + &mut settings.footer_numbering, + ), + ] { + if let Some(style) = opts.get_one::(opt) { + match NumberingStyle::try_from(style.as_str()) { + Ok(numbering) => *field = numbering, + Err(NumberingStyleError::InvalidRegex) => { + return Err(translate!("nl-error-invalid-regex")); + } + Err(NumberingStyleError::InvalidStyle) => style_errs.push(( + opts.index_of(opt).unwrap_or(0), + translate!("nl-error-invalid-numbering-style", "kind" => kind, "value" => style.clone()), + )), + } + } } - errs + + style_errs.sort_by_key(|(index, _)| *index); + errs.extend(style_errs.into_iter().map(|(_, message)| message)); + + Ok(errs) } diff --git a/src/uu/nl/src/nl.rs b/src/uu/nl/src/nl.rs index c94a684c168..939950c430f 100644 --- a/src/uu/nl/src/nl.rs +++ b/src/uu/nl/src/nl.rs @@ -80,8 +80,19 @@ enum NumberingStyle { Regex(Box), } +/// Which of the two ways a numbering style argument can fail: with an +/// invalid style word, or -- for a `p`-prefixed one -- an invalid regular +/// expression. The caller picks the message: GNU's wording for the first +/// depends on which of the header/body/footer/format options it came +/// from, and its "Try --help" hint depends on whether it's a style word +/// at all, so this can't be a single, already-translated `String`. +enum NumberingStyleError { + InvalidStyle, + InvalidRegex, +} + impl TryFrom<&str> for NumberingStyle { - type Error = String; + type Error = NumberingStyleError; fn try_from(s: &str) -> Result { match s { @@ -90,9 +101,9 @@ impl TryFrom<&str> for NumberingStyle { "n" => Ok(Self::None), _ if s.starts_with('p') => match regex::bytes::Regex::new(&s[1..]) { Ok(re) => Ok(Self::Regex(Box::new(re))), - Err(_) => Err(translate!("nl-error-invalid-regex")), + Err(_) => Err(NumberingStyleError::InvalidRegex), }, - _ => Err(translate!("nl-error-invalid-numbering-style", "style" => s)), + _ => Err(NumberingStyleError::InvalidStyle), } } } @@ -220,17 +231,26 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let mut settings = Settings::default(); // Update the settings from the command line options, and terminate the - // program if some options could not successfully be parsed. - let parse_errors = helper::parse_options(&mut settings, &matches); - if !parse_errors.is_empty() { - return Err(USimpleError::new( - 1, - format!( - "{}\n{}", - translate!("nl-error-invalid-arguments"), - parse_errors.join("\n") - ), - )); + // program if some options could not successfully be parsed. GNU stops + // immediately, with no "Try --help" hint, on the first option whose + // own value is invalid (an immediately-fatal `Err` here); it instead + // collects every bad numbering-style/format option and reports them + // all together with a single trailing hint at the end. + match helper::parse_options(&mut settings, &matches) { + Ok(errs) if errs.is_empty() => {} + Ok(errs) => { + // The eventual `show_error!` only prefixes the message's first + // line with "nl: "; every other error line needs its own. + let mut lines = errs.into_iter(); + let mut message = lines.next().unwrap_or_default(); + for err in lines { + message.push_str(&format!("\nnl: {err}")); + } + message.push('\n'); + message.push_str(&translate!("nl-error-try-help")); + return Err(USimpleError::new(1, message)); + } + Err(err) => return Err(USimpleError::new(1, err)), } let files: Vec = match matches.get_many::(options::FILE) { @@ -329,7 +349,7 @@ pub fn uu_app() -> Command { .long(options::LINE_INCREMENT) .help(translate!("nl-help-line-increment")) .value_name("NUMBER") - .value_parser(clap::value_parser!(i64)), + .allow_hyphen_values(true), ) .arg( Arg::new(options::JOIN_BLANK_LINES) @@ -337,15 +357,14 @@ pub fn uu_app() -> Command { .long(options::JOIN_BLANK_LINES) .help(translate!("nl-help-join-blank-lines")) .value_name("NUMBER") - .value_parser(clap::value_parser!(u64)), + .allow_hyphen_values(true), ) .arg( Arg::new(options::NUMBER_FORMAT) .short('n') .long(options::NUMBER_FORMAT) .help(translate!("nl-help-number-format")) - .value_name("FORMAT") - .value_parser(["ln", "rn", "rz"]), + .value_name("FORMAT"), ) .arg( Arg::new(options::NO_RENUMBER) @@ -368,7 +387,7 @@ pub fn uu_app() -> Command { .long(options::STARTING_LINE_NUMBER) .help(translate!("nl-help-starting-line-number")) .value_name("NUMBER") - .value_parser(clap::value_parser!(i64)), + .allow_hyphen_values(true), ) .arg( Arg::new(options::NUMBER_WIDTH) @@ -376,7 +395,7 @@ pub fn uu_app() -> Command { .long(options::NUMBER_WIDTH) .help(translate!("nl-help-number-width")) .value_name("NUMBER") - .value_parser(clap::value_parser!(u64).range(1..=(i32::MAX as u64))), + .allow_hyphen_values(true), ) } diff --git a/tests/by-util/test_nl.rs b/tests/by-util/test_nl.rs index d2cb7b509d6..ae6a66fd92a 100644 --- a/tests/by-util/test_nl.rs +++ b/tests/by-util/test_nl.rs @@ -159,7 +159,7 @@ fn test_invalid_number_format() { new_ucmd!() .arg(arg) .fails() - .stderr_contains("invalid value 'invalid'"); + .stderr_is("nl: invalid line numbering format: 'invalid'\nTry 'nl --help' for more information.\n"); } } @@ -180,10 +180,9 @@ fn test_number_width() { #[test] fn test_number_width_zero() { for arg in ["-w0", "--number-width=0"] { - new_ucmd!() - .arg(arg) - .fails() - .stderr_contains("is not in 1..=2147483647"); + new_ucmd!().arg(arg).fails().stderr_is( + "nl: invalid line number field width: '0': Numerical result out of range\n", + ); } } @@ -196,7 +195,7 @@ fn test_number_width_too_large() { .arg(arg) .pipe_in("") .fails() - .stderr_contains("is not in 1..=2147483647"); + .stderr_is("nl: invalid line number field width: '2147483648': Value too large for defined data type\n"); } } @@ -215,7 +214,7 @@ fn test_invalid_number_width() { new_ucmd!() .arg(arg) .fails() - .stderr_contains("invalid value 'invalid'"); + .stderr_is("nl: invalid line number field width: 'invalid'\n"); } } @@ -276,13 +275,29 @@ fn test_negative_starting_line_number() { } } +#[test] +fn test_negative_starting_line_number_as_separate_arg() { + // A negative value passed as its own argument (not attached with `-v-10` + // or `=`) must not be mistaken for a new, unrecognized flag. + new_ucmd!() + .args(&["-v", "-10"]) + .pipe_in("test") + .succeeds() + .stdout_is(" -10\ttest\n"); + new_ucmd!() + .args(&["--starting-line-number", "-10"]) + .pipe_in("test") + .succeeds() + .stdout_is(" -10\ttest\n"); +} + #[test] fn test_invalid_starting_line_number() { for arg in ["-vinvalid", "--starting-line-number=invalid"] { new_ucmd!() .arg(arg) .fails() - .stderr_contains("invalid value 'invalid'"); + .stderr_is("nl: invalid starting line number: 'invalid'\n"); } } @@ -320,13 +335,29 @@ fn test_negative_line_increment() { } } +#[test] +fn test_negative_line_increment_as_separate_arg() { + // A negative value passed as its own argument (not attached with `-i-10` + // or `=`) must not be mistaken for a new, unrecognized flag. + new_ucmd!() + .args(&["-i", "-10"]) + .pipe_in("a\nb\nc") + .succeeds() + .stdout_is(" 1\ta\n -9\tb\n -19\tc\n"); + new_ucmd!() + .args(&["--line-increment", "-10"]) + .pipe_in("a\nb\nc") + .succeeds() + .stdout_is(" 1\ta\n -9\tb\n -19\tc\n"); +} + #[test] fn test_invalid_line_increment() { for arg in ["-iinvalid", "--line-increment=invalid"] { new_ucmd!() .arg(arg) .fails() - .stderr_contains("invalid value 'invalid'"); + .stderr_is("nl: invalid line number increment: 'invalid'\n"); } } @@ -399,7 +430,7 @@ fn test_invalid_join_blank_lines() { new_ucmd!() .arg(arg) .fails() - .stderr_contains("invalid value 'invalid'"); + .stderr_is("nl: invalid line number of blank lines: 'invalid'\n"); } } @@ -573,19 +604,18 @@ fn test_numbering_matched_lines() { #[test] fn test_invalid_numbering() { let invalid_args = [ - "-hinvalid", - "--header-numbering=invalid", - "-binvalid", - "--body-numbering=invalid", - "-finvalid", - "--footer-numbering=invalid", + ("-hinvalid", "header"), + ("--header-numbering=invalid", "header"), + ("-binvalid", "body"), + ("--body-numbering=invalid", "body"), + ("-finvalid", "footer"), + ("--footer-numbering=invalid", "footer"), ]; - for invalid_arg in invalid_args { - new_ucmd!() - .arg(invalid_arg) - .fails() - .stderr_contains("invalid numbering style: 'invalid'"); + for (invalid_arg, kind) in invalid_args { + new_ucmd!().arg(invalid_arg).fails().stderr_is(format!( + "nl: invalid {kind} numbering style: 'invalid'\nTry 'nl --help' for more information.\n" + )); } } @@ -604,7 +634,7 @@ fn test_invalid_regex_numbering() { new_ucmd!() .arg(invalid_arg) .fails() - .stderr_contains("invalid regular expression"); + .stderr_is("nl: Invalid regular expression\n"); } }