cksum: Fix #6375 and un-ignore now passing tests #7261
Conversation
|
GNU testsuite comparison: |
RenjiSann
changed the title
RenjiSann
changed the title
|
GNU testsuite comparison: |
| @@ -602,7 +602,6 @@ fn test_reset_binary() { | |||
| .stdout_contains("d41d8cd98f00b204e9800998ecf8427e "); | |||
| } | |||
|
|
|||
| #[ignore = "issue #6375"] | |||
| #[test] | |||
There was a problem hiding this comment.
I guess the comment at the end of the function (line 621) is no longer needed?
There was a problem hiding this comment.
It is not indeed, thanks for catching this !
| if algorithm != ALGORITHM_OPTIONS_BLAKE2B { | ||
| // An invalid "bits" part was given to the regex | ||
| // eg: MD5-128 (foo.txt) = fffffffff | ||
| return None; | ||
| } | ||
| if bitlen % 8 != 0 { | ||
| // The given length is wrong |
There was a problem hiding this comment.
Wouldn't it be easier to read if you check for the positive case? Something like
if algorithm == ALGORITHM_OPTIONS_BLAKE2B && bitlen % 8 == 0 {
Some(bitlen / 8)
} else {
return None;
}There was a problem hiding this comment.
I merged both conditions in a single if statement, but I find it is more understandable to check for the negative case. I've also slightly refactored the identify_algo_name_and_length function to make it return an error instead of an option. It helps making the difference between the error case and the case where no bit size was given.
WDYT ?
| @@ -710,6 +710,11 @@ fn identify_algo_name_and_length( | |||
| } | |||
|
|
|||
| let bytes = if let Some(bitlen) = line_info.algo_bit_len { | |||
| if algorithm != ALGORITHM_OPTIONS_BLAKE2B { | |||
| // An invalid "bits" part was given to the regex | |||
There was a problem hiding this comment.
To me as a reader it's unclear what regex you refer to because this function doesn't use a regex.
There was a problem hiding this comment.
fair enough, I'll rephrase this
|
GNU testsuite comparison: |
src/uu/cksum/src/cksum.rs
Outdated
There was a problem hiding this comment.
Much cleaner than the previous approach :)
There was a problem hiding this comment.
Yup, it's smaller, only iterates once, and is actually the expected behavior :)
|
GNU testsuite comparison: |
|
Thanks! |
Fixes #6375, and 2 other ignored tests