Skip to content
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

Jump to bottom

checksum: rework for improving checkum checking GNU behavior match #6654

Closed
wants to merge 20 commits into from
Closed

checksum: rework for improving checkum checking GNU behavior match #6654

wants to merge 20 commits into from

Conversation

RenjiSann
Copy link
Collaborator

This PR makes a significant refactor of the checksum checking code.

The current architecture prevents us to fix #6572, #6614 and #6653.

For #6614, we will need to implement a "retry" step in case we matched the hexa regex and we wish to try again considering the checksum as base64.

The refactor mainly consists in decomposing and extracting functionalities, and improving error management.

It adds several tests, for #6653 and #6572.

Its merge is gated by #6603, as it depends on its commits.

@RenjiSann
Copy link
Collaborator Author

I'm aware that this PR is consequent and that the rewrite is complex and difficult to review.

That's why I tried to make meaningful commits so its easier to go step by step.

I'm also considering to split this PR into several smaller ones, but I'm not sure where to split.

@RenjiSann RenjiSann mentioned this pull request Aug 17, 2024
Closed
@RenjiSann RenjiSann force-pushed the cksum-rework-bis branch 2 times, most recently from 8a191be to 1d56096 Compare August 26, 2024 13:17
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

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

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

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

sylvestre
sylvestre reviewed Aug 29, 2024
View reviewed changes
src/uucore/src/lib/features/checksum/algo.rs
length: Option<usize>,
}

impl ChecksumAlgoBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a rustdoc comment here

@RenjiSann RenjiSann force-pushed the cksum-rework-bis branch 4 times, most recently from 63362c5 to 86fe708 Compare August 30, 2024 09:59
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/cksum/cksum-base64. tests/cksum/cksum-base64 is passing on 'main'. Maybe you have to rebase?

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/cksum/cksum-base64. tests/cksum/cksum-base64 is passing on 'main'. Maybe you have to rebase?

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/cksum/cksum-base64. tests/cksum/cksum-base64 is passing on 'main'. Maybe you have to rebase?

@RenjiSann RenjiSann force-pushed the cksum-rework-bis branch 2 times, most recently from 1d6dbc4 to 0c8729d Compare August 31, 2024 23:45
@RenjiSann
Copy link
Collaborator Author

I've finally managed to fix #6572, which needed a consequent rewrite of the logic.
It still misses stuff like proper documentation, and proper refactoring for it to be maintainable.
It might take me some times to to it, as I won't be available that much in the coming weeks.

But hey, at least it works :)

@RenjiSann
Copy link
Collaborator Author

changes: lint fixes + windows code error (as usual :')

@sylvestre
Copy link
Contributor

it is still marked as draft, is it ready to be reviewed?

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/mv/mv-n is no longer failing!
Congrats! The gnu test tests/mv/update is no longer failing!

@RenjiSann
cksum: simplify nesting
5842f14
@RenjiSann
checksum: Allow for non UTF-8 input
ccf5559
@RenjiSann
test(checksum): add a test for non UTF-8 chars in comments
780db80
@RenjiSann
test(cksum): add multiple tests
f6f0ed9 
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
@RenjiSann
feat(checksum): extract algorithm detection in a specific file
b5bb0a7
@RenjiSann
feat(checksum): move u8/str/OsString conversion to utils
7246067
@RenjiSann
fix(checksum): lines should not be trimmed before regex matching
c00a444
@RenjiSann
feat(checksum): extract line processing into a separate function
4570f1b
@RenjiSann
feat(checksum): add a ChecksumOption struct to bundle CLI flags
57cd029
@RenjiSann
feat(checksum): extract utility functions to specific file to make lo…
0e316c2 
…gic simpler
@RenjiSann
feat(checksum): introduce a LineCheckError type
0aeed93
@RenjiSann
feat(checksum): small type/docstring changes
af925e6
@RenjiSann
feat(checksum): refactor get_expected_checksum
a0b301c
@RenjiSann
feat(checksum): extract checksum file processing in a separate function
b1d0701
@RenjiSann
checksum: handle no-length case in algo-based BLAKE2b
48531e9
@RenjiSann
feat(checksum): get rid of specific Base64 regex
7dce4f2
@RenjiSann
cksum: Fix handling of both encodings in a single file
cf8811d
@RenjiSann
feat(checksum): prevent incorrectly formatted checksums from stopping…
c9fa70f 
… the program
@RenjiSann
feat(checksum): get rid of ChecksumResult at the line level
5fed16f
@RenjiSann
checksum: handle hexadecimal/base64 confusion and retry
f5f6d6c
@RenjiSann RenjiSann mentioned this pull request Oct 25, 2024
Merged
@sylvestre
Copy link
Contributor

@RenjiSann it has been merged in other PR, right ?
thanks

@RenjiSann
Copy link
Collaborator Author

@RenjiSann it has been merged in other PR, right ?

Right, we can close this

@RenjiSann RenjiSann closed this Dec 2, 2024
@sylvestre
Copy link
Contributor

thanks :)

@RenjiSann RenjiSann deleted the cksum-rework-bis branch January 29, 2025 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvestre sylvestre sylvestre left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cksum: gets confused by base64 that happens to consist entirely of hexadecimal digits
2 participants
@RenjiSann @sylvestre