fix: flag any pattern containing a path separator, not just ones naming a directory#1975
Conversation
e65a6b6 to
1ab45cb
Compare
tmccombs
left a comment
There was a problem hiding this comment.
This seems pretty good to me. Just need to fix the failing tests.
Although I wouldn't mind a second opinion on if this is a good way of addressing the problem.
| && opts.pattern.contains(std::path::MAIN_SEPARATOR) | ||
| && Path::new(&opts.pattern).is_dir() | ||
| { | ||
| && Path::new(&opts.pattern).is_dir(); |
There was a problem hiding this comment.
Minor performance note. If has_forward_slash is true, it would probably be a little more efficient to avoid checking if the path is a directory, since we already know it will be an error.
| // Pattern that is NOT a real directory; old behaviour: no warning. | ||
| te.assert_failure_with_error( | ||
| &["nonexistent/path"], | ||
| "[fd error]: The search pattern 'nonexistent/path' contains a path-separation character", |
There was a problem hiding this comment.
| "[fd error]: The search pattern 'nonexistent/path' contains a path-separation character", | |
| "[fd error]: The search pattern 'nonexistent/path' contains a path-separation character and will not lead to any search results.", |
I think why there are failed tests is because the way assert_failure_with_error work is it (possibly unintentionally) adds a "\n" at the end of the expected string, which doesn't match if you don't have the full line.
Although I'm confused why the test is currently succeeding on some platforms.
| let has_forward_slash = opts.pattern.contains('/'); | ||
| let looks_like_existing_windows_dir = cfg!(windows) | ||
| && opts.pattern.contains(std::path::MAIN_SEPARATOR) | ||
| && Path::new(&opts.pattern).is_dir() | ||
| { | ||
| && Path::new(&opts.pattern).is_dir(); | ||
|
|
||
| if has_forward_slash || looks_like_existing_windows_dir { |
There was a problem hiding this comment.
Could be:
| let has_forward_slash = opts.pattern.contains('/'); | |
| let looks_like_existing_windows_dir = cfg!(windows) | |
| && opts.pattern.contains(std::path::MAIN_SEPARATOR) | |
| && Path::new(&opts.pattern).is_dir() | |
| { | |
| && Path::new(&opts.pattern).is_dir(); | |
| if has_forward_slash || looks_like_existing_windows_dir { | |
| #[cfg_attr(not(windows), allow(unused_mut))] | |
| let mut should_warn = opts.pattern.contains('/'); | |
| #[cfg(windows)] | |
| should_warn = should_warn || ( | |
| opts.pattern.contains(std::path::MAIN_SEPARATOR) | |
| && Path::new(&opts.pattern).is_dir()); | |
| if should_warn{ |
… name a directory Without --full-path, fd matches patterns against file names, so any pattern containing a path separator can never match. The previous implementation in main.rs fired the path-separator diagnostic only when the pattern also named an existing directory on disk, so the most common Linux/macOS typo - pasting a full path as the pattern - silently returned zero matches. Restructure ensure_search_pattern_is_not_a_path so that any '/' in the pattern triggers the diagnostic unconditionally, and keep the existing 'directory must exist on disk' guard only for the native '\' separator on Windows, which is also the regex escape character. The Windows check is short-circuited via '||' so the is_dir syscall never runs when the pattern already contains '/'. The #[cfg_attr(not(windows), allow(unused_mut))] attribute keeps the mut binding warning-free on non-Windows targets. Update the error message to drop the now-ambiguous parenthetical that showed the platform-specific separator character, and widen the integration test assertions to cover the full first line so the assert_failure_with_error helper (which trims lines on both sides) cannot early-accept on a partial prefix. Closes sharkdp#1873 Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
1ab45cb to
ed47664
Compare
|
Thanks @tmccombs — addressed all three inline suggestions:
Verified locally on macOS:
One note: test_pattern_with_forward_slash_allowed_with_full_path fails on my macOS checkout on master too, because the default filesystem is case-insensitive and |
No. It's actually because by default So to fix that test case I think you either need to add the Although, I'm not really sure why the ARM builds are passing.... |
Fix test_pattern_with_forward_slash_allowed_with_full_path to include all expected search results.
Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com> On Windows the regex-over-full-path match uses `\`, so the pattern `one/two/c` cannot match a real entry. The diagnostic- didn't-fire behaviour this test pins is already covered by the fact that the invocation does not error. Gate it to non-Windows per tmccombs's CI feedback on the PR.
|
Thanks @tmccombs — you're right that it's smart-case rather than the filesystem itself; my mistake on the local analysis. For the Windows failure I've gated the |
|
|
||
| // On Windows we additionally accept the native `\` separator, but only when | ||
| // the pattern actually resolves to an existing directory - `\` is also the | ||
| // regex escape char there, so valid patterns like `\Ac` or `\d+` must still |
There was a problem hiding this comment.
I feel like we should be able to use regex-syntax or something to distinguish between escape sequences and literal backslashes, but this is a good start
Reopening per @tmccombs ("if you open a new PR, I can look at it" — the
previous PR #1973 force-push blocked reopening). Addresses the three
review items raised there:
fired only when the pattern contained
MAIN_SEPARATORAND the patternnamed an existing directory. This PR now fires in two cases:
/(safe on every platform because/isalways a path separator and has no regex meaning), and
\separator AND namesa real directory (kept intact from master, since
\is also theregex escape character and we can't treat it as a pure path signal).
tests/tests.rs:test_pattern_with_forward_slash_is_rejectedcovers both anon-existing path (previously silent, now errors) and an existing
directory (must still error).
test_pattern_with_forward_slash_allowed_with_full_pathguardsthe
--full-pathescape hatch.# Unreleased / ## Bugfixes, referencing[BUG] The 'pattern contains path separator' error is only shown when pattern is an existing directory #1873.
Context on #1873
Without
--full-path, fd matches the pattern against the file name,never the full path. A pattern like
src/foo.rstherefore cannot matchanything regardless of whether
src/foo.rsexists on disk. The oldcheck only fired when
Path::new(&opts.pattern).is_dir()was true, sothe common typo (pasting a path, or using a non-existing directory
prefix) silently returned zero results:
After this change:
The
--full-pathopt-in is unchanged.Refs #1873.
cc @tmccombs