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.

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

cksum: accept non-UTF-8 filenames #6575

Merged
merged 3 commits into from
Jul 30, 2024

Conversation

BenWiederhake
Copy link
Collaborator

Fixes #6574.

Example:

$ touch $'funky\xffname'
$ cargo run -q cksum $'funky\xffname'
4294967295 0 funky�name
$ cksum $'funky\xffname'
4294967295 0 funky�name

@BenWiederhake
Copy link
Collaborator Author

CI failures are unrelated issues:

---- test_stat::test_stdin_pipe_fifo1 stdout ----
run: /Users/runner/work/coreutils/coreutils/target/aarch64-apple-darwin/debug/coreutils stat -
thread 'test_stat::test_stdin_pipe_fifo1' panicked at 'Expected stderr to be empty, but it's:
stat: cannot stat '-': Bad file descriptor (os error 9)
', tests/by-util/test_stat.rs:279:10

@BenWiederhake BenWiederhake marked this pull request as draft July 16, 2024 01:00
@BenWiederhake BenWiederhake marked this pull request as ready for review July 16, 2024 01:57
@BenWiederhake
Copy link
Collaborator Author

Changes since initial push:

  • Added a test (doink!)
  • Changed the output method so that non-UTF-8 filenames are correctly output (without Unicode replacement encoding or other loss)
  • Unified the "OsStr(ing) to bytes" conversion with cut, as already suggested in tr: Accept non UTF-8 arguments for sets #6563

@BenWiederhake BenWiederhake force-pushed the dev-cksum-nonutf8-filename branch 4 times, most recently from 415c1f7 to 3c949a7 Compare July 16, 2024 03:46
@BenWiederhake
Copy link
Collaborator Author

Changes since last push description:

  • Formatting (looks like something disabled my pre-commit hook, whoops)
  • Added missing #[cfg(unix)] on use
  • Fix the unified definition to work on windows – turns out, a if cfg!() branch must successfully compile even if it is not taken.
  • Added some spellcheck exceptions. (These should really be added to the pre-commit hook)
  • Restrict the test to linux-only, because MacOS doesn't allow non-UTF-8 filenames in the first place

@BenWiederhake BenWiederhake force-pushed the dev-cksum-nonutf8-filename branch 2 times, most recently from 26957a3 to a3efe5b Compare July 16, 2024 04:13
@BenWiederhake
Copy link
Collaborator Author

Changes since last push description:

  • Now that the test is restricted, the initial import must only happen on linux. Aaargh!
  • Both of them.

Remaining CI failures are unrelated:

@BenWiederhake BenWiederhake marked this pull request as draft July 16, 2024 14:57
@BenWiederhake BenWiederhake marked this pull request as ready for review July 19, 2024 18:35
@BenWiederhake BenWiederhake force-pushed the dev-cksum-nonutf8-filename branch 3 times, most recently from 66112fb to 3d5232b Compare July 19, 2024 18:44
@BenWiederhake
Copy link
Collaborator Author

Changes since last push description:

"{} {}",
sum.parse::<u16>().unwrap(),
div_ceil(sz, options.output_bits)
let (before_filename, print_filename, after_filename) = match (options.algo_name, not_file)
Copy link
Contributor

@cakebaker cakebaker Jul 23, 2024

Choose a reason for hiding this comment

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

The print_filename var name is a bit misleading and I was surprised it's of type bool. I expected it to be a String, like the other two vars. My suggestion is to rename it to something like should_print_filename.

Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm2 is no longer failing!

@BenWiederhake
Copy link
Collaborator Author

Changes since (my?) last push:

  • Remove notfile from the match structure, reducing code duplication
  • Rename print_filename to should_print_filename
  • Reduce filename copying in the test

@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

  • Turn if !not_file { " " } else { "" } into if not_file { "" } else { " " } to make clippy happy (why wasn't this caught by pre-commit?)

@cakebaker cakebaker merged commit 9fbfb29 into uutils:main Jul 30, 2024
67 of 68 checks passed
@cakebaker
Copy link
Contributor

Thanks :)

@BenWiederhake BenWiederhake deleted the dev-cksum-nonutf8-filename branch July 30, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cksum: can't handle non-UTF-8 filenames
3 participants