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

Improvements to ripgrep scanner #19444

Merged
merged 7 commits into from Jun 6, 2019

Conversation

Projects
None yet
2 participants
@rafeca
Copy link
Contributor

commented Jun 4, 2019

This PR includes support for a few options that were ignored on the initial PR that added support for using ripgrep as the scan() backend.

List of options:

  • excludeVcsIgnores: Whether or not to ignore files/folders that are ignored by git.
  • follow: Whether or not to follow symlinks.
  • includeHidden: Whether or not to search on hidden files (workspace.scan() hardcodes this to true though).

We did not catch this because there was no test checking them, so I've added tests for each of them to ensure that we don't add regressions in the future.

Missing thing

I've noticed that the scandal scanner is able to search on a subfolder of a vcs ignored folder even when excludeVcsIgnores is enabled, so if we have (with node_modules being ignored by git):

node_modules/
  left-pad/
    index.js
  1. If the user performs a normal search, neither scandal or ripgrep will search on the node_modules/left-pad/index.js file.
  2. If the user performs a search on the node_modules folder, scandal and ripgrep will search on the node_modules/left-pad/index.js file.
  3. If the user performs a search on the node_modules/left-pad folder, scandal will search on the node_modules/left-pad/index.js file but ripgrep won't.

I'll check how can we fix the scenario 3 (it may require bigger changes) and send a follow-up PR.

@rafeca rafeca force-pushed the improvements-to-ripgrep-scanner branch from 7b98dd1 to 95adde3 Jun 5, 2019

@rafeca rafeca requested review from nathansobo and as-cii Jun 5, 2019

@rafeca rafeca self-assigned this Jun 5, 2019

@rafeca rafeca force-pushed the improvements-to-ripgrep-scanner branch from ecfd35f to 94df548 Jun 5, 2019

Remove logic to prepend wildcard on globs
That logic was only needed to make `ripgrep` match correctly globs like
`src` when we pass it the folder to search on.

If we don't pass the folder, `ripgrep` assumes it's the cwd and their
glob matching logic improves by allowing globs like `src`.

@rafeca rafeca force-pushed the improvements-to-ripgrep-scanner branch from 94df548 to 8c80d13 Jun 5, 2019

Use '.' as the directory for ripgrep to scan
This misteriously solves issues in Windows.
@nathansobo

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

If the user performs a search on the node_modules/left-pad folder, scandal will search on the node_modules/left-pad/index.js file but ripgrep won't.

This is probably related to the fact that we search within gitignored folders any time an explicit path is specified. That behavior is actually really annoying. The perfect fix would be to detect when the user was explicitly specifying a gitignored folder, but is probably more work than whatever Scandal is doing.

@nathansobo
Copy link
Contributor

left a comment

Nice work adding test coverage.

Remove test that checks that we can search of a subfolder of an ignor…
…ed folder

This seems to not be working on Windows or with ripgrep, so it's not
expected behaviour.
@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

This is probably related to the fact that we search within gitignored folders any time an explicit path is specified. That behavior is actually really annoying. The perfect fix would be to detect when the user was explicitly specifying a gitignored folder, but is probably more work than whatever Scandal is doing.

There were CI failures on this PR for Windows, caused by the test that I added for this specific behaviour.

After testing it locally, it seems that even the behaviour from scandal is inconsistent across platforms: while the search works in OSX, it does not seem to work on Windows, so I've removed the test and I'm not going to "fix" this at least for now in ripgrep.

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Seems like current failures are due to flaky tests, so I'm gonna merge this one

@rafeca rafeca merged commit 33f2bd3 into master Jun 6, 2019

0 of 2 checks passed

Atom Pull Requests #20190606.11 failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details

@rafeca rafeca deleted the improvements-to-ripgrep-scanner branch Jun 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.