checksum: prepare further behavior fix with a rework #6822

RenjiSann · 2024-10-25T03:10:15Z

Now that #6782, #6793 and #6815 have landed, it is time to achieve the rework of checksum.rs started in #6654.

This will allow us to fix #6572, #6614 and #6653.

This PR mainly tries to split the massive perform_checksum_verification in smaller functions, and to get rid of the &mut ChecksumResult that is passed everywhere, which makes it very difficult to understand where the global state of the program is modified.

Also, it adds ignored tests for future features (it will prevent too many conflicts when rebasing future PRs on each other).

Next PRs will focus on reworking the algorithm detection behavior, in particular for blake2b size guessing,
Regex detection behavior which for now prevents us from encountering both hexa and base64 digits in the same file.

RenjiSann · 2024-10-25T03:12:24Z

Depending on the feedback on this discussion, I might split the checksum.rs file into several files (a bit like the initial PR #6654).
I'd like to move Error structs to checksum/error.rs, everything that's Algorithm-related to checksum/algo.rs, etc...

What do you think ?

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

cakebaker · 2024-10-25T07:07:18Z

I'd like to move Error structs to checksum/error.rs, everything that's Algorithm-related to checksum/algo.rs, etc...

What do you think ?

It sounds good to me.

tests/by-util/test_cksum.rs

github-actions · 2024-10-25T13:15:03Z

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

tests/by-util/test_cksum.rs

src/uu/cksum/src/cksum.rs

RenjiSann · 2024-10-25T15:20:18Z

I've finally decided to keep the splitting in several files for later, because it's a nightmare to mix changes and file architecture when you have to rebase to keep a sound change history.

github-actions · 2024-10-25T16:05:08Z

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

RenjiSann · 2024-10-25T16:16:59Z

I'm not sure why the macos test failed. Is it just flakey or have I touched something ?

sylvestre · 2024-10-25T17:25:49Z

flaky:

---- test_stat::test_stdin_pipe_fifo1 stdout ----
run: /Users/runner/work/coreutils/coreutils/target/aarch64-apple-darwin/debug/coreutils stat -
run: /Users/runner/work/coreutils/coreutils/target/aarch64-apple-darwin/debug/coreutils stat -L -
thread 'test_stat::test_stdin_pipe_fifo1' panicked at ''  File: -
  Size: 0         	Blocks: 0          IO Block: 512    weird file
Device: 4ef1bb9ch/1324465052d	Inode: 500453238   Links: 0
Access: (0660/?rw-rw----)  Uid: (  501/  runner)   Gid: (   20/   staff)
Access: 2024-10-25 15:48:53.191317902 +0000
Modify: 2024-10-25 15:48:53.191317902 +0000
Change: 2024-10-25 15:48:53.191317902 +0000
 Birth: 1970-01-01 00:00:00.000000000 +0000
' does not contain 'fifo'', tests/by-util/test_stat.rs:288:10

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

github-actions · 2024-10-27T16:21:34Z

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

RenjiSann · 2024-10-27T16:26:42Z

@cakebaker Both of your comments address the details of implementation I came to when working on it.
If you come up with better alternatives, I'll gladly hear them ^^

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

cakebaker · 2024-11-02T09:43:27Z

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

+    res: &mut ChecksumResult,
+    cli_algo_name: Option<&str>,
+    cli_algo_length: Option<usize>,
+    properly_formatted: &mut bool,


Is this argument necessary? There's already LineCheckError::ImproperlyFormatted.

With the way the program currently works, it is complicated to completely get rid of it.
I plan to get rid of it with the following PRs that will be in charge of fixing the tests that are added but ignored by this PR.

You can take a look at my previous, now deprecated, PR #6654, which fixed everything at once, but which was a pain to review.

cakebaker

Overall good job with this refactoring :)

…, LineCheckError> - Treat digest mismatch as an error

test(cksum): add test for blake length gessing test(cksum): add test for hexa/base64 confusion test(cksum): add test for error handling on incorrectly formatted checksum test(cksum): add test for trailing spaces making a line improperly formatted test(cksum): Re-implement GNU test 'cksum-base64' in the testsuite

github-actions · 2024-11-03T11:11:50Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

github-actions · 2024-11-03T11:44:09Z

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

RenjiSann · 2024-11-06T20:18:42Z

Up @cakebaker
Once this is done, I will move on and fix the tests I added :)

sylvestre reviewed Oct 25, 2024

View reviewed changes

src/uucore/src/lib/features/checksum.rs Show resolved Hide resolved

sylvestre reviewed Oct 25, 2024

View reviewed changes

src/uucore/src/lib/features/checksum.rs Show resolved Hide resolved

RenjiSann force-pushed the cksum-big-rework branch 2 times, most recently from 7898120 to aa5f0c0 Compare October 25, 2024 12:48