From b29e219e4dc7fe162c83d0e0dec301da4fd9526c Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Tue, 1 Feb 2022 00:35:26 +0100 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 5/6] 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 6/6] 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]