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

feat(defaults): use ripgrep (rg) for 'grepprg' if available #28324

Merged
merged 1 commit into from Apr 16, 2024

Conversation

LunarLambda
Copy link
Contributor

Discussed as an idea in #28296.

Changes the defaults for 'grepprg' and 'grepformat' to use ripgrep and its --vimgrep mode, if available on the system.

Points discussed in the thread:

  • Zero configuration: This PR provides a better default for an existing vim feature (:grep), which the user is free to configure themselves as needed. It adds no additional configuration settings or hooks.
  • Low to zero maintenance: The change consists of a single executable() check and two option changes. It is very unlikely that ripgrep will experience breaking changes in its CLI that would necessitate adjusting the code or adding version checking.
  • Performance: A basic benchmark (running a recursive, fixed string search on the neovim repository) shows significant performance improvements over GNU grep (few seconds vs <1 second).
  • Precedence: Many popular plugins already integrate with ripgrep for searching, and ripgrep itself provides features specifically for integrating with vim (--vimgrep format, --smart-case option). This makes it a promising candidate for a place where Neovim can match the pulse of the community.

This is my first PR to adjust core Neovim code, as such there's a few points I am seeking feedback on:

  • Is this change wanted? The discussion thread concluded that the change is promising, but may not be wanted by Neovim users.
  • Is the documentation appropriate? I Added a small note to the documentation of 'grepprg' that points the user toward ripgrep and why they may want to use it. Is it clear enough that this default only takes effect if the user makes sure that the rg binary is found by Neovim?
  • Does this need extra testing? @clason mentioned a :checkhealth test, I'm unsure what that should look like.

I'm happy to make any necessary adjustments.

@github-actions github-actions bot added the defaults issues or PRs involving changing the defaults label Apr 14, 2024
@github-actions github-actions bot requested a review from gpanders April 14, 2024 11:39
src/nvim/options.lua Outdated Show resolved Hide resolved
src/nvim/options.lua Outdated Show resolved Hide resolved
src/nvim/options.lua Outdated Show resolved Hide resolved
@clason
Copy link
Member

clason commented Apr 14, 2024

a :checkhealth test, I'm unsure what that should look like.

I would recommend a new section on "optional tools" that checks vim.fn.executable('rg') == 1 and prints OK if so and WARN if not. Bonus points for adding

  • which rg and
  • rg --version

output. (We can extend this later with curl and git and tree-sitter, once we use them.)

@LunarLambda
Copy link
Contributor Author

Have adjusted the documentation, turned off the default filtering (with a note in the documentation hinting the user they may want to re-enable it if they wish), and added a rudimentary healthcheck.

runtime/doc/options.txt Outdated Show resolved Hide resolved
@LunarLambda
Copy link
Contributor Author

I have overhauled the documentation as requested. I've kept the health check in the proposed 'External Tools' section for now.

src/nvim/options.lua Outdated Show resolved Hide resolved
@LunarLambda LunarLambda force-pushed the ripgrep branch 2 times, most recently from 2c340f6 to 8678e3c Compare April 14, 2024 17:28
@clason
Copy link
Member

clason commented Apr 15, 2024

Needs a news.txt (and probably also vim-diff.txt) entry.

@LunarLambda
Copy link
Contributor Author

LunarLambda commented Apr 15, 2024

Needs a news.txt (and probably also vim-diff.txt) entry.

For vim-diff, which section? nvim-defaults? Is a single line calling out grepprg sufficient?

For news.txt, is this news-breaking or news-changed (or news-features, even) ? Is there a specific ordering to those sections or should it simply go at the end?

@clason
Copy link
Member

clason commented Apr 15, 2024

For vim-diff, which section? nvim-defaults? Is a single line calling out grepprg sufficient?

I'd say nvim-defaults

For news.txt, is this news-breaking or news-changed (or news-features, even) ? Is there a specific ordering to those sections or should it simply go at the end?

I'd say to the "Defaults:" list under news-changed (at the end).

@LunarLambda
Copy link
Contributor Author

Added. For the sake of brevity I did not include the full default value, since both places link to the option where it's documented.

runtime/doc/news.txt Outdated Show resolved Hide resolved
@clason clason merged commit 20b3867 into neovim:master Apr 16, 2024
29 checks passed
@github-actions github-actions bot removed the request for review from gpanders April 16, 2024 17:21
@LunarLambda
Copy link
Contributor Author

Happy to have landed this! Thank you for the feedback and advice.

@dundargoc
Copy link
Member

Thanks for this! The PR description and the context is useful, be sure to add that to the commit message as well next time :)

@LunarLambda LunarLambda deleted the ripgrep branch April 17, 2024 13:49
if vim.fn.executable('rg') == 1 then
-- Match :grep default, otherwise rg searches cwd by default
-- Use -uuu to make ripgrep not do its default filtering
vim.o.grepprg = 'rg --vimgrep -uuu $* ' .. (vim.fn.has('unix') == 1 and '/dev/null' or 'nul')
Copy link
Member

Choose a reason for hiding this comment

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

Apologies for the late review, but what is the rationale for -uuu rather than -uu? Is this strictly for compatibility with grep (which also searches binary files by default)?

Omitting binary file matches strikes me as a better default for a text editor. If the concern is consistent behavior for both rg and grep, I'd suggest adding -I to the default grepprg to ignore binary file matches in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes rg -uuu matches grep -r. I do agree that omitting binary files would be better. The documentation at least points out that the user may wish to adjust the flags used

Copy link
Member

Choose a reason for hiding this comment

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

The use of /dev/null here is also unnecessary AFAIK. It's used for grep because some versions of grep (e.g. BSD grep) will try to read from stdin if no file argument is given, so a plain grep foo will block indefinitely and the /dev/null argument prevents that.

But ripgrep does not behave that way, and adding /dev/null here means you cannot use :grep foo to search for foo recursively (since that expands to rg -uuu foo /dev/null, which of course returns nothing).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ripgrep searches the current directory by default. given it's also recursive by default, the /dev/null placeholder is to be both consistent with regular grep, and to prevent an accidental grep without files from potentially spinning up a lengthy process.

@LunarLambda
Copy link
Contributor Author

Given the feedback by @gpanders, is it worthwhile to revisit this before 0.10? Either when #23235 is closer to completion, or I could do it this weekend. The change is still very young, so I don't think many people have seen or given feedback on it yet, but I do think the suggestions are worth considering.

@clason
Copy link
Member

clason commented Apr 26, 2024

Sure, can still be changed (but also in a 0.10.x, so not a blocker either way).

LunarLambda added a commit to LunarLambda/neovim that referenced this pull request Apr 28, 2024
Based on feedback from neovim#28324, pass -H and -I to regular grep
(available on all platforms officially supported by Neovim), and
only pass -uu to ripgrep. This makes :grep ignore binary files by
default in both cases.
justinmk pushed a commit that referenced this pull request Apr 28, 2024
Based on feedback from #28324, pass -H and -I to regular grep
(available on all platforms officially supported by Neovim), and
only pass -uu to ripgrep. This makes :grep ignore binary files by
default in both cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defaults issues or PRs involving changing the defaults
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants