Merge pull request #7261 from RenjiSann/cksum-6375

cksum: Fix #6375 and un-ignore now passing tests

Author: cakebaker

src/uu/cksum/src/cksum.rs

@@ -205,61 +205,33 @@ mod options {
     pub const ZERO: &str = "zero";
 }
 
-/// Determines whether to prompt an asterisk (*) in the output.
-///
-/// This function checks the `tag`, `binary`, and `had_reset` flags and returns a boolean
-/// indicating whether to prompt an asterisk (*) in the output.
-/// It relies on the overrides provided by clap (i.e., `--binary` overrides `--text` and vice versa).
-/// Same for `--tag` and `--untagged`.
-fn prompt_asterisk(tag: bool, binary: bool, had_reset: bool) -> bool {
-    if tag {
-        return false;
-    }
-    if had_reset {
-        return false;
-    }
-    binary
-}
-
-/**
- * Determine if we had a reset.
- * This is basically a hack to support the behavior of cksum
- * when we have the following arguments:
- * --binary --tag --untagged
- * Don't do it with clap because if it struggling with the --overrides_with
- * marking the value as set even if not present
- */
-fn had_reset(args: &[OsString]) -> bool {
-    // Indices where "--binary" or "-b", "--tag", and "--untagged" are found
-    let binary_index = args.iter().position(|x| x == "--binary" || x == "-b");
-    let tag_index = args.iter().position(|x| x == "--tag");
-    let untagged_index = args.iter().position(|x| x == "--untagged");
-
-    // Check if all arguments are present and in the correct order
-    match (binary_index, tag_index, untagged_index) {
-        (Some(b), Some(t), Some(u)) => b < t && t < u,
-        _ => false,
-    }
-}
-
 /***
  * cksum has a bunch of legacy behavior.
  * We handle this in this function to make sure they are self contained
  * and "easier" to understand
  */
-fn handle_tag_text_binary_flags(matches: &clap::ArgMatches) -> UResult<(bool, bool)> {
-    let untagged = matches.get_flag(options::UNTAGGED);
-    let tag = matches.get_flag(options::TAG);
-    let tag = tag || !untagged;
-
-    let binary_flag = matches.get_flag(options::BINARY);
-
-    let args: Vec<OsString> = std::env::args_os().collect();
-    let had_reset = had_reset(&args);
-
-    let asterisk = prompt_asterisk(tag, binary_flag, had_reset);
+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;
+
+    // --binary, --tag and --untagged are tight together: none of them
+    // conflicts with each other but --tag will reset "binary" and set "tag".
+
+    for arg in args {
+        let arg = arg.as_ref();
+        if arg == "-b" || arg == "--binary" {
+            binary = true;
+        } else if arg == "--tag" {
+            tag = true;
+            binary = false;
+        } else if arg == "--untagged" {
+            tag = false;
+        }
+    }
 
-    Ok((tag, asterisk))
+    Ok((tag, !tag && binary))
 }
 
 #[uucore::main]
@@ -336,7 +308,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         return perform_checksum_validation(files.iter().copied(), algo_option, length, opts);
     }
 
-    let (tag, asterisk) = handle_tag_text_binary_flags(&matches)?;
+    let (tag, asterisk) = handle_tag_text_binary_flags(std::env::args_os())?;
 
     let algo = detect_algo(algo_name, length)?;
     let line_ending = LineEnding::from_zero_flag(matches.get_flag(options::ZERO));
@@ -501,75 +473,7 @@ pub fn uu_app() -> Command {
 
 #[cfg(test)]
 mod tests {
-    use super::had_reset;
     use crate::calculate_blake2b_length;
-    use crate::prompt_asterisk;
-    use std::ffi::OsString;
-
-    #[test]
-    fn test_had_reset() {
-        let args = ["--binary", "--tag", "--untagged"]
-            .iter()
-            .map(|&s| s.into())
-            .collect::<Vec<OsString>>();
-        assert!(had_reset(&args));
-
-        let args = ["-b", "--tag", "--untagged"]
-            .iter()
-            .map(|&s| s.into())
-            .collect::<Vec<OsString>>();
-        assert!(had_reset(&args));
-
-        let args = ["-b", "--binary", "--tag", "--untagged"]
-            .iter()
-            .map(|&s| s.into())
-            .collect::<Vec<OsString>>();
-        assert!(had_reset(&args));
-
-        let args = ["--untagged", "--tag", "--binary"]
-            .iter()
-            .map(|&s| s.into())
-            .collect::<Vec<OsString>>();
-        assert!(!had_reset(&args));
-
-        let args = ["--untagged", "--tag", "-b"]
-            .iter()
-            .map(|&s| s.into())
-            .collect::<Vec<OsString>>();
-        assert!(!had_reset(&args));
-
-        let args = ["--binary", "--tag"]
-            .iter()
-            .map(|&s| s.into())
-            .collect::<Vec<OsString>>();
-        assert!(!had_reset(&args));
-
-        let args = ["--tag", "--untagged"]
-            .iter()
-            .map(|&s| s.into())
-            .collect::<Vec<OsString>>();
-        assert!(!had_reset(&args));
-
-        let args = ["--text", "--untagged"]
-            .iter()
-            .map(|&s| s.into())
-            .collect::<Vec<OsString>>();
-        assert!(!had_reset(&args));
-
-        let args = ["--binary", "--untagged"]
-            .iter()
-            .map(|&s| s.into())
-            .collect::<Vec<OsString>>();
-        assert!(!had_reset(&args));
-    }
-
-    #[test]
-    fn test_prompt_asterisk() {
-        assert!(!prompt_asterisk(true, false, false));
-        assert!(!prompt_asterisk(false, false, true));
-        assert!(prompt_asterisk(false, true, false));
-        assert!(!prompt_asterisk(false, false, false));
-    }
 
     #[test]
     fn test_calculate_length() {

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

@@ -692,7 +692,7 @@ fn identify_algo_name_and_length(
     line_info: &LineInfo,
     algo_name_input: Option<&str>,
     last_algo: &mut Option<String>,
-) -> Option<(String, Option<usize>)> {
+) -> Result<(String, Option<usize>), LineCheckError> {
     let algo_from_line = line_info.algo_name.clone().unwrap_or_default();
     let algorithm = algo_from_line.to_lowercase();
     *last_algo = Some(algo_from_line);
@@ -701,18 +701,25 @@ fn identify_algo_name_and_length(
     // (for example SHA1 (f) = d...)
     // Also handle the case cksum -s sm3 but the file contains other formats
     if algo_name_input.is_some() && algo_name_input != Some(&algorithm) {
-        return None;
+        return Err(LineCheckError::ImproperlyFormatted);
     }
 
     if !SUPPORTED_ALGORITHMS.contains(&algorithm.as_str()) {
         // Not supported algo, leave early
-        return None;
+        return Err(LineCheckError::ImproperlyFormatted);
     }
 
     let bytes = if let Some(bitlen) = line_info.algo_bit_len {
-        if bitlen % 8 != 0 {
-            // The given length is wrong
-            return None;
+        if algorithm != ALGORITHM_OPTIONS_BLAKE2B || bitlen % 8 != 0 {
+            // Either
+            //  the algo based line is provided with a bit length
+            //  with an algorithm that does not support it (only Blake2B does).
+            //
+            //  eg: MD5-128 (foo.txt) = fffffffff
+            //          ^ This is illegal
+            // OR
+            //  the given length is wrong because it's not a multiple of 8.
+            return Err(LineCheckError::ImproperlyFormatted);
         }
         Some(bitlen / 8)
     } else if algorithm == ALGORITHM_OPTIONS_BLAKE2B {
@@ -722,7 +729,7 @@ fn identify_algo_name_and_length(
         None
     };
 
-    Some((algorithm, bytes))
+    Ok((algorithm, bytes))
 }
 
 /// Given a filename and an algorithm, compute the digest and compare it with
@@ -773,8 +780,7 @@ fn process_algo_based_line(
     let filename_to_check = line_info.filename.as_slice();
 
     let (algo_name, algo_byte_len) =
-        identify_algo_name_and_length(line_info, cli_algo_name, last_algo)
-            .ok_or(LineCheckError::ImproperlyFormatted)?;
+        identify_algo_name_and_length(line_info, cli_algo_name, last_algo)?;
 
     // If the digest bitlen is known, we can check the format of the expected
     // checksum with it.

tests/by-util/test_cksum.rs

@@ -602,7 +602,6 @@ fn test_reset_binary() {
         .stdout_contains("d41d8cd98f00b204e9800998ecf8427e  ");
 }
 
-#[ignore = "issue #6375"]
 #[test]
 fn test_reset_binary_but_set() {
     let scene = TestScenario::new(util_name!());
@@ -619,7 +618,7 @@ fn test_reset_binary_but_set() {
         .arg("--algorithm=md5")
         .arg(at.subdir.join("f"))
         .succeeds()
-        .stdout_contains("d41d8cd98f00b204e9800998ecf8427e *"); // currently, asterisk=false. It should be true
+        .stdout_contains("d41d8cd98f00b204e9800998ecf8427e *");
 }
 
 #[test]
@@ -1137,7 +1136,6 @@ fn test_cksum_garbage() {
         .stderr_contains("check-file: no properly formatted checksum lines found");
 }
 
-#[ignore = "Should fail on bits"]
 #[test]
 fn test_md5_bits() {
     let (at, mut ucmd) = at_and_ucmd!();
@@ -1188,7 +1186,6 @@ fn test_bsd_case() {
         .stderr_contains("f: no properly formatted checksum lines found");
 }
 
-#[ignore = "Different output"]
 #[test]
 fn test_blake2d_tested_with_sha1() {
     let (at, mut ucmd) = at_and_ucmd!();