Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cakebaker merged 3 commits into from
Feb 3, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2fadd25
cksum: fix --binary reset (issue #6375)
RenjiSann Feb 3, 2025
b8abebf
test(cksum): un-ignore now passing test `test_blake2b_tested_with_sha1`
RenjiSann Feb 3, 2025
f2cf08b
test(cksum): fix and un-ignore `test_md5_bits`
RenjiSann Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 22 additions & 118 deletions src/uu/cksum/src/cksum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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() {
Expand Down
24 changes: 15 additions & 9 deletions src/uucore/src/lib/features/checksum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 1 addition & 4 deletions tests/by-util/test_cksum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,6 @@ fn test_reset_binary() {
.stdout_contains("d41d8cd98f00b204e9800998ecf8427e ");
}

#[ignore = "issue #6375"]
#[test]
Copy link
Copy Markdown
Contributor

@cakebaker cakebaker Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the comment at the end of the function (line 621) is no longer needed?

Copy link
Copy Markdown
Collaborator Author

@RenjiSann RenjiSann Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not indeed, thanks for catching this !

Copy link
Copy Markdown
Collaborator Author

@RenjiSann RenjiSann Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 👍

fn test_reset_binary_but_set() {
let scene = TestScenario::new(util_name!());
Expand All @@ -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]
Expand Down Expand Up @@ -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!();
Expand Down Expand Up @@ -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!();
Expand Down