Skip to content

fix: show path-separator error for any pattern containing a separator#1973

Closed
SAY-5 wants to merge 2 commits intosharkdp:masterfrom
SAY-5:fix-path-separator-error-consistency
Closed

fix: show path-separator error for any pattern containing a separator#1973
SAY-5 wants to merge 2 commits intosharkdp:masterfrom
SAY-5:fix-path-separator-error-consistency

Conversation

@SAY-5
Copy link
Copy Markdown
Contributor

@SAY-5 SAY-5 commented Apr 17, 2026

Fixes #1873.

Problem

`fd pattern` with a path-separator character in the pattern silently returns zero results most of the time. The user only sees the helpful "pattern contains a path-separation character" error when the pattern also happens to match an existing directory on disk:

$ fd /        # pattern happens to be a real directory → error fires
$ fd 'src/foo'  # not an existing directory → silent zero results

That's because `ensure_search_pattern_is_not_a_path` in `src/main.rs` checks both:

if !opts.full_path
    && opts.pattern.contains(std::path::MAIN_SEPARATOR)
    && Path::new(&opts.pattern).is_dir()

Fix

Drop the `is_dir()` check. By fd's design, the search is file-name based unless `--full-path` is set, so a pattern containing a separator will never match anything. The error should always fire.

-    if !opts.full_path
-        && opts.pattern.contains(std::path::MAIN_SEPARATOR)
-        && Path::new(&opts.pattern).is_dir()
-    {
+    if !opts.full_path && opts.pattern.contains(std::path::MAIN_SEPARATOR) {

The existing error message already generalizes: it suggests `fd . 'pattern'` or `fd --full-path 'pattern'`, both of which make sense whether the pattern is a real directory or not.

Behavior change

Before: silent zero results for patterns with separators unless the pattern happens to be a real directory.
After: consistent helpful error for any such pattern, pointing the user to `--full-path` or a corrected match-all invocation.

This is the change the issue author explicitly asked for ("I would expect the error about how a pattern with a '/' in it will not lead to any search results to be displayed regardless of the pattern possibly being a directory path").

SAY-5 and others added 2 commits April 16, 2026 22:56
Fixes sharkdp#1873.

`ensure_search_pattern_is_not_a_path` previously only fired when the
pattern both (a) contained a path-separation character AND (b) happened
to match an existing directory on disk. That meant users who typed a
pattern with a slash got the helpful error only by coincidence — any
pattern with a separator that didnt also happen to be a real directory
returned zero results silently.

The separator check alone is sufficient: fd's default search is
file-name based, so any pattern containing a separator will never match.
Remove the extra `Path::new(&opts.pattern).is_dir()` condition so the
error is consistent for every pattern with a separator.

No test change needed; the existing error text still makes sense for
non-directory patterns (the advice to use `fd . 'pattern'` or
`fd --full-path 'pattern'` is the same either way).

Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
MAIN_SEPARATOR is '\\' on Windows, which is also a regex escape
character. Treating '\\' as a path-separator produced false positives
for valid regex patterns such as '\\Ac', '\\d+', etc., breaking the
test_smart_case integration test on windows-2025.

Restrict the check to '/', which is always a path separator (including
on Windows) and has no regex meaning. This still fixes sharkdp#1873 on all
platforms without breaking regex patterns on Windows.

Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented Apr 17, 2026

Pushed a follow-up (8c9f814) that restricts the path-separator check to '/'.

The CI failure on windows-*-msvc / windows-gnu was caused by me using std::path::MAIN_SEPARATOR, which is '\' on Windows. That collides with regex escape characters — test_smart_case uses \\Ac as a regex anchor pattern, and my check was incorrectly rejecting it. The original issue #1873 is specifically about '/', which is always a path separator (including on Windows) and has no regex meaning, so checking only '/' fixes the bug on every platform without breaking Windows regex patterns.

@tmccombs tmccombs closed this Apr 17, 2026
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented Apr 17, 2026

Hi @tmccombs — I noticed this was closed without comment. Was the approach wrong, or is there something I can adjust? Happy to rework if you'd prefer a different approach to fixing #1873. Apologies if the initial breaking of Windows CI caused frustration — the follow-up (8c9f814) limits the check to '/' only, which sidesteps the regex-escape collision entirely.

@tmccombs
Copy link
Copy Markdown
Collaborator

There have been many PRs created for this already that do basically the same thing, and from the style of the comments, I'm suspicious this is AI generated.

@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented Apr 17, 2026

I am doing it manually, sorry if it felt that way @tmccombs

@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented Apr 17, 2026

Understood, @tmccombs — thanks for the context on the existing PRs. I'll leave it at that. Appreciate you looking.

@tmccombs
Copy link
Copy Markdown
Collaborator

If you want to make this mergeable, the issues with it are:

  1. On windows, it will no longer print the warning if you use a pattern that is an actual directory. Maybe we're ok with losing that, but it warrants some discussion, and my initial thought is that is behavior we probably want to keep.
  2. We should add tests for the new behavior
  3. You'll need an entry in the changelog, although you can wait to do that until it looks like the rest is ready to be merged.

@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented Apr 17, 2026

Hi @tmccombs — followed up on your three points. Force-pushed onto the same branch:

  1. Preserved the Windows is_dir() behaviour. The new check is:

    let has_forward_slash = pattern.contains('/');
    let has_windows_dir_backslash =
        cfg!(windows) && pattern.contains('\\') && Path::new(pattern).is_dir();
    if has_forward_slash || has_windows_dir_backslash { ... }

    So / always warns (the original issue [BUG] The 'pattern contains path separator' error is only shown when pattern is an existing directory #1873), and on Windows a backslash pattern that also resolves to a real directory still warns the way it did before. Regex patterns like \Ac or \d+ no longer hit the check since / is not present and the pattern is not a directory.

  2. Added three integration tests in tests/tests.rs:

    • test_pattern_forward_slash_always_errors - asserts the warning fires for src/foo and /.
    • test_pattern_full_path_allows_separator - --full-path 'one/two' succeeds.
    • test_pattern_regex_anchor_not_flagged - \Ac runs as a regex anchor without triggering the warning.
  3. Added a CHANGELOG entry under Unreleased / Bugfixes.

Would appreciate a reopen so CI can run on the updated commit. Happy to further adjust the Windows check (e.g. also include forward-slash-free but backslash-and-path patterns) if you'd prefer different coverage there.

@tmccombs
Copy link
Copy Markdown
Collaborator

Thanks you. Looks like I can't reopen because there was a force push, but if you open a new PR, I can look at it. Sorry.

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.

[BUG] The 'pattern contains path separator' error is only shown when pattern is an existing directory

2 participants