fmt: fix error priority, make goal-errors more helpful

src/uu/fmt/src/fmt.rs

@@ -87,10 +87,24 @@ impl FmtOptions {
             .get_one::<String>(options::SKIP_PREFIX)
             .map(String::from);
 
-        let width_opt = extract_width(matches);
+        let width_opt = extract_width(matches)?;
-        let goal_opt = matches.get_one::<usize>(options::GOAL);
+        let goal_opt_str = matches.get_one::<String>(options::GOAL);
+        let goal_opt = if let Some(goal_str) = goal_opt_str {
+            match goal_str.parse::<usize>() {
+                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<Vec<String>> {
     }
 }
 
-fn extract_width(matches: &ArgMatches) -> Option<usize> {
+fn extract_width(matches: &ArgMatches) -> UResult<Option<usize>> {
-    let width_opt = matches.get_one::<usize>(options::WIDTH);
+    let width_opt = matches.get_one::<String>(options::WIDTH);
-    if let Some(width) = width_opt {
+    if let Some(width_str) = width_opt {
-        return Some(*width);
+        if let Ok(width) = width_str.parse::<usize>() {
+            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::<String>(options::FILES_OR_WIDTH).unwrap();
         if let Some(num) = width_arg.strip_prefix('-') {
-            num.parse::<usize>().ok()
+            Ok(num.parse::<usize>().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")
+                // We must accept invalid values if they are overridden later. This is not supported by clap, so accept all strings instead.
-                .value_parser(clap::value_parser!(usize)),
+                .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")
+                // We must accept invalid values if they are overridden later. This is not supported by clap, so accept all strings instead.
-                .value_parser(clap::value_parser!(usize)),
+                .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(())
     }
 }

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"] {