Merge pull request #5225 from tertsdiepraam/factor-short-circuit-write-error

`factor`: short circuit on write error, but not on parse error

src/uu/factor/src/cli.rs

@@ -3,8 +3,6 @@
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
 
-use std::error::Error;
-use std::fmt::Write as FmtWrite;
 use std::io::BufRead;
 use std::io::{self, stdin, stdout, Write};
 
@@ -12,7 +10,7 @@ mod factor;
 use clap::{crate_version, Arg, ArgAction, Command};
 pub use factor::*;
 use uucore::display::Quotable;
-use uucore::error::UResult;
+use uucore::error::{set_exit_code, FromIo, UResult};
 use uucore::{format_usage, help_about, help_usage, show_error, show_warning};
 
 mod miller_rabin;
@@ -32,26 +30,27 @@ mod options {
 fn print_factors_str(
     num_str: &str,
     w: &mut io::BufWriter<impl io::Write>,
-    factors_buffer: &mut String,
     print_exponents: bool,
-) -> Result<(), Box<dyn Error>> {
+) -> io::Result<()> {
-    num_str
+    let x = match num_str.trim().parse::<u64>() {
-        .trim()
+        Ok(x) => x,
-        .parse::<u64>()
+        Err(e) => {
-        .map_err(|e| e.into())
+            // We return Ok() instead of Err(), because it's non-fatal and we should try the next
-        .and_then(|x| {
+            // number.
-            factors_buffer.clear();
+            show_warning!("{}: {}", num_str.maybe_quote(), e);
-            // If print_exponents is true, use the alternate format specifier {:#} from fmt to print the factors
+            set_exit_code(1);
-            // of x in the form of p^e.
+            return Ok(());
-            if print_exponents {
+        }
-                writeln!(factors_buffer, "{}:{:#}", x, factor(x))?;
+    };
-            } else {
+
-                writeln!(factors_buffer, "{}:{}", x, factor(x))?;
+    // If print_exponents is true, use the alternate format specifier {:#} from fmt to print the factors
-            }
+    // of x in the form of p^e.
-            w.write_all(factors_buffer.as_bytes())?;
+    if print_exponents {
-            w.flush()?;
+        writeln!(w, "{}:{:#}", x, factor(x))?;
-            Ok(())
+    } else {
-        })
+        writeln!(w, "{}:{}", x, factor(x))?;
+    }
+    w.flush()
 }
 
 #[uucore::main]
@@ -64,25 +63,19 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let stdout = stdout();
     // We use a smaller buffer here to pass a gnu test. 4KiB appears to be the default pipe size for bash.
     let mut w = io::BufWriter::with_capacity(4 * 1024, stdout.lock());
-    let mut factors_buffer = String::new();
 
     if let Some(values) = matches.get_many::<String>(options::NUMBER) {
         for number in values {
-            if let Err(e) = print_factors_str(number, &mut w, &mut factors_buffer, print_exponents)
+            print_factors_str(number, &mut w, print_exponents)
-            {
+                .map_err_context(|| "write error".into())?;
-                show_warning!("{}: {}", number.maybe_quote(), e);
-            }
         }
     } else {
         let stdin = stdin();
         let lines = stdin.lock().lines();
         for line in lines {
             for number in line.unwrap().split_whitespace() {
-                if let Err(e) =
+                print_factors_str(number, &mut w, print_exponents)
-                    print_factors_str(number, &mut w, &mut factors_buffer, print_exponents)
+                    .map_err_context(|| "write error".into())?;
-                {
-                    show_warning!("{}: {}", number.maybe_quote(), e);
-                }
             }
         }
     }

tests/by-util/test_factor.rs

@@ -337,6 +337,37 @@ fn test_primes_with_exponents() {
         .stdout_is(String::from_utf8(output_string.as_bytes().to_owned()).unwrap());
 }
 
+#[test]
+fn fails_on_invalid_number() {
+    new_ucmd!().arg("not-a-valid-number").fails();
+    new_ucmd!()
+        .arg("not-a-valid-number")
+        .arg("12")
+        .fails()
+        .stdout_contains("12: 2 2 3");
+}
+
+#[test]
+#[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "netbsd"))]
+fn short_circuit_write_error() {
+    use std::fs::OpenOptions;
+
+    // Check that the error is printed exactly once and factor does not move on
+    // to the next number when a write error happens.
+    //
+    // Note: Technically, GNU prints the error twice, not because it does not
+    // short circuit the error, but because it always prints the error twice,
+    // for any number of inputs. That's silly behavior and printing once is
+    // clearly better.
+    let f = OpenOptions::new().write(true).open("/dev/full").unwrap();
+    new_ucmd!()
+        .arg("12")
+        .arg("10")
+        .set_stdout(f)
+        .fails()
+        .stderr_is("factor: write error: No space left on device\n");
+}
+
 const PRIMES_BY_BITS: &[&[u64]] = &[
     PRIMES14, PRIMES15, PRIMES16, PRIMES17, PRIMES18, PRIMES19, PRIMES20, PRIMES21, PRIMES22,
     PRIMES23, PRIMES24, PRIMES25, PRIMES26, PRIMES27, PRIMES28, PRIMES29, PRIMES30, PRIMES31,