From 8949065b5d15265c5322e0e4394dad2a98e23554 Mon Sep 17 00:00:00 2001 From: arbelonson-source <269032023+arbelonson-source@users.noreply.github.com> Date: Mon, 31 Aug 2026 09:13:05 +0300 Subject: [PATCH] nl: match GNU's errors for its numeric and numbering-style option values nl's numeric options (-i/--line-increment, -l/--join-blank-lines, -v/--starting-line-number, -w/--number-width) validated their values with clap's own i64/u64 value_parser, so: - a negative value passed as its own argument (`-i -5`, not the attached `-i-5`) was rejected as an unrecognized flag instead of being read as the option's value, since these Args did not have allow_hyphen_values; - an invalid value produced clap's own generic wording instead of GNU's (e.g. 'invalid digit found in string' instead of "invalid line number increment: 'bogus'"), and did not distinguish GNU's two distinct overflow messages: a value that overflows the *option's own* underlying integer width (e.g. --number-width is a plain C int, so anything outside i32 -- even though it parses as an i64 just fine) is "Value too large for defined data type", while one that fits that width but falls outside the option's accepted range (e.g. --number-width also requires a positive value) is "Numerical result out of range". Also fixes the header/body/footer numbering style options (-h, -b, -f) and the number format option (-n), which validated via a plain value_parser and clap's own PossibleValuesParser respectively, same underlying wording problem. GNU reports each of these with a field-specific message ("invalid header numbering style", vs "body" vs "footer", vs "invalid line numbering format" for -n) that a single generic message can't produce, appends 'Try --help' (which the numeric options and an invalid regular expression for -h/-b/-f do not), and -- unlike the numeric options, which stop parsing immediately at the first bad one -- collects every bad style/format option and reports them together, in the order they were actually given on the command line. Replaced the existing 'Invalid arguments supplied.' preamble + joined list, which didn't match any of this, with per-error 'nl: ' prefixed lines (only the first of which the outer error handler adds for us) and a single trailing hint line when appropriate. AI-assisted-by: Claude Opus 5, via Claude Code --- src/uu/nl/locales/en-US.ftl | 10 +- src/uu/nl/locales/fr-FR.ftl | 10 +- src/uu/nl/src/helper.rs | 184 +++++++++++++++++++++++++++--------- src/uu/nl/src/nl.rs | 59 ++++++++---- tests/by-util/test_nl.rs | 74 ++++++++++----- 5 files changed, 242 insertions(+), 95 deletions(-) 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"); } }