Fix macos filename encoding #949
Conversation
|
Traveling right now, can only review in detail in 2 weeks. Some points already:
|
This commit resolves rfjakob#850 by addressing Unicode normalization mismatches on macOS between NFC (used by CLI tools) and NFD (used by GUI apps). The solution is inspired by Cryptomator's approach. Forward mode now enforces NFC for storage and adds fallback lookups in NFD. Reverse mode adds fallback lookups without modifying the plaintext FS.
rafjaf
force-pushed
the
rebase-onto-2.6.0
branch
from
1867ecb to
58f1ec0
Compare
|
Thanks, I have already changed the commit message and will implement the other changes asap |
|
@rfjakob As requested, I have:
Also, after further reflection, I have cancelled all changes that I had made to the reverse mode. Indeed, in reverse mode, the plaintext folder is just an ordinary folder, meaning that macos is already doing the normalization required; and as far as I can tell, the existing version of gocrypts has no problem to encode both NFC and NFD filenames to the ciphertext folder. The PR is therefore much simplified. I hope this works for you and remain at your disposal to discuss this. |
|
Sorry, I had forgotten to revert the changes in /internal/fusefrontend_reverse/node_dirs_ops.go, now all the changes in reverse mode should effectively be cancelled. |
rafjaf
changed the title
rafjaf
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes macOS filename encoding issues by implementing Unicode normalization fallback to handle the difference between NFC and NFD character encodings that macOS uses. The fix ensures files created with either normalization form can be accessed using the other form, improving compatibility with macOS filesystem conventions.
- Implements Unicode normalization logic in
prepareAtSyscallto try both NFC and NFD forms when accessing files - Normalizes filenames to NFC for internal storage while displaying them as NFD for macOS GUI compatibility
- Adds comprehensive test suite covering various Unicode characters and edge cases
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/macos_filename_encoding/test.bash |
Test runner script for macOS filename encoding tests |
tests/macos_filename_encoding/nfc_nfd_test.go |
Comprehensive test suite covering Unicode normalization scenarios |
internal/fusefrontend/node_prepare_syscall.go |
Core Unicode normalization logic with NFC/NFD fallback |
internal/fusefrontend/node_open_create.go |
Normalize filenames during file creation |
internal/fusefrontend/node_dir_ops.go |
Directory operations and filename normalization utilities |
internal/fusefrontend/node.go |
Normalize filenames in various file operations |
internal/fusefrontend/file_dir_ops.go |
Directory listing with NFD display normalization |
go.mod |
Updated Go version and added text/unicode dependency |
Comments suppressed due to low confidence (2)
go.mod:3
- Go version 1.23.0 does not exist. The latest stable Go version as of my knowledge cutoff is 1.22.x. Please use a valid Go version like 1.22 or 1.21.
go 1.23.0
go.mod:5
- Go toolchain version 1.24.4 does not exist. The toolchain version should match an actual Go release version.
toolchain go1.24.4
| } | ||
| } | ||
|
|
||
| return n.prepareAtSyscallDirect(child) // Non-macOS or fallback |
There was a problem hiding this comment.
[nitpick] The logic for trying different normalization forms is complex and could be simplified. Consider extracting the normalization attempts into a separate helper function to improve readability and maintainability.
| return n.prepareAtSyscallDirect(child) // Non-macOS or fallback | |
| return n.tryNormalizationForms(child) | |
| } | |
| return n.prepareAtSyscallDirect(child) // Non-macOS or fallback | |
| } | |
| // tryNormalizationForms tries different Unicode normalization forms for macOS | |
| func (n *Node) tryNormalizationForms(child string) (dirfd int, cName string, errno syscall.Errno) { | |
| // Step 1: Always try NFC first (canonical storage form) | |
| normalizedChild := norm.NFC.String(child) | |
| dirfd, cName, errno = n.prepareAtSyscallDirect(normalizedChild) | |
| if errno == 0 { | |
| return dirfd, cName, 0 // Found NFC version | |
| } | |
| // Step 2: Try alternate form if input was different | |
| if normalizedChild != child { | |
| // Input was NFD, try original NFD form | |
| dirfdNFD, cNameNFD, errnoNFD := n.prepareAtSyscallDirect(child) | |
| if errnoNFD == 0 { | |
| return dirfdNFD, cNameNFD, 0 | |
| } | |
| } | |
| // Step 3: If input was NFC, also try NFD as fallback | |
| if normalizedChild == child { | |
| nfdChild := norm.NFD.String(child) | |
| if nfdChild != child { | |
| dirfdNFD, cNameNFD, errnoNFD := n.prepareAtSyscallDirect(nfdChild) | |
| if errnoNFD == 0 { | |
| return dirfdNFD, cNameNFD, 0 | |
| } | |
| } | |
| } | |
| return n.prepareAtSyscallDirect(child) |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| // Step 3: If input was NFC, also try NFD as fallback | ||
| if normalizedChild == child { |
There was a problem hiding this comment.
This condition checks if input was NFC after already trying NFC and NFD. This creates redundant NFD normalization when the input was already NFC. Consider restructuring to avoid duplicate normalization calls.
| if normalizedChild == child { | |
| if normalizedChild != child { | |
| // Input was NFD, try original NFD form | |
| dirfdNFD, cNameNFD, errnoNFD := n.prepareAtSyscallDirect(child) | |
| if errnoNFD == 0 { | |
| return dirfdNFD, cNameNFD, 0 | |
| } | |
| } else { | |
| // Input was NFC, try NFD as fallback if different |
Copilot uses AI. Check for mistakes.
|
@rfjakob Just to be sure, do you wish me to implement all or part of Copilot's suggestions, or do you prefer to take over from here? |
I'll take over |
2 participants
I rewrote PR #936 (where more information is given) to adapt it to version 2.6.0 (the dirstream fix had become obsolete since switching to the new go-fuse directory API).