-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix: Show path separator error regardless of directory existence #1876
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -147,25 +147,69 @@ fn set_working_dir(opts: &Opts) -> Result<()> { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Detect if the user accidentally supplied a path instead of a search pattern | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn ensure_search_pattern_is_not_a_path(opts: &Opts) -> Result<()> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !opts.full_path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && opts.pattern.contains(std::path::MAIN_SEPARATOR) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && Path::new(&opts.pattern).is_dir() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Err(anyhow!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "The search pattern '{pattern}' contains a path-separation character ('{sep}') \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and will not lead to any search results.\n\n\ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| If you want to search for all files inside the '{pattern}' directory, use a match-all pattern:\n\n \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fd . '{pattern}'\n\n\ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Instead, if you want your pattern to match the full file path, use:\n\n \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fd --full-path '{pattern}'", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pattern = &opts.pattern, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sep = std::path::MAIN_SEPARATOR, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !opts.full_path && opts.pattern.contains(std::path::MAIN_SEPARATOR) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // On Windows, backslash is both a path separator and a regex escape character. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // We need to distinguish between paths (e.g., "C:\path" or "\nonexistent") and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // regex patterns (e.g., "\Ac" where \A is a regex anchor). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // A simple heuristic: if the pattern looks like it could be a path (not just | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // a single-character regex escape), show the error. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let looks_like_path = if cfg!(windows) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // On Windows, check if it's a drive path (C:\) or if the backslash is | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // followed by something that looks like a path component (not a single regex escape) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let is_drive_path = opts.pattern.len() >= 3 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && opts | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .pattern | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .chars() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .next() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .is_some_and(|c| c.is_ascii_alphabetic()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && opts.pattern.chars().nth(1) == Some(':') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && opts.pattern.chars().nth(2) == Some(std::path::MAIN_SEPARATOR); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| is_drive_path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| || (opts.pattern.matches(std::path::MAIN_SEPARATOR).count() > 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && !is_likely_regex_escape(&opts.pattern)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // On Unix, if it starts with / or contains /, it's likely a path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+156
to
+173
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
So that on non-windows we don't even have to compile the code for the path that we'll never encounter. It would be nice if we could avoid the if statement below as well, but I'm not sure how to do that in a clean way without repeating the errror. Maybe if we factored the error message out into a const? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if looks_like_path { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Err(anyhow!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "The search pattern '{pattern}' contains a path-separation character ('{sep}') \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and will not lead to any search results.\n\n\ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| If you want to search for all files inside the '{pattern}' directory, use a match-all pattern:\n\n \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fd . '{pattern}'\n\n\ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Instead, if you want your pattern to match the full file path, use:\n\n \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fd --full-path '{pattern}'", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pattern = &opts.pattern, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sep = std::path::MAIN_SEPARATOR, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Check if a pattern is likely a regex escape sequence rather than a path. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// This is a heuristic to avoid false positives on Windows where \ is both | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// a path separator and a regex escape character. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn is_likely_regex_escape(pattern: &str) -> bool { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !cfg!(windows) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. get rid of this, and use conditional compilation on the entire function |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Common regex escape sequences: \A, \z, \b, \d, \s, \w, \1, \2, etc. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If the pattern is very short (like "\Ac") and starts with \ followed by | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // a letter or digit, it's likely a regex escape. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if pattern.len() <= 3 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This heuristic will have a lot of false positives. This will prevent you from using escapes in a regex for anything longer than 3 characters on windows. I'm not really sure what a good path forward is for windows. Honestly, our best option might be to continue using the same heuristic we had before (checking if the path corresponds to a directory). Or maybe just check if the first path component is a directory? Maybe we could also look for "/" on windows, since that can be a path separator as well. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && pattern.starts_with('\\') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && let Some(ch) = pattern.chars().nth(1) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ch.is_ascii_alphanumeric(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn build_pattern_regex(pattern: &str, opts: &Opts) -> Result<String> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(if opts.glob && !pattern.is_empty() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let glob = GlobBuilder::new(pattern).literal_separator(true).build()?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.