From 4f8d1c5fcfe1bff7971047e05895e7f896c675eb Mon Sep 17 00:00:00 2001 From: Daniel Eades Date: Sun, 30 Jan 2022 21:25:09 +0100 Subject: [PATCH 01/14] add additional lints --- .cargo/config | 9 +++++++++ src/bin/coreutils.rs | 8 ++++---- src/uu/chroot/src/chroot.rs | 6 +++--- src/uu/du/src/du.rs | 2 +- src/uu/env/src/env.rs | 1 + src/uu/kill/src/kill.rs | 6 +++--- src/uu/split/src/number.rs | 24 ++++++++++++------------ src/uu/split/src/split.rs | 6 +++--- src/uu/tail/src/platform/unix.rs | 1 + src/uu/tail/src/platform/windows.rs | 4 ++-- src/uucore/src/lib/features/fsext.rs | 8 ++++---- src/uucore/src/lib/features/wide.rs | 4 ++-- tests/by-util/test_du.rs | 6 +++--- tests/by-util/test_env.rs | 2 +- 14 files changed, 49 insertions(+), 38 deletions(-) diff --git a/.cargo/config b/.cargo/config index 58e1381b1..0a8fd3d00 100644 --- a/.cargo/config +++ b/.cargo/config @@ -1,2 +1,11 @@ [target.x86_64-unknown-redox] linker = "x86_64-unknown-redox-gcc" + +[target.'cfg(feature = "cargo-clippy")'] +rustflags = [ + "-Wclippy::use_self", + "-Wclippy::needless_pass_by_value", + "-Wclippy::semicolon_if_nothing_returned", + "-Wclippy::single_char_pattern", + "-Wclippy::explicit_iter_loop", +] diff --git a/src/bin/coreutils.rs b/src/bin/coreutils.rs index 901139edc..fcd86c93f 100644 --- a/src/bin/coreutils.rs +++ b/src/bin/coreutils.rs @@ -87,7 +87,7 @@ fn main() { }; if util == "completion" { - gen_completions(args, utils); + gen_completions(args, &utils); } match utils.get(util) { @@ -132,7 +132,7 @@ fn main() { /// Prints completions for the utility in the first parameter for the shell in the second parameter to stdout fn gen_completions( args: impl Iterator, - util_map: UtilityMap, + util_map: &UtilityMap, ) -> ! { let all_utilities: Vec<_> = std::iter::once("coreutils") .chain(util_map.keys().copied()) @@ -168,9 +168,9 @@ fn gen_completions( process::exit(0); } -fn gen_coreutils_app(util_map: UtilityMap) -> App<'static> { +fn gen_coreutils_app(util_map: &UtilityMap) -> App<'static> { let mut app = App::new("coreutils"); - for (_, (_, sub_app)) in &util_map { + for (_, (_, sub_app)) in util_map { app = app.subcommand(sub_app()); } app diff --git a/src/uu/chroot/src/chroot.rs b/src/uu/chroot/src/chroot.rs index 179880b14..f656ed77d 100644 --- a/src/uu/chroot/src/chroot.rs +++ b/src/uu/chroot/src/chroot.rs @@ -201,12 +201,12 @@ fn set_main_group(group: &str) -> UResult<()> { } #[cfg(any(target_vendor = "apple", target_os = "freebsd"))] -fn set_groups(groups: Vec) -> libc::c_int { +fn set_groups(groups: &[libc::gid_t]) -> libc::c_int { unsafe { setgroups(groups.len() as libc::c_int, groups.as_ptr()) } } #[cfg(target_os = "linux")] -fn set_groups(groups: Vec) -> libc::c_int { +fn set_groups(groups: &[libc::gid_t]) -> libc::c_int { unsafe { setgroups(groups.len() as libc::size_t, groups.as_ptr()) } } @@ -220,7 +220,7 @@ fn set_groups_from_str(groups: &str) -> UResult<()> { }; groups_vec.push(gid); } - let err = set_groups(groups_vec); + let err = set_groups(&groups_vec); if err != 0 { return Err(ChrootError::SetGroupsFailed(Error::last_os_error()).into()); } diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index 7629aba69..e75210ef5 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -144,7 +144,7 @@ impl Stat { #[cfg(windows)] let file_info = get_file_info(&path); #[cfg(windows)] - Ok(Stat { + Ok(Self { path, is_dir: metadata.is_dir(), size: metadata.len(), diff --git a/src/uu/env/src/env.rs b/src/uu/env/src/env.rs index 677ffe1bf..c0e94a578 100644 --- a/src/uu/env/src/env.rs +++ b/src/uu/env/src/env.rs @@ -104,6 +104,7 @@ fn load_config_file(opts: &mut Options) -> UResult<()> { } #[cfg(not(windows))] +#[allow(clippy::ptr_arg)] fn build_command<'a, 'b>(args: &'a mut Vec<&'b str>) -> (Cow<'b, str>, &'a [&'b str]) { let progname = Cow::from(args[0]); (progname, &args[1..]) diff --git a/src/uu/kill/src/kill.rs b/src/uu/kill/src/kill.rs index 02dc91dbf..413a183a8 100644 --- a/src/uu/kill/src/kill.rs +++ b/src/uu/kill/src/kill.rs @@ -74,7 +74,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { table(); Ok(()) } - Mode::List => list(pids_or_signals.get(0).cloned()), + Mode::List => list(pids_or_signals.get(0)), } } @@ -168,9 +168,9 @@ fn print_signals() { println!(); } -fn list(arg: Option) -> UResult<()> { +fn list(arg: Option<&String>) -> UResult<()> { match arg { - Some(ref x) => print_signal(x), + Some(x) => print_signal(x), None => { print_signals(); Ok(()) diff --git a/src/uu/split/src/number.rs b/src/uu/split/src/number.rs index b2c402716..ef3ccbc4b 100644 --- a/src/uu/split/src/number.rs +++ b/src/uu/split/src/number.rs @@ -96,8 +96,8 @@ impl Number { #[allow(dead_code)] fn digits(&self) -> &Vec { match self { - Number::FixedWidth(number) => &number.digits, - Number::DynamicWidth(number) => &number.digits, + Self::FixedWidth(number) => &number.digits, + Self::DynamicWidth(number) => &number.digits, } } @@ -136,8 +136,8 @@ impl Number { /// ``` pub fn increment(&mut self) -> Result<(), Overflow> { match self { - Number::FixedWidth(number) => number.increment(), - Number::DynamicWidth(number) => number.increment(), + Self::FixedWidth(number) => number.increment(), + Self::DynamicWidth(number) => number.increment(), } } } @@ -145,8 +145,8 @@ impl Number { impl Display for Number { fn fmt(&self, f: &mut Formatter) -> fmt::Result { match self { - Number::FixedWidth(number) => number.fmt(f), - Number::DynamicWidth(number) => number.fmt(f), + Self::FixedWidth(number) => number.fmt(f), + Self::DynamicWidth(number) => number.fmt(f), } } } @@ -183,8 +183,8 @@ pub struct FixedWidthNumber { impl FixedWidthNumber { /// Instantiate a number of the given radix and width. - pub fn new(radix: u8, width: usize) -> FixedWidthNumber { - FixedWidthNumber { + pub fn new(radix: u8, width: usize) -> Self { + Self { radix, digits: vec![0; width], } @@ -286,8 +286,8 @@ impl DynamicWidthNumber { /// /// This associated function returns a new instance of the struct /// with the given radix and a width of two digits, both 0. - pub fn new(radix: u8) -> DynamicWidthNumber { - DynamicWidthNumber { + pub fn new(radix: u8) -> Self { + Self { radix, digits: vec![0, 0], } @@ -404,7 +404,7 @@ mod tests { fn num(n: usize) -> Number { let mut number = Number::DynamicWidth(DynamicWidthNumber::new(26)); for _ in 0..n { - number.increment().unwrap() + number.increment().unwrap(); } number } @@ -428,7 +428,7 @@ mod tests { fn num(n: usize) -> Number { let mut number = Number::DynamicWidth(DynamicWidthNumber::new(10)); for _ in 0..n { - number.increment().unwrap() + number.increment().unwrap(); } number } diff --git a/src/uu/split/src/split.rs b/src/uu/split/src/split.rs index 23eb24768..a05959810 100644 --- a/src/uu/split/src/split.rs +++ b/src/uu/split/src/split.rs @@ -62,7 +62,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .override_usage(&usage[..]) .after_help(&long_usage[..]) .get_matches_from(args); - let settings = Settings::from(matches)?; + let settings = Settings::from(&matches)?; split(&settings) } @@ -232,7 +232,7 @@ struct Settings { impl Settings { /// Parse a strategy from the command-line arguments. - fn from(matches: ArgMatches) -> UResult { + fn from(matches: &ArgMatches) -> UResult { let result = Self { suffix_length: matches .value_of(OPT_SUFFIX_LENGTH) @@ -242,7 +242,7 @@ impl Settings { numeric_suffix: matches.occurrences_of(OPT_NUMERIC_SUFFIXES) > 0, additional_suffix: matches.value_of(OPT_ADDITIONAL_SUFFIX).unwrap().to_owned(), verbose: matches.occurrences_of("verbose") > 0, - strategy: Strategy::from(&matches)?, + strategy: Strategy::from(matches)?, input: matches.value_of(ARG_INPUT).unwrap().to_owned(), prefix: matches.value_of(ARG_PREFIX).unwrap().to_owned(), filter: matches.value_of(OPT_FILTER).map(|s| s.to_owned()), diff --git a/src/uu/tail/src/platform/unix.rs b/src/uu/tail/src/platform/unix.rs index e7f75c31e..e01d5e444 100644 --- a/src/uu/tail/src/platform/unix.rs +++ b/src/uu/tail/src/platform/unix.rs @@ -30,6 +30,7 @@ impl ProcessChecker { } // Borrowing mutably to be aligned with Windows implementation + #[allow(clippy::wrong_self_convention)] pub fn is_dead(&mut self) -> bool { unsafe { libc::kill(self.pid, 0) != 0 && get_errno() != libc::EPERM } } diff --git a/src/uu/tail/src/platform/windows.rs b/src/uu/tail/src/platform/windows.rs index 7faa872e6..c63040a2a 100644 --- a/src/uu/tail/src/platform/windows.rs +++ b/src/uu/tail/src/platform/windows.rs @@ -24,11 +24,11 @@ pub struct ProcessChecker { } impl ProcessChecker { - pub fn new(process_id: self::Pid) -> ProcessChecker { + pub fn new(process_id: self::Pid) -> Self { #[allow(non_snake_case)] let FALSE = 0i32; let h = unsafe { OpenProcess(SYNCHRONIZE, FALSE, process_id as DWORD) }; - ProcessChecker { + Self { dead: h.is_null(), handle: h, } diff --git a/src/uucore/src/lib/features/fsext.rs b/src/uucore/src/lib/features/fsext.rs index d1e623757..a3b05dff8 100644 --- a/src/uucore/src/lib/features/fsext.rs +++ b/src/uucore/src/lib/features/fsext.rs @@ -238,7 +238,7 @@ impl MountInfo { } } #[cfg(windows)] - fn new(mut volume_name: String) -> Option { + fn new(mut volume_name: String) -> Option { let mut dev_name_buf = [0u16; MAX_PATH]; volume_name.pop(); unsafe { @@ -289,7 +289,7 @@ impl MountInfo { } else { None }; - let mut mn_info = MountInfo { + let mut mn_info = Self { dev_id: volume_name, dev_name, fs_type: fs_type.unwrap_or_else(|| "".to_string()), @@ -319,7 +319,7 @@ use std::ffi::CStr; ))] impl From for MountInfo { fn from(statfs: StatFs) -> Self { - let mut info = MountInfo { + let mut info = Self { dev_id: "".to_string(), dev_name: unsafe { // spell-checker:disable-next-line @@ -553,7 +553,7 @@ impl FsUsage { } let bytes_per_cluster = sectors_per_cluster as u64 * bytes_per_sector as u64; - FsUsage { + Self { // f_bsize File system block size. blocksize: bytes_per_cluster as u64, // f_blocks - Total number of blocks on the file system, in units of f_frsize. diff --git a/src/uucore/src/lib/features/wide.rs b/src/uucore/src/lib/features/wide.rs index 49ce575a7..6b9368f50 100644 --- a/src/uucore/src/lib/features/wide.rs +++ b/src/uucore/src/lib/features/wide.rs @@ -27,10 +27,10 @@ pub trait FromWide { fn from_wide_null(wide: &[u16]) -> Self; } impl FromWide for String { - fn from_wide(wide: &[u16]) -> String { + fn from_wide(wide: &[u16]) -> Self { OsString::from_wide(wide).to_string_lossy().into_owned() } - fn from_wide_null(wide: &[u16]) -> String { + fn from_wide_null(wide: &[u16]) -> Self { let len = wide.iter().take_while(|&&c| c != 0).count(); OsString::from_wide(&wide[..len]) .to_string_lossy() diff --git a/tests/by-util/test_du.rs b/tests/by-util/test_du.rs index efc097773..b0506d071 100644 --- a/tests/by-util/test_du.rs +++ b/tests/by-util/test_du.rs @@ -179,15 +179,15 @@ fn test_du_hard_link() { #[cfg(target_vendor = "apple")] fn _du_hard_link(s: &str) { - assert_eq!(s, "12\tsubdir/links\n") + assert_eq!(s, "12\tsubdir/links\n"); } #[cfg(target_os = "windows")] fn _du_hard_link(s: &str) { - assert_eq!(s, "8\tsubdir/links\n") + assert_eq!(s, "8\tsubdir/links\n"); } #[cfg(target_os = "freebsd")] fn _du_hard_link(s: &str) { - assert_eq!(s, "16\tsubdir/links\n") + assert_eq!(s, "16\tsubdir/links\n"); } #[cfg(all( not(target_vendor = "apple"), diff --git a/tests/by-util/test_env.rs b/tests/by-util/test_env.rs index e1950f0df..3559e74cd 100644 --- a/tests/by-util/test_env.rs +++ b/tests/by-util/test_env.rs @@ -219,7 +219,7 @@ fn test_change_directory() { .args(&pwd) .succeeds() .stdout_move_str(); - assert_eq!(out.trim(), temporary_path.as_os_str()) + assert_eq!(out.trim(), temporary_path.as_os_str()); } #[test] From cd1b5c5748ab2613faa5c63c59ed415a9739c27d Mon Sep 17 00:00:00 2001 From: Sam Caldwell Date: Mon, 31 Jan 2022 22:08:59 -0700 Subject: [PATCH 02/14] [truncate] change cli error return code Exit with status code 1 for argument parsing errors in `truncate`. When `clap` encounters an error during argument parsing, it exits with status code 2. This causes some GNU tests to fail since they expect status code 1. --- src/uu/truncate/src/truncate.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/uu/truncate/src/truncate.rs b/src/uu/truncate/src/truncate.rs index 685363f8f..142524aad 100644 --- a/src/uu/truncate/src/truncate.rs +++ b/src/uu/truncate/src/truncate.rs @@ -6,6 +6,7 @@ // * file that was distributed with this source code. // spell-checker:ignore (ToDO) RFILE refsize rfilename fsize tsize +use clap; use clap::{crate_version, App, AppSettings, Arg}; use std::convert::TryFrom; use std::fs::{metadata, OpenOptions}; @@ -115,7 +116,16 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let matches = uu_app() .override_usage(&usage[..]) .after_help(&long_usage[..]) - .get_matches_from(args); + .try_get_matches_from(args) + .unwrap_or_else(|e| { + e.print(); + match e.kind { + clap::ErrorKind::DisplayHelp | clap::ErrorKind::DisplayVersion => { + std::process::exit(0) + } + _ => std::process::exit(1), + } + }); let files: Vec = matches .values_of(options::ARG_FILES) From 6f24166c63a3aa001f9c06df8a93fc2f0ab211b7 Mon Sep 17 00:00:00 2001 From: Sam Caldwell Date: Mon, 31 Jan 2022 22:25:59 -0700 Subject: [PATCH 03/14] [truncate] handle unused_must_use warning --- src/uu/truncate/src/truncate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/truncate/src/truncate.rs b/src/uu/truncate/src/truncate.rs index 142524aad..4850e592a 100644 --- a/src/uu/truncate/src/truncate.rs +++ b/src/uu/truncate/src/truncate.rs @@ -118,7 +118,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .after_help(&long_usage[..]) .try_get_matches_from(args) .unwrap_or_else(|e| { - e.print(); + e.print().expect("Error writing clap::Error"); match e.kind { clap::ErrorKind::DisplayHelp | clap::ErrorKind::DisplayVersion => { std::process::exit(0) From e1f7c774d811c4c3da6cd548bdc25c47a3183580 Mon Sep 17 00:00:00 2001 From: Sam Caldwell Date: Mon, 31 Jan 2022 22:59:10 -0700 Subject: [PATCH 04/14] Remove redundant import --- src/uu/truncate/src/truncate.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/uu/truncate/src/truncate.rs b/src/uu/truncate/src/truncate.rs index 4850e592a..12f413247 100644 --- a/src/uu/truncate/src/truncate.rs +++ b/src/uu/truncate/src/truncate.rs @@ -6,7 +6,6 @@ // * file that was distributed with this source code. // spell-checker:ignore (ToDO) RFILE refsize rfilename fsize tsize -use clap; use clap::{crate_version, App, AppSettings, Arg}; use std::convert::TryFrom; use std::fs::{metadata, OpenOptions}; From b29e219e4dc7fe162c83d0e0dec301da4fd9526c Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Tue, 1 Feb 2022 00:35:26 +0100 Subject: [PATCH 05/14] true: Rework to return true more often Now treats recognized command line options and ignores unrecognized command line options instead of returning a special exit status for them. There is one point of interest, which is related to an implementation detail in GNU `true`. It may return a non-true exit status (in particular EXIT_FAIL) if writing the diagnostics of a GNU specific option fails. For example `true --version > /dev/full` would fail and have exit status 1. This behavior was acknowledged in gnu in commit <9a6a486e6503520fd2581f2d3356b7149f1b225d>. No further justification provided for keeping this quirk. POSIX knows no such options, and requires an exit status of 0 in all cases. We replicate GNU here which is a consistency improvement over the prior implementation. Adds documentation to clarify the intended behavior more properly. --- src/uu/true/src/true.rs | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/src/uu/true/src/true.rs b/src/uu/true/src/true.rs index ff5b08e85..21b064e73 100644 --- a/src/uu/true/src/true.rs +++ b/src/uu/true/src/true.rs @@ -4,16 +4,41 @@ // * // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. +use clap::{App, AppSettings, ErrorKind}; +use std::io::Write; +use uucore::error::{set_exit_code, UResult}; -use clap::{App, AppSettings}; -use uucore::error::UResult; +static ABOUT: &str = " + Returns true, a successful exit status. + + Immediately returns with the exit status `0`, except when invoked with one of the recognized + options. In those cases it will try to write the help or version text. Any IO error during this + operation causes the program to return `1` instead. +"; #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - uu_app().get_matches_from(args); + let app = uu_app(); + + if let Err(err) = app.try_get_matches_from(args) { + if let ErrorKind::DisplayHelp | ErrorKind::DisplayVersion = err.kind { + if let Err(print_fail) = err.print() { + // Try to display this error. + let _ = writeln!(std::io::stderr(), "{}: {}", uucore::util_name(), print_fail); + // Mirror GNU options. When failing to print warnings or version flags, then we exit + // with FAIL. This avoids allocation some error information which may result in yet + // other types of failure. + set_exit_code(1); + } + } + } + Ok(()) } pub fn uu_app<'a>() -> App<'a> { - App::new(uucore::util_name()).setting(AppSettings::InferLongArgs) + App::new(uucore::util_name()) + .version(clap::crate_version!()) + .about(ABOUT) + .setting(AppSettings::InferLongArgs) } From dcf177f9083b2f676b85049846f78a1ce642d602 Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Tue, 1 Feb 2022 01:43:59 +0100 Subject: [PATCH 06/14] false: Align behavior to true and GNU --- src/uu/false/src/false.rs | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/src/uu/false/src/false.rs b/src/uu/false/src/false.rs index 324f90579..b304b4af6 100644 --- a/src/uu/false/src/false.rs +++ b/src/uu/false/src/false.rs @@ -4,16 +4,43 @@ // * // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. +use clap::{App, AppSettings, ErrorKind}; +use std::io::Write; +use uucore::error::{set_exit_code, UResult}; -use clap::App; -use uucore::error::UResult; +static ABOUT: &str = " + Returns false, an unsuccessful exit status. + + Immediately returns with the exit status `1`. When invoked with one of the recognized options it + will try to write the help or version text. Any IO error during this operation is diagnosed, yet + the program will also return `1`. +"; #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - uu_app().get_matches_from(args); - Err(1.into()) + let app = uu_app(); + + // Mirror GNU options, always return `1`. In particular even the 'successful' cases of no-op, + // and the interrupted display of help and version should return `1`. Also, we return Ok in all + // paths to avoid the allocation of an error object, an operation that could, in theory, fail + // and unwind through the standard library allocation handling machinery. + set_exit_code(1); + + if let Err(err) = app.try_get_matches_from(args) { + if let ErrorKind::DisplayHelp | ErrorKind::DisplayVersion = err.kind { + if let Err(print_fail) = err.print() { + // Try to display this error. + let _ = writeln!(std::io::stderr(), "{}: {}", uucore::util_name(), print_fail); + } + } + } + + Ok(()) } pub fn uu_app<'a>() -> App<'a> { App::new(uucore::util_name()) + .version(clap::crate_version!()) + .about(ABOUT) + .setting(AppSettings::InferLongArgs) } From 149399c1bf54f7c0df5558578e28bd82ad219581 Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Tue, 1 Feb 2022 12:19:00 +0100 Subject: [PATCH 07/14] false,true: Add by-util tests for options --- tests/by-util/test_false.rs | 39 +++++++++++++++++++++++++++++++++++++ tests/by-util/test_true.rs | 39 +++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/tests/by-util/test_false.rs b/tests/by-util/test_false.rs index bbabc7a52..366dd277b 100644 --- a/tests/by-util/test_false.rs +++ b/tests/by-util/test_false.rs @@ -1,6 +1,45 @@ use crate::common::util::*; +#[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "netbsd"))] +use std::fs::OpenOptions; #[test] fn test_exit_code() { new_ucmd!().fails(); } + +#[test] +fn test_version() { + new_ucmd!() + .args(&["--version"]) + .fails() + .stdout_contains("false"); +} + +#[test] +fn test_help() { + new_ucmd!() + .args(&["--help"]) + .fails() + .stdout_contains("false"); +} + +#[test] +fn test_short_options() { + for option in ["-h", "-v"] { + new_ucmd!().arg(option).fails().stdout_is(""); + } +} + +#[test] +#[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "netbsd"))] +fn test_full() { + for option in ["--version", "--help"] { + let dev_full = OpenOptions::new().write(true).open("/dev/full").unwrap(); + + new_ucmd!() + .arg(option) + .set_stdout(dev_full) + .fails() + .stderr_contains("No space left on device"); + } +} diff --git a/tests/by-util/test_true.rs b/tests/by-util/test_true.rs index 1d8622c96..d8ac2003b 100644 --- a/tests/by-util/test_true.rs +++ b/tests/by-util/test_true.rs @@ -1,6 +1,45 @@ use crate::common::util::*; +#[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "netbsd"))] +use std::fs::OpenOptions; #[test] fn test_exit_code() { new_ucmd!().succeeds(); } + +#[test] +fn test_version() { + new_ucmd!() + .args(&["--version"]) + .succeeds() + .stdout_contains("true"); +} + +#[test] +fn test_help() { + new_ucmd!() + .args(&["--help"]) + .succeeds() + .stdout_contains("true"); +} + +#[test] +fn test_short_options() { + for option in ["-h", "-v"] { + new_ucmd!().arg(option).succeeds().stdout_is(""); + } +} + +#[test] +#[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "netbsd"))] +fn test_full() { + for option in ["--version", "--help"] { + let dev_full = OpenOptions::new().write(true).open("/dev/full").unwrap(); + + new_ucmd!() + .arg(option) + .set_stdout(dev_full) + .fails() + .stderr_contains("No space left on device"); + } +} From c1e108933fe66853e0d081f13b657c3acee75cd6 Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Tue, 1 Feb 2022 12:32:30 +0100 Subject: [PATCH 08/14] false,true: Align behavior of short flags to GNU --- src/uu/false/src/false.rs | 15 +++++++++++++-- src/uu/true/src/true.rs | 15 +++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/uu/false/src/false.rs b/src/uu/false/src/false.rs index b304b4af6..0ebbb5be2 100644 --- a/src/uu/false/src/false.rs +++ b/src/uu/false/src/false.rs @@ -4,7 +4,7 @@ // * // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -use clap::{App, AppSettings, ErrorKind}; +use clap::{App, Arg, ArgSettings, ErrorKind}; use std::io::Write; use uucore::error::{set_exit_code, UResult}; @@ -42,5 +42,16 @@ pub fn uu_app<'a>() -> App<'a> { App::new(uucore::util_name()) .version(clap::crate_version!()) .about(ABOUT) - .setting(AppSettings::InferLongArgs) + // Hide the default -V and -h for version and help. + // This requires us to overwrite short, not short_aliases. + .arg( + Arg::new("dummy-help") + .short('h') + .setting(ArgSettings::Hidden), + ) + .arg( + Arg::new("dummy-version") + .short('V') + .setting(ArgSettings::Hidden), + ) } diff --git a/src/uu/true/src/true.rs b/src/uu/true/src/true.rs index 21b064e73..20ee100b7 100644 --- a/src/uu/true/src/true.rs +++ b/src/uu/true/src/true.rs @@ -4,7 +4,7 @@ // * // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -use clap::{App, AppSettings, ErrorKind}; +use clap::{App, Arg, ArgSettings, ErrorKind}; use std::io::Write; use uucore::error::{set_exit_code, UResult}; @@ -40,5 +40,16 @@ pub fn uu_app<'a>() -> App<'a> { App::new(uucore::util_name()) .version(clap::crate_version!()) .about(ABOUT) - .setting(AppSettings::InferLongArgs) + // Hide the default -V and -h for version and help. + // This requires us to overwrite short, not short_aliases. + .arg( + Arg::new("dummy-help") + .short('h') + .setting(ArgSettings::Hidden), + ) + .arg( + Arg::new("dummy-version") + .short('V') + .setting(ArgSettings::Hidden), + ) } From 23a544c485672743b9fa119e0c6614ea12706da3 Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Tue, 1 Feb 2022 14:29:26 +0100 Subject: [PATCH 09/14] false,true: Implement custom help, version This avoids hacking around the short options of these command line arguments that have been introduced by clap. Additionally, we test and correctly handle the combination of both version and help. The GNU binary will ignore both arguments in this case while clap would perform the first one. A test for this edge case was added. --- src/uu/false/src/false.rs | 40 ++++++++++++++++++------------- src/uu/true/src/true.rs | 47 +++++++++++++++++++++---------------- tests/by-util/test_false.rs | 10 +++++++- tests/by-util/test_true.rs | 10 +++++++- 4 files changed, 69 insertions(+), 38 deletions(-) diff --git a/src/uu/false/src/false.rs b/src/uu/false/src/false.rs index 0ebbb5be2..8b487847d 100644 --- a/src/uu/false/src/false.rs +++ b/src/uu/false/src/false.rs @@ -4,7 +4,7 @@ // * // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -use clap::{App, Arg, ArgSettings, ErrorKind}; +use clap::{App, AppSettings, Arg}; use std::io::Write; use uucore::error::{set_exit_code, UResult}; @@ -18,7 +18,7 @@ static ABOUT: &str = " #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - let app = uu_app(); + let mut app = uu_app(); // Mirror GNU options, always return `1`. In particular even the 'successful' cases of no-op, // and the interrupted display of help and version should return `1`. Also, we return Ok in all @@ -26,12 +26,19 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // and unwind through the standard library allocation handling machinery. set_exit_code(1); - if let Err(err) = app.try_get_matches_from(args) { - if let ErrorKind::DisplayHelp | ErrorKind::DisplayVersion = err.kind { - if let Err(print_fail) = err.print() { - // Try to display this error. - let _ = writeln!(std::io::stderr(), "{}: {}", uucore::util_name(), print_fail); - } + if let Ok(matches) = app.try_get_matches_from_mut(args) { + let error = if matches.index_of("help").is_some() { + app.print_long_help() + } else if matches.index_of("version").is_some() { + writeln!(std::io::stdout(), "{}", app.render_version()) + } else { + Ok(()) + }; + + // Try to display this error. + if let Err(print_fail) = error { + // Completely ignore any error here, no more failover and we will fail in any case. + let _ = writeln!(std::io::stderr(), "{}: {}", uucore::util_name(), print_fail); } } @@ -42,16 +49,17 @@ pub fn uu_app<'a>() -> App<'a> { App::new(uucore::util_name()) .version(clap::crate_version!()) .about(ABOUT) - // Hide the default -V and -h for version and help. - // This requires us to overwrite short, not short_aliases. + // We provide our own help and version options, to ensure maximum compatibility with GNU. + .setting(AppSettings::DisableHelpFlag | AppSettings::DisableVersionFlag) .arg( - Arg::new("dummy-help") - .short('h') - .setting(ArgSettings::Hidden), + Arg::new("help") + .long("help") + .help("Print help information") + .exclusive(true), ) .arg( - Arg::new("dummy-version") - .short('V') - .setting(ArgSettings::Hidden), + Arg::new("version") + .long("version") + .help("Print version information"), ) } diff --git a/src/uu/true/src/true.rs b/src/uu/true/src/true.rs index 20ee100b7..c3026e684 100644 --- a/src/uu/true/src/true.rs +++ b/src/uu/true/src/true.rs @@ -4,7 +4,7 @@ // * // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -use clap::{App, Arg, ArgSettings, ErrorKind}; +use clap::{App, AppSettings, Arg}; use std::io::Write; use uucore::error::{set_exit_code, UResult}; @@ -18,18 +18,24 @@ static ABOUT: &str = " #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - let app = uu_app(); + let mut app = uu_app(); - if let Err(err) = app.try_get_matches_from(args) { - if let ErrorKind::DisplayHelp | ErrorKind::DisplayVersion = err.kind { - if let Err(print_fail) = err.print() { - // Try to display this error. - let _ = writeln!(std::io::stderr(), "{}: {}", uucore::util_name(), print_fail); - // Mirror GNU options. When failing to print warnings or version flags, then we exit - // with FAIL. This avoids allocation some error information which may result in yet - // other types of failure. - set_exit_code(1); - } + if let Ok(matches) = app.try_get_matches_from_mut(args) { + let error = if matches.index_of("help").is_some() { + app.print_long_help() + } else if matches.index_of("version").is_some() { + writeln!(std::io::stdout(), "{}", app.render_version()) + } else { + Ok(()) + }; + + if let Err(print_fail) = error { + // Try to display this error. + let _ = writeln!(std::io::stderr(), "{}: {}", uucore::util_name(), print_fail); + // Mirror GNU options. When failing to print warnings or version flags, then we exit + // with FAIL. This avoids allocation some error information which may result in yet + // other types of failure. + set_exit_code(1); } } @@ -40,16 +46,17 @@ pub fn uu_app<'a>() -> App<'a> { App::new(uucore::util_name()) .version(clap::crate_version!()) .about(ABOUT) - // Hide the default -V and -h for version and help. - // This requires us to overwrite short, not short_aliases. + // We provide our own help and version options, to ensure maximum compatibility with GNU. + .setting(AppSettings::DisableHelpFlag | AppSettings::DisableVersionFlag) .arg( - Arg::new("dummy-help") - .short('h') - .setting(ArgSettings::Hidden), + Arg::new("help") + .long("help") + .help("Print help information") + .exclusive(true), ) .arg( - Arg::new("dummy-version") - .short('V') - .setting(ArgSettings::Hidden), + Arg::new("version") + .long("version") + .help("Print version information"), ) } diff --git a/tests/by-util/test_false.rs b/tests/by-util/test_false.rs index 366dd277b..5ce64e7a8 100644 --- a/tests/by-util/test_false.rs +++ b/tests/by-util/test_false.rs @@ -25,11 +25,19 @@ fn test_help() { #[test] fn test_short_options() { - for option in ["-h", "-v"] { + for option in ["-h", "-V"] { new_ucmd!().arg(option).fails().stdout_is(""); } } +#[test] +fn test_conflict() { + new_ucmd!() + .args(&["--help", "--version"]) + .fails() + .stdout_is(""); +} + #[test] #[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "netbsd"))] fn test_full() { diff --git a/tests/by-util/test_true.rs b/tests/by-util/test_true.rs index d8ac2003b..aba32578b 100644 --- a/tests/by-util/test_true.rs +++ b/tests/by-util/test_true.rs @@ -25,11 +25,19 @@ fn test_help() { #[test] fn test_short_options() { - for option in ["-h", "-v"] { + for option in ["-h", "-V"] { new_ucmd!().arg(option).succeeds().stdout_is(""); } } +#[test] +fn test_conflict() { + new_ucmd!() + .args(&["--help", "--version"]) + .succeeds() + .stdout_is(""); +} + #[test] #[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "netbsd"))] fn test_full() { From c6d5eccf6c85420dd99fa2bca7501b292f0416f0 Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Tue, 1 Feb 2022 19:53:25 +0100 Subject: [PATCH 10/14] false,true: Resolve formatting nit in About --- src/uu/false/src/false.rs | 10 +++++----- src/uu/true/src/true.rs | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/uu/false/src/false.rs b/src/uu/false/src/false.rs index 8b487847d..c6661dc35 100644 --- a/src/uu/false/src/false.rs +++ b/src/uu/false/src/false.rs @@ -8,12 +8,12 @@ use clap::{App, AppSettings, Arg}; use std::io::Write; use uucore::error::{set_exit_code, UResult}; -static ABOUT: &str = " - Returns false, an unsuccessful exit status. +static ABOUT: &str = "\ +Returns false, an unsuccessful exit status. - Immediately returns with the exit status `1`. When invoked with one of the recognized options it - will try to write the help or version text. Any IO error during this operation is diagnosed, yet - the program will also return `1`. +Immediately returns with the exit status `1`. When invoked with one of the recognized options it +will try to write the help or version text. Any IO error during this operation is diagnosed, yet +the program will also return `1`. "; #[uucore::main] diff --git a/src/uu/true/src/true.rs b/src/uu/true/src/true.rs index c3026e684..4a8452db6 100644 --- a/src/uu/true/src/true.rs +++ b/src/uu/true/src/true.rs @@ -8,12 +8,12 @@ use clap::{App, AppSettings, Arg}; use std::io::Write; use uucore::error::{set_exit_code, UResult}; -static ABOUT: &str = " - Returns true, a successful exit status. +static ABOUT: &str = "\ +Returns true, a successful exit status. - Immediately returns with the exit status `0`, except when invoked with one of the recognized - options. In those cases it will try to write the help or version text. Any IO error during this - operation causes the program to return `1` instead. +Immediately returns with the exit status `0`, except when invoked with one of the recognized +options. In those cases it will try to write the help or version text. Any IO error during this +operation causes the program to return `1` instead. "; #[uucore::main] From ea5541db5692022df684084bf6b4be2c58da6352 Mon Sep 17 00:00:00 2001 From: Sam Caldwell Date: Tue, 1 Feb 2022 08:29:22 -0700 Subject: [PATCH 11/14] truncate: add test_invalid_option --- tests/by-util/test_truncate.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/tests/by-util/test_truncate.rs b/tests/by-util/test_truncate.rs index 0ef65ec16..c1e44f605 100644 --- a/tests/by-util/test_truncate.rs +++ b/tests/by-util/test_truncate.rs @@ -250,11 +250,24 @@ fn test_size_and_reference() { #[test] fn test_error_filename_only() { // truncate: you must specify either '--size' or '--reference' - new_ucmd!().args(&["file"]).fails().stderr_contains( - "error: The following required arguments were not provided: + new_ucmd!() + .args(&["file"]) + .fails() + .code_is(1) + .stderr_contains( + "error: The following required arguments were not provided: --reference --size ", - ); + ); +} + +#[test] +fn test_invalid_option() { + // truncate: cli parsing error returns 1 + new_ucmd!() + .args(&["--this-arg-does-not-exist"]) + .fails() + .code_is(1); } #[test] From 39f8329222acf64ff9d5a3193cd8608b46c1f4f4 Mon Sep 17 00:00:00 2001 From: Sam Caldwell Date: Tue, 1 Feb 2022 14:13:52 -0700 Subject: [PATCH 12/14] truncate: use `map_err` instead of `unwrap_or_else` --- src/uu/truncate/src/truncate.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/uu/truncate/src/truncate.rs b/src/uu/truncate/src/truncate.rs index 12f413247..242dd416a 100644 --- a/src/uu/truncate/src/truncate.rs +++ b/src/uu/truncate/src/truncate.rs @@ -116,15 +116,13 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .override_usage(&usage[..]) .after_help(&long_usage[..]) .try_get_matches_from(args) - .unwrap_or_else(|e| { + .map_err(|e| { e.print().expect("Error writing clap::Error"); match e.kind { - clap::ErrorKind::DisplayHelp | clap::ErrorKind::DisplayVersion => { - std::process::exit(0) - } - _ => std::process::exit(1), + clap::ErrorKind::DisplayHelp | clap::ErrorKind::DisplayVersion => 0, + _ => 1, } - }); + })?; let files: Vec = matches .values_of(options::ARG_FILES) From 7e32b6ba17a482d970e78966abffed7a4a2d4ee2 Mon Sep 17 00:00:00 2001 From: Rahul Kadukar Date: Tue, 1 Feb 2022 23:51:48 -0500 Subject: [PATCH 13/14] Added description for hostid --- src/uu/hostid/src/hostid.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/uu/hostid/src/hostid.rs b/src/uu/hostid/src/hostid.rs index 764b7d279..99b800f14 100644 --- a/src/uu/hostid/src/hostid.rs +++ b/src/uu/hostid/src/hostid.rs @@ -12,6 +12,7 @@ use libc::c_long; use uucore::error::UResult; static SYNTAX: &str = "[options]"; +const SUMMARY: &str = "Print the numeric identifier (in hexadecimal) for the current host"; // currently rust libc interface doesn't include gethostid extern "C" { @@ -28,6 +29,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { pub fn uu_app<'a>() -> App<'a> { App::new(uucore::util_name()) .version(crate_version!()) + .about(SUMMARY) .override_usage(SYNTAX) .setting(AppSettings::InferLongArgs) } From 773ceb5534a0c85ff2e7ca86222b4b037767bad9 Mon Sep 17 00:00:00 2001 From: DevSaab Date: Wed, 2 Feb 2022 10:08:48 -0500 Subject: [PATCH 14/14] Include ABOUT for shuf --- src/uu/shuf/src/shuf.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/uu/shuf/src/shuf.rs b/src/uu/shuf/src/shuf.rs index a7dcd48e9..da2dfff1b 100644 --- a/src/uu/shuf/src/shuf.rs +++ b/src/uu/shuf/src/shuf.rs @@ -31,6 +31,7 @@ Write a random permutation of the input lines to standard output. With no FILE, or when FILE is -, read standard input. "#; +static ABOUT: &str = "Shuffle the input by outputting a random permutation of input lines. Each output permutation is equally likely."; static TEMPLATE: &str = "Usage: {usage}\nMandatory arguments to long options are mandatory for short options too.\n{options}"; struct Options { @@ -121,6 +122,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { pub fn uu_app<'a>() -> App<'a> { App::new(uucore::util_name()) .name(NAME) + .about(ABOUT) .version(crate_version!()) .help_template(TEMPLATE) .override_usage(USAGE)