diff --git a/src/uu/fmt/src/fmt.rs b/src/uu/fmt/src/fmt.rs index 04aad88839..86d4573b04 100644 --- a/src/uu/fmt/src/fmt.rs +++ b/src/uu/fmt/src/fmt.rs @@ -87,10 +87,24 @@ impl FmtOptions { .get_one::(options::SKIP_PREFIX) .map(String::from); - let width_opt = extract_width(matches); - let goal_opt = matches.get_one::(options::GOAL); + let width_opt = extract_width(matches)?; + let goal_opt_str = matches.get_one::(options::GOAL); + let goal_opt = if let Some(goal_str) = goal_opt_str { + match goal_str.parse::() { + Ok(goal) => Some(goal), + Err(_) => { + return Err(USimpleError::new( + 1, + format!("invalid goal: {}", goal_str.quote()), + )); + } + } + } else { + None + }; + let (width, goal) = match (width_opt, goal_opt) { - (Some(w), Some(&g)) => { + (Some(w), Some(g)) => { if g > w { return Err(USimpleError::new(1, "GOAL cannot be greater than WIDTH.")); } @@ -101,7 +115,7 @@ impl FmtOptions { let g = (w * DEFAULT_GOAL_TO_WIDTH_RATIO / 100).max(if w == 0 { 0 } else { 1 }); (w, g) } - (None, Some(&g)) => { + (None, Some(g)) => { if g > DEFAULT_WIDTH { return Err(USimpleError::new(1, "GOAL cannot be greater than WIDTH.")); } @@ -243,22 +257,29 @@ fn extract_files(matches: &ArgMatches) -> UResult> { } } -fn extract_width(matches: &ArgMatches) -> Option { - let width_opt = matches.get_one::(options::WIDTH); - if let Some(width) = width_opt { - return Some(*width); +fn extract_width(matches: &ArgMatches) -> UResult> { + let width_opt = matches.get_one::(options::WIDTH); + if let Some(width_str) = width_opt { + if let Ok(width) = width_str.parse::() { + return Ok(Some(width)); + } else { + return Err(USimpleError::new( + 1, + format!("invalid width: {}", width_str.quote()), + )); + } } if let Some(1) = matches.index_of(options::FILES_OR_WIDTH) { let width_arg = matches.get_one::(options::FILES_OR_WIDTH).unwrap(); if let Some(num) = width_arg.strip_prefix('-') { - num.parse::().ok() + Ok(num.parse::().ok()) } else { // will be treated as a file name - None + Ok(None) } } else { - None + Ok(None) } } @@ -406,16 +427,16 @@ pub fn uu_app() -> Command { .short('w') .long("width") .help("Fill output lines up to a maximum of WIDTH columns, default 75. This can be specified as a negative number in the first argument.") - .value_name("WIDTH") - .value_parser(clap::value_parser!(usize)), + // We must accept invalid values if they are overridden later. This is not supported by clap, so accept all strings instead. + .value_name("WIDTH"), ) .arg( Arg::new(options::GOAL) .short('g') .long("goal") .help("Goal width, default of 93% of WIDTH. Must be less than or equal to WIDTH.") - .value_name("GOAL") - .value_parser(clap::value_parser!(usize)), + // We must accept invalid values if they are overridden later. This is not supported by clap, so accept all strings instead. + .value_name("GOAL"), ) .arg( Arg::new(options::QUICK) @@ -460,7 +481,7 @@ mod tests { assert_eq!(extract_files(&matches).unwrap(), vec!["some-file"]); - assert_eq!(extract_width(&matches), Some(3)); + assert_eq!(extract_width(&matches).ok(), Some(Some(3))); Ok(()) } @@ -471,7 +492,7 @@ mod tests { assert_eq!(extract_files(&matches).unwrap(), vec!["some-file"]); - assert_eq!(extract_width(&matches), Some(3)); + assert_eq!(extract_width(&matches).ok(), Some(Some(3))); Ok(()) } @@ -482,7 +503,7 @@ mod tests { assert_eq!(extract_files(&matches).unwrap(), vec!["-"]); - assert_eq!(extract_width(&matches), None); + assert_eq!(extract_width(&matches).ok(), Some(None)); Ok(()) } @@ -493,7 +514,7 @@ mod tests { assert_eq!(extract_files(&matches).unwrap(), vec!["some-file"]); - assert_eq!(extract_width(&matches), None); + assert_eq!(extract_width(&matches).ok(), Some(None)); Ok(()) } @@ -504,7 +525,7 @@ mod tests { assert_eq!(extract_files(&matches).unwrap(), vec!["some-file"]); - assert_eq!(extract_width(&matches), Some(3)); + assert_eq!(extract_width(&matches).ok(), Some(Some(3))); Ok(()) } } diff --git a/tests/by-util/test_fmt.rs b/tests/by-util/test_fmt.rs index d56c4f760f..9bb82ede56 100644 --- a/tests/by-util/test_fmt.rs +++ b/tests/by-util/test_fmt.rs @@ -41,6 +41,21 @@ fn test_fmt_width() { .stdout_is("this is a\nfile with\none word\nper line\n"); } +#[test] +fn test_fmt_width_invalid() { + new_ucmd!() + .args(&["one-word-per-line.txt", "-w", "apple"]) + .fails() + .code_is(1) + .no_stdout() + .stderr_is("fmt: invalid width: 'apple'\n"); + // an invalid width can be successfully overwritten later: + new_ucmd!() + .args(&["one-word-per-line.txt", "-w", "apple", "-w10"]) + .succeeds() + .stdout_is("this is a\nfile with\none word\nper line\n"); +} + #[test] fn test_fmt_positional_width() { new_ucmd!() @@ -84,7 +99,7 @@ fn test_fmt_invalid_width() { .args(&["one-word-per-line.txt", param, "invalid"]) .fails() .code_is(1) - .stderr_contains("invalid value 'invalid'"); + .stderr_contains("invalid width: 'invalid'"); } } @@ -182,10 +197,36 @@ fn test_fmt_invalid_goal() { .args(&["one-word-per-line.txt", param, "invalid"]) .fails() .code_is(1) - .stderr_contains("invalid value 'invalid'"); + // GNU complains about "invalid width", which is confusing. + // We intentionally deviate from GNU, and show a more helpful message: + .stderr_contains("invalid goal: 'invalid'"); } } +#[test] +fn test_fmt_invalid_goal_override() { + new_ucmd!() + .args(&["one-word-per-line.txt", "-g", "apple", "-g", "74"]) + .succeeds() + .stdout_is("this is a file with one word per line\n"); +} + +#[test] +fn test_fmt_invalid_goal_width_priority() { + new_ucmd!() + .args(&["one-word-per-line.txt", "-g", "apple", "-w", "banana"]) + .fails() + .code_is(1) + .no_stdout() + .stderr_is("fmt: invalid width: 'banana'\n"); + new_ucmd!() + .args(&["one-word-per-line.txt", "-w", "banana", "-g", "apple"]) + .fails() + .code_is(1) + .no_stdout() + .stderr_is("fmt: invalid width: 'banana'\n"); +} + #[test] fn test_fmt_set_goal_not_contain_width() { for param in ["-g", "--goal"] {