Skip to content
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

Fix ignores when searching subdirectories #2295

Closed
wants to merge 1 commit into from

Conversation

sengaya
Copy link
Contributor

@sengaya sengaya commented Aug 29, 2022

When searching subdirectories the path was not correctly build and
included duplicate parts. This fix will remove the duplicate part if
possible. It also contains a small optimization that will skip the
second for loop if there is already a match.

Fixes #1757

When searching subdirectories the path was not correctly build and
included duplicate parts. This fix will remove the duplicate part if
possible. It also contains a small optimization that will skip the
second for loop if there is already a match.

Fixes BurntSushi#1757
@BurntSushi BurntSushi added the rollup A PR that has been merged with many others in a rollup. label Sep 18, 2023
Copy link
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you! This fixes quite a gnarly bug that I've seen a lot of folks run into for a while. Thank you.

I ended up fixing two problems with this patch though:

  1. First, the optimization does not look correct to me. The issue is that, for some ignore files, the existence of any rule regardless of where it is in the directory tree will override any other rule from a different type of ignore file. For example, .rgignore has higher precedent than .ignore. So you need to let all of the rules be visited. In practice, I doubt this optimization matters too much anyway.
  2. This used Path::strip_prefix instead of ripgrep's own strip_prefix routine which does things a little differently. This meant that giving ripgrep a path like .////path would still trigger the bug. But using ripgrep's strip_prefix routine avoids this issue.

Overall this still feels Not Right to me, but it fixes a bug and doesn't seem to cause any other regressions. Hopefully we can straighten out this logic in a better way in the future when I overhaul the ignore crate.

BurntSushi pushed a commit that referenced this pull request Sep 20, 2023
When searching subdirectories the path was not correctly built and
included duplicate parts. This fix will remove the duplicate part if
possible.

Fixes #1757, Closes #2295
ink-splatters pushed a commit to ink-splatters/ripgrep that referenced this pull request Oct 25, 2023
When searching subdirectories the path was not correctly built and
included duplicate parts. This fix will remove the duplicate part if
possible.

Fixes BurntSushi#1757, Closes BurntSushi#2295
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rollup A PR that has been merged with many others in a rollup.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only some ignores applied when searching a sub-directory
2 participants