Merge pull request #10141 from RenjiSann/revert-checksum-clap

Revert recent checksum CLI changes

Author: sylvestre

src/uu/cksum/src/cksum.rs

@@ -7,7 +7,7 @@
 
 use clap::builder::ValueParser;
 use clap::{Arg, ArgAction, Command};
-use std::ffi::OsString;
+use std::ffi::{OsStr, OsString};
 use uucore::checksum::compute::{
     ChecksumComputeOptions, figure_out_output_format, perform_checksum_computation,
 };
@@ -73,6 +73,42 @@ mod options {
 /// Returns a pair of boolean. The first one indicates if we should use tagged
 /// output format, the second one indicates if we should use the binary flag in
 /// the untagged case.
+fn handle_tag_text_binary_flags<S: AsRef<OsStr>>(
+    args: impl Iterator<Item = S>,
+) -> UResult<(bool, bool)> {
+    let mut tag = true;
+    let mut binary = false;
+    let mut text = false;
+
+    // --binary, --tag and --untagged are tight together: none of them
+    // conflicts with each other but --tag will reset "binary" and "text" and
+    // set "tag".
+
+    for arg in args {
+        let arg = arg.as_ref();
+        if arg == "-b" || arg == "--binary" {
+            text = false;
+            binary = true;
+        } else if arg == "--text" {
+            text = true;
+            binary = false;
+        } else if arg == "--tag" {
+            tag = true;
+            binary = false;
+            text = false;
+        } else if arg == "--untagged" {
+            tag = false;
+        }
+    }
+
+    // Specifying --text without ever mentioning --untagged fails.
+    if text && tag {
+        return Err(ChecksumError::TextWithoutUntagged.into());
+    }
+
+    Ok((tag, binary))
+}
+
 /// Sanitize the `--length` argument depending on `--algorithm` and `--length`.
 fn maybe_sanitize_length(
     algo_cli: Option<AlgoKind>,
@@ -103,11 +139,19 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
 
     let check = matches.get_flag(options::CHECK);
 
-    let ignore_missing = matches.get_flag(options::IGNORE_MISSING);
-    let warn = matches.get_flag(options::WARN);
-    let quiet = matches.get_flag(options::QUIET);
-    let strict = matches.get_flag(options::STRICT);
-    let status = matches.get_flag(options::STATUS);
+    let check_flag = |flag| match (check, matches.get_flag(flag)) {
+        (_, false) => Ok(false),
+        (true, true) => Ok(true),
+        (false, true) => Err(ChecksumError::CheckOnlyFlag(flag.into())),
+    };
+
+    // Each of the following flags are only expected in --check mode.
+    // If we encounter them otherwise, end with an error.
+    let ignore_missing = check_flag(options::IGNORE_MISSING)?;
+    let warn = check_flag(options::WARN)?;
+    let quiet = check_flag(options::QUIET)?;
+    let strict = check_flag(options::STRICT)?;
+    let status = check_flag(options::STATUS)?;
 
     let algo_cli = matches
         .get_one::<String>(options::ALGORITHM)
@@ -132,6 +176,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
             return Err(ChecksumError::AlgorithmNotSupportedWithCheck.into());
         }
 
+        let text_flag = matches.get_flag(options::TEXT);
+        let binary_flag = matches.get_flag(options::BINARY);
+        let tag = matches.get_flag(options::TAG);
+
+        if tag || binary_flag || text_flag {
+            return Err(ChecksumError::BinaryTextConflict.into());
+        }
+
         // Execute the checksum validation based on the presence of files or the use of stdin
 
         let verbose = ChecksumVerbose::new(status, quiet, warn);
@@ -154,8 +206,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     // Set the default algorithm to CRC when not '--check'ing.
     let algo_kind = algo_cli.unwrap_or(AlgoKind::Crc);
 
-    let tag = !matches.get_flag(options::UNTAGGED); // Making TAG default at clap blocks --untagged 
-    let binary = matches.get_flag(options::BINARY);
+    let (tag, binary) = handle_tag_text_binary_flags(std::env::args_os())?;
 
     let algo = SizedAlgoKind::from_unsized(algo_kind, length)?;
     let line_ending = LineEnding::from_zero_flag(matches.get_flag(options::ZERO));
@@ -214,9 +265,7 @@ pub fn uu_app() -> Command {
                 .long(options::TAG)
                 .help(translate!("cksum-help-tag"))
                 .action(ArgAction::SetTrue)
-                .overrides_with(options::UNTAGGED)
-                .overrides_with(options::BINARY)
-                .overrides_with(options::TEXT),
+                .overrides_with(options::UNTAGGED),
         )
         .arg(
             Arg::new(options::LENGTH)
@@ -235,17 +284,13 @@ pub fn uu_app() -> Command {
             Arg::new(options::STRICT)
                 .long(options::STRICT)
                 .help(translate!("cksum-help-strict"))
-                .action(ArgAction::SetTrue)
-                .requires(options::CHECK),
+                .action(ArgAction::SetTrue),
         )
         .arg(
             Arg::new(options::CHECK)
                 .short('c')
                 .long(options::CHECK)
                 .help(translate!("cksum-help-check"))
-                .conflicts_with(options::TAG)
-                .conflicts_with(options::BINARY)
-                .conflicts_with(options::TEXT)
                 .action(ArgAction::SetTrue),
         )
         .arg(
@@ -263,8 +308,7 @@ pub fn uu_app() -> Command {
                 .short('t')
                 .hide(true)
                 .overrides_with(options::BINARY)
-                .action(ArgAction::SetTrue)
-                .requires(options::UNTAGGED),
+                .action(ArgAction::SetTrue),
         )
         .arg(
             Arg::new(options::BINARY)
@@ -280,31 +324,27 @@ pub fn uu_app() -> Command {
                 .long("warn")
                 .help(translate!("cksum-help-warn"))
                 .action(ArgAction::SetTrue)
-                .overrides_with_all([options::STATUS, options::QUIET])
-                .requires(options::CHECK),
+                .overrides_with_all([options::STATUS, options::QUIET]),
         )
         .arg(
             Arg::new(options::STATUS)
                 .long("status")
                 .help(translate!("cksum-help-status"))
                 .action(ArgAction::SetTrue)
-                .overrides_with_all([options::WARN, options::QUIET])
-                .requires(options::CHECK),
+                .overrides_with_all([options::WARN, options::QUIET]),
         )
         .arg(
             Arg::new(options::QUIET)
                 .long(options::QUIET)
                 .help(translate!("cksum-help-quiet"))
                 .action(ArgAction::SetTrue)
-                .overrides_with_all([options::WARN, options::STATUS])
-                .requires(options::CHECK),
+                .overrides_with_all([options::WARN, options::STATUS]),
         )
         .arg(
             Arg::new(options::IGNORE_MISSING)
                 .long(options::IGNORE_MISSING)
                 .help(translate!("cksum-help-ignore-missing"))
-                .action(ArgAction::SetTrue)
-                .requires(options::CHECK),
+                .action(ArgAction::SetTrue),
         )
         .arg(
             Arg::new(options::ZERO)

src/uu/hashsum/src/hashsum.rs

@@ -157,11 +157,19 @@ pub fn uumain(mut args: impl uucore::Args) -> UResult<()> {
     };
     let check = matches.get_flag("check");
 
-    let ignore_missing = matches.get_flag("ignore-missing");
-    let warn = matches.get_flag("warn");
-    let quiet = matches.get_flag("quiet");
-    let strict = matches.get_flag("strict");
-    let status = matches.get_flag("status");
+    let check_flag = |flag| match (check, matches.get_flag(flag)) {
+        (_, false) => Ok(false),
+        (true, true) => Ok(true),
+        (false, true) => Err(ChecksumError::CheckOnlyFlag(flag.into())),
+    };
+
+    // Each of the following flags are only expected in --check mode.
+    // If we encounter them otherwise, end with an error.
+    let ignore_missing = check_flag("ignore-missing")?;
+    let warn = check_flag("warn")?;
+    let quiet = check_flag("quiet")?;
+    let strict = check_flag("strict")?;
+    let status = check_flag("status")?;
 
     // clap provides the default value -. So we unwrap() safety.
     let files = matches
@@ -170,7 +178,18 @@ pub fn uumain(mut args: impl uucore::Args) -> UResult<()> {
         .map(|s| s.as_os_str());
 
     if check {
-        // No reason to allow --check with --binary/--text on Cygwin. It want to be same with Linux and --text was broken for a long time.
+        // on Windows, allow --binary/--text to be used with --check
+        // and keep the behavior of defaulting to binary
+        #[cfg(not(windows))]
+        {
+            let text_flag = matches.get_flag("text");
+            let binary_flag = matches.get_flag("binary");
+
+            if binary_flag || text_flag {
+                return Err(ChecksumError::BinaryTextConflict.into());
+            }
+        }
+
         let verbose = ChecksumVerbose::new(status, quiet, warn);
 
         let opts = ChecksumValidateOptions {
@@ -220,10 +239,6 @@ mod options {
 }
 
 pub fn uu_app_common() -> Command {
-    // --text --arg-deps-check should be error by Arg::new(options::CHECK)...conflicts_with(options::TEXT)
-    // https://github.com/clap-rs/clap/issues/4520 ?
-    // Let --{warn,strict,quiet,status,ignore-missing} reject --text and remove them later.
-    // Bad error message, but not a lie...
     Command::new(uucore::util_name())
         .version(uucore::crate_version!())
         .help_template(uucore::localized_help_template(uucore::util_name()))
@@ -253,9 +268,7 @@ pub fn uu_app_common() -> Command {
                 .long("check")
                 .help(translate!("hashsum-help-check"))
                 .action(ArgAction::SetTrue)
-                .conflicts_with(options::BINARY)
-                .conflicts_with(options::TEXT)
-                .conflicts_with(options::TAG),
+                .conflicts_with("tag"),
         )
         .arg(
             Arg::new(options::TAG)
@@ -287,45 +300,35 @@ pub fn uu_app_common() -> Command {
                 .long(options::QUIET)
                 .help(translate!("hashsum-help-quiet"))
                 .action(ArgAction::SetTrue)
-                .overrides_with_all([options::STATUS, options::WARN])
-                .conflicts_with("text")
-                .requires(options::CHECK),
+                .overrides_with_all([options::STATUS, options::WARN]),
         )
         .arg(
             Arg::new(options::STATUS)
                 .short('s')
                 .long("status")
                 .help(translate!("hashsum-help-status"))
                 .action(ArgAction::SetTrue)
-                .overrides_with_all([options::QUIET, options::WARN])
-                .conflicts_with("text")
-                .requires(options::CHECK),
+                .overrides_with_all([options::QUIET, options::WARN]),
         )
         .arg(
             Arg::new(options::STRICT)
                 .long("strict")
                 .help(translate!("hashsum-help-strict"))
-                .action(ArgAction::SetTrue)
-                .conflicts_with("text")
-                .requires(options::CHECK),
+                .action(ArgAction::SetTrue),
         )
         .arg(
             Arg::new("ignore-missing")
                 .long("ignore-missing")
                 .help(translate!("hashsum-help-ignore-missing"))
-                .action(ArgAction::SetTrue)
-                .conflicts_with("text")
-                .requires(options::CHECK),
+                .action(ArgAction::SetTrue),
         )
         .arg(
             Arg::new(options::WARN)
                 .short('w')
                 .long("warn")
                 .help(translate!("hashsum-help-warn"))
                 .action(ArgAction::SetTrue)
-                .overrides_with_all([options::QUIET, options::STATUS])
-                .conflicts_with("text")
-                .requires(options::CHECK),
+                .overrides_with_all([options::QUIET, options::STATUS]),
         )
         .arg(
             Arg::new("zero")

src/uucore/src/lib/features/checksum/mod.rs

@@ -374,6 +374,9 @@ pub enum ChecksumError {
     #[error("the --raw option is not supported with multiple files")]
     RawMultipleFiles,
 
+    #[error("the --{0} option is meaningful only when verifying checksums")]
+    CheckOnlyFlag(String),
+
     // --length sanitization errors
     #[error("--length required for {}", .0.quote())]
     LengthRequired(String),
@@ -390,6 +393,8 @@ pub enum ChecksumError {
     #[error("--length is only supported with --algorithm blake2b, sha2, or sha3")]
     LengthOnlyForBlake2bSha2Sha3,
 
+    #[error("the --binary and --text options are meaningless when verifying checksums")]
+    BinaryTextConflict,
     #[error("--text mode is only supported with --untagged")]
     TextWithoutUntagged,
     #[error("--check is not supported with --algorithm={{bsd,sysv,crc,crc32b}}")]

tests/by-util/test_cksum.rs

@@ -1066,7 +1066,7 @@ mod output_format {
             .args(&["-a", "md5"])
             .arg(at.subdir.join("f"))
             .fails_with_code(1)
-            .stderr_contains("the following required arguments were not provided"); //clap does not change the meaning
+            .stderr_contains("--text mode is only supported with --untagged");
     }
 
     #[test]
@@ -1216,7 +1216,7 @@ fn test_conflicting_options() {
         .fails_with_code(1)
         .no_stdout()
         .stderr_contains(
-            "cannot be used with", //clap generated error
+            "cksum: the --binary and --text options are meaningless when verifying checksums",
         );
 
     scene
@@ -1228,7 +1228,7 @@ fn test_conflicting_options() {
         .fails_with_code(1)
         .no_stdout()
         .stderr_contains(
-            "cannot be used with", //clap generated error
+            "cksum: the --binary and --text options are meaningless when verifying checksums",
         );
 }
 

tests/by-util/test_hashsum.rs

@@ -268,7 +268,7 @@ fn test_check_md5_ignore_missing() {
         .arg("--ignore-missing")
         .arg(at.subdir.join("testf.sha1"))
         .fails()
-        .stderr_contains("the following required arguments were not provided"); //clap generated error
+        .stderr_contains("the --ignore-missing option is meaningful only when verifying checksums");
 }
 
 #[test]
@@ -1021,13 +1021,13 @@ fn test_check_quiet() {
         .arg("--quiet")
         .arg(at.subdir.join("in.md5"))
         .fails()
-        .stderr_contains("the following required arguments were not provided"); //clap generated error
+        .stderr_contains("md5sum: the --quiet option is meaningful only when verifying checksums");
     scene
         .ccmd("md5sum")
         .arg("--strict")
         .arg(at.subdir.join("in.md5"))
         .fails()
-        .stderr_contains("the following required arguments were not provided"); //clap generated error
+        .stderr_contains("md5sum: the --strict option is meaningful only when verifying checksums");
 }
 
 #[test]

util/build-gnu.sh

@@ -320,9 +320,9 @@ echo "n_stat1 = \$n_stat1"\n\
 echo "n_stat2 = \$n_stat2"\n\
 test \$n_stat1 -ge \$n_stat2 \\' tests/ls/stat-free-color.sh
 
-# clap changes the error message. Check exit code only.
+# no need to replicate this output with hashsum
 "${SED}" -i -e  "s|Try 'md5sum --help' for more information.\\\n||" tests/cksum/md5sum.pl
-"${SED}" -i '/check-ignore-missing-4/,/EXIT/c \     ['\''check-ignore-missing-4'\'', '\''--ignore-missing'\'', {IN=> {f=> '\'''\''}}, {ERR_SUBST=>"s/.*//s"}, {EXIT=> 1}],' tests/cksum/md5sum.pl
+
 # Our ls command always outputs ANSI color codes prepended with a zero. However,
 # in the case of GNU, it seems inconsistent. Nevertheless, it looks like it
 # doesn't matter whether we prepend a zero or not.