tac: exit with proper code, move from getopts to clap, add test for invalid inputs (#1957)

Cargo.lock

@@ -2323,9 +2323,9 @@ dependencies = [
 name = "uu_tac"
 version = "0.0.4"
 dependencies = [
- "getopts",
- "uucore",
- "uucore_procs",
+ "clap 2.33.3 (registry+https://github.com/rust-lang/crates.io-index)",
+ "uucore 0.0.7",
+ "uucore_procs 0.0.5",
 ]
 
 [[package]]

src/uu/tac/Cargo.toml

@@ -15,7 +15,7 @@ edition = "2018"
 path = "src/tac.rs"
 
 [dependencies]
-getopts = "0.2.18"
+clap = "2.33"
 uucore = { version=">=0.0.7", package="uucore", path="../../uucore" }
 uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" }
 

src/uu/tac/src/tac.rs

@@ -10,79 +10,77 @@
 #[macro_use]
 extern crate uucore;
 
-use std::fs::File;
+use clap::{App, Arg};
 use std::io::{stdin, stdout, BufReader, Read, Stdout, Write};
+use std::{fs::File, path::Path};
 
 static NAME: &str = "tac";
 static VERSION: &str = env!("CARGO_PKG_VERSION");
+static USAGE: &str = "[OPTION]... [FILE]...";
+static SUMMARY: &str = "Write each file to standard output, last line first.";
+
+mod options {
+    pub static BEFORE: &str = "before";
+    pub static REGEX: &str = "regex";
+    pub static SEPARATOR: &str = "separator";
+    pub static FILE: &str = "file";
+}
 
 pub fn uumain(args: impl uucore::Args) -> i32 {
     let args = args.collect_str();
 
-    let mut opts = getopts::Options::new();
+    let matches = App::new(executable!())
+        .name(NAME)
+        .version(VERSION)
+        .usage(USAGE)
+        .about(SUMMARY)
+        .arg(
+            Arg::with_name(options::BEFORE)
+                .short("b")
+                .long(options::BEFORE)
+                .help("attach the separator before instead of after")
+                .takes_value(false),
+        )
+        .arg(
+            Arg::with_name(options::REGEX)
+                .short("r")
+                .long(options::REGEX)
+                .help("interpret the sequence as a regular expression (NOT IMPLEMENTED)")
+                .takes_value(false),
+        )
+        .arg(
+            Arg::with_name(options::SEPARATOR)
+                .short("s")
+                .long(options::SEPARATOR)
+                .help("use STRING as the separator instead of newline")
+                .takes_value(true),
+        )
+        .arg(Arg::with_name(options::FILE).hidden(true).multiple(true))
+        .get_matches_from(args);
 
-    opts.optflag(
-        "b",
-        "before",
-        "attach the separator before instead of after",
-    );
-    opts.optflag(
-        "r",
-        "regex",
-        "interpret the sequence as a regular expression (NOT IMPLEMENTED)",
-    );
-    opts.optopt(
-        "s",
-        "separator",
-        "use STRING as the separator instead of newline",
-        "STRING",
-    );
-    opts.optflag("h", "help", "display this help and exit");
-    opts.optflag("V", "version", "output version information and exit");
-
-    let matches = match opts.parse(&args[1..]) {
-        Ok(m) => m,
-        Err(f) => crash!(1, "{}", f),
-    };
-    if matches.opt_present("help") {
-        let msg = format!(
-            "{0} {1}
-
-Usage:
-  {0} [OPTION]... [FILE]...
-
-Write each file to standard output, last line first.",
-            NAME, VERSION
-        );
-
-        print!("{}", opts.usage(&msg));
-    } else if matches.opt_present("version") {
-        println!("{} {}", NAME, VERSION);
-    } else {
-        let before = matches.opt_present("b");
-        let regex = matches.opt_present("r");
-        let separator = match matches.opt_str("s") {
-            Some(m) => {
-                if m.is_empty() {
-                    crash!(1, "separator cannot be empty")
-                } else {
-                    m
-                }
+    let before = matches.is_present(options::BEFORE);
+    let regex = matches.is_present(options::REGEX);
+    let separator = match matches.value_of(options::SEPARATOR) {
+        Some(m) => {
+            if m.is_empty() {
+                crash!(1, "separator cannot be empty")
+            } else {
+                m.to_owned()
             }
-            None => "\n".to_owned(),
-        };
-        let files = if matches.free.is_empty() {
-            vec!["-".to_owned()]
-        } else {
-            matches.free
-        };
-        tac(files, before, regex, &separator[..]);
-    }
+        }
+        None => "\n".to_owned(),
+    };
 
-    0
+    let files: Vec<String> = match matches.values_of(options::FILE) {
+        Some(v) => v.map(|v| v.to_owned()).collect(),
+        None => vec!["-".to_owned()],
+    };
+
+    tac(files, before, regex, &separator[..])
 }
 
-fn tac(filenames: Vec<String>, before: bool, _: bool, separator: &str) {
+fn tac(filenames: Vec<String>, before: bool, _: bool, separator: &str) -> i32 {
+    let mut exit_code = 0;
     let mut out = stdout();
     let sbytes = separator.as_bytes();
     let slen = sbytes.len();
@@ -91,10 +89,19 @@ fn tac(filenames: Vec<String>, before: bool, _: bool, separator: &str) {
         let mut file = BufReader::new(if filename == "-" {
             Box::new(stdin()) as Box<dyn Read>
         } else {
-            match File::open(filename) {
+            let path = Path::new(filename);
+            if path.is_dir() || !path.metadata().is_ok() {
+                show_error!(
+                    "failed to open '{}' for reading: No such file or directory",
+                    filename
+                );
+                continue;
+            }
+            match File::open(path) {
                 Ok(f) => Box::new(f) as Box<dyn Read>,
                 Err(e) => {
-                    show_warning!("failed to open '{}' for reading: {}", filename, e);
+                    show_error!("failed to open '{}' for reading: {}", filename, e);
+                    exit_code = 1;
                     continue;
                 }
             }
@@ -102,7 +109,8 @@ fn tac(filenames: Vec<String>, before: bool, _: bool, separator: &str) {
 
         let mut data = Vec::new();
         if let Err(e) = file.read_to_end(&mut data) {
-            show_warning!("failed to read '{}': {}", filename, e);
+            show_error!("failed to read '{}': {}", filename, e);
+            exit_code = 1;
             continue;
         };
 
@@ -141,6 +149,8 @@ fn tac(filenames: Vec<String>, before: bool, _: bool, separator: &str) {
         }
         show_line(&mut out, sbytes, &data[0..prev], before);
     }
+
+    exit_code
 }
 
 fn show_line(out: &mut Stdout, sep: &[u8], dat: &[u8], before: bool) {

tests/by-util/test_tac.rs

@@ -49,3 +49,21 @@ fn test_single_non_newline_separator_before() {
         .run()
         .stdout_is_fixture("delimited_primes_before.expected");
 }
+
+#[test]
+fn test_invalid_input() {
+    let (_, mut ucmd) = at_and_ucmd!();
+
+    ucmd.arg("b")
+        .run()
+        .stderr
+        .contains("tac: error: failed to open 'b' for reading");
+
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.mkdir("a");
+    ucmd.arg("a")
+        .run()
+        .stderr
+        .contains("tac: error: failed to read 'a'");
+}