feat: casefold #3114
Merged
feat: casefold #3114
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR introduces casefold functionality to handle case-insensitive file operations on various filesystems. The changes refactor the existing case validation system to provide more robust case-insensitive filename handling.
- Replaces
valid_name_casewithcasefoldfunction that returns the actual cased filename - Adds platform-specific implementations for casefold operations across Unix-like systems and Windows
- Updates filesystem operations to use the new casefold functionality for proper case handling
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| yazi-watcher/src/watcher.rs | Updates file watcher to use new case matching function |
| yazi-fs/src/provider/provider.rs | Adds public casefold function to provider interface |
| yazi-fs/src/provider/local/local.rs | Updates parameter names for consistency |
| yazi-fs/src/provider/local/casefold.rs | Replaces case validation with comprehensive casefold implementation |
| yazi-fs/src/fns.rs | Removes old realname functionality and associated tests |
| yazi-actor/src/mgr/rename.rs | Updates rename operations to use casefold for proper case handling |
| yazi-actor/src/mgr/create.rs | Updates file creation to use casefold functionality |
| cspell.json | Adds "inodes" to dictionary |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
+22
to
+26
| use std::{ffi::{CStr, CString, OsString}, os::{fd::{AsRawFd, FromRawFd, OwnedFd}, unix::ffi::OsStringExt}}; | ||
|
|
||
| use libc::{F_GETPATH, O_RDONLY, O_SYMLINK, PATH_MAX}; | ||
|
|
||
| let cstr = CString::new(path.into_os_string().into_encoded_bytes())?; | ||
| let Some(name) = Path::new(OsStr::from_bytes(cstr.as_bytes())).file_name() else { | ||
| return Ok(true); | ||
| }; | ||
|
|
||
| let cstr = CString::new(path.into_os_string().into_vec())?; |
There was a problem hiding this comment.
The into_vec() conversion on line 26 and the subsequent vector operations could be inefficient for large paths. Consider using byte slice operations where possible to avoid unnecessary allocations.
Copilot uses AI. Check for mistakes.
Comment on lines
+71
to
+86
| let mut entries: Vec<_> = std::fs::read_dir(parent)? | ||
| .filter_map(Result::ok) | ||
| .filter_map(|e| e.metadata().ok().map(|m| (e, m))) | ||
| .filter(|(_, m)| m.dev() == meta.dev() && m.ino() == meta.ino()) | ||
| .map(|(e, _)| e.path()) | ||
| .collect(); | ||
|
|
||
| if entries.len() == 1 { | ||
| // No hardlink that shares the same inode | ||
| Ok(entries.remove(0)) | ||
| } else if let Some(i) = entries.iter().position(|p| p == path) { | ||
| // Exact match | ||
| Ok(entries.swap_remove(i)) | ||
| } else if let Some(i) = entries.iter().position(|p| p.as_os_str().eq_ignore_ascii_case(oss)) { | ||
| // Case-insensitive match | ||
| Ok(entries.swap_remove(i)) |
There was a problem hiding this comment.
Reading the entire directory and filtering by inode could be inefficient for large directories. This creates an O(n) operation for each casefold call. Consider caching directory contents or using a more efficient lookup strategy.
Suggested change
| let mut entries: Vec<_> = std::fs::read_dir(parent)? | |
| .filter_map(Result::ok) | |
| .filter_map(|e| e.metadata().ok().map(|m| (e, m))) | |
| .filter(|(_, m)| m.dev() == meta.dev() && m.ino() == meta.ino()) | |
| .map(|(e, _)| e.path()) | |
| .collect(); | |
| if entries.len() == 1 { | |
| // No hardlink that shares the same inode | |
| Ok(entries.remove(0)) | |
| } else if let Some(i) = entries.iter().position(|p| p == path) { | |
| // Exact match | |
| Ok(entries.swap_remove(i)) | |
| } else if let Some(i) = entries.iter().position(|p| p.as_os_str().eq_ignore_ascii_case(oss)) { | |
| // Case-insensitive match | |
| Ok(entries.swap_remove(i)) | |
| let mut inode_map: HashMap<(u64, u64), Vec<PathBuf>> = HashMap::new(); | |
| for entry in std::fs::read_dir(parent)? { | |
| if let Ok(e) = entry { | |
| if let Ok(m) = e.metadata() { | |
| let key = (m.dev(), m.ino()); | |
| inode_map.entry(key).or_default().push(e.path()); | |
| } | |
| } | |
| } | |
| let key = (meta.dev(), meta.ino()); | |
| let entries = inode_map.get_mut(&key); | |
| if let Some(entries) = entries { | |
| if entries.len() == 1 { | |
| // No hardlink that shares the same inode | |
| Ok(entries.remove(0)) | |
| } else if let Some(i) = entries.iter().position(|p| p == path) { | |
| // Exact match | |
| Ok(entries.swap_remove(i)) | |
| } else if let Some(i) = entries.iter().position(|p| p.as_os_str().eq_ignore_ascii_case(oss)) { | |
| // Case-insensitive match | |
| Ok(entries.swap_remove(i)) | |
| } else { | |
| Err(io::Error::new(io::ErrorKind::NotFound, "file not found")) | |
| } |
Copilot uses AI. Check for mistakes.
Comment on lines
+78
to
+86
| if entries.len() == 1 { | ||
| // No hardlink that shares the same inode | ||
| Ok(entries.remove(0)) | ||
| } else if let Some(i) = entries.iter().position(|p| p == path) { | ||
| // Exact match | ||
| Ok(entries.swap_remove(i)) | ||
| } else if let Some(i) = entries.iter().position(|p| p.as_os_str().eq_ignore_ascii_case(oss)) { | ||
| // Case-insensitive match | ||
| Ok(entries.swap_remove(i)) |
There was a problem hiding this comment.
[nitpick] The logic for handling different numbers of entries (1, exact match, case-insensitive match) could be simplified and made more readable by restructuring the conditional flow.
Suggested change
| if entries.len() == 1 { | |
| // No hardlink that shares the same inode | |
| Ok(entries.remove(0)) | |
| } else if let Some(i) = entries.iter().position(|p| p == path) { | |
| // Exact match | |
| Ok(entries.swap_remove(i)) | |
| } else if let Some(i) = entries.iter().position(|p| p.as_os_str().eq_ignore_ascii_case(oss)) { | |
| // Case-insensitive match | |
| Ok(entries.swap_remove(i)) | |
| // Prefer exact match, then case-insensitive match, then unique inode match | |
| if let Some(i) = entries.iter().position(|p| p == path) { | |
| // Exact match | |
| Ok(entries[i].clone()) | |
| } else if let Some(i) = entries.iter().position(|p| p.as_os_str().eq_ignore_ascii_case(oss)) { | |
| // Case-insensitive match | |
| Ok(entries[i].clone()) | |
| } else if entries.len() == 1 { | |
| // Only one entry with matching inode | |
| Ok(entries[0].clone()) |
Copilot uses AI. Check for mistakes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.