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

Make sure that we don't pipe stdin when starting a job #250

Merged
merged 2 commits into from
Aug 20, 2021

Conversation

ddeville
Copy link
Contributor

@ddeville ddeville commented Aug 19, 2021

Starting with version 13, ripgrep always stats stdin and if it's not a TTY it uses it to read data. Unfortunately, Neovim always attaches a pipe to stdin by default and that leads to ripgrep reading nothing and it essentially breaks vim-grepper when targeting a recent version of ripgrep.

(see BurntSushi/ripgrep#1892 for more info)

This was fixed in nvim by adding an option to jobstart to not pipe stdin (see neovim/neovim#14812). So we use this here which I verified fixes search through rg.

Note that I'm not 100% sure how to gate this. It was technically added as a commit to neovim after 0.5 shipped so I imagine it will technically be released in 0.5.1. But it should also be harmless to only gate this to nvim since the options are a dictionary and old versions of neovim will just ignore that stdin key.

This fixes #244

Starting with version 13, ripgrep always stats stdin and if it's not a
TTY it uses it to read data. Unfortunately, Neovim always attaches a
pipe to stdin by default and that leads to ripgrep reading nothing and
it essentially breaks vim-grepper when targeting a recent version of
ripgrep.

(see neovim/neovim#14812 for more info)

This was fixed in nvim by adding an option to jobstart to not pipe stdin
(see neovim/neovim#14812). So we use this here
which I verified fixes search through rg.

Note that I'm not 100% sure how to gate this. It was technically added
as a commit to neovim after 0.5 shipped so I imagine it will technically
be released in 0.5.1. But it should also be harmless to only gate this
to `nvim` since the options are a dictionary and old versions of neovim
will just ignore that `stdin` key.
Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

Confirmed to work with updated Neovim master.

Thanks!

plugin/grepper.vim Outdated Show resolved Hide resolved
@blueyed
Copy link
Contributor

blueyed commented Aug 20, 2021

Great, thanks again.
Let's wait for @mhinz to approve/merge it (I cannot).

@lbonn
Copy link
Collaborator

lbonn commented Aug 20, 2021

I have merge rights, just one question, will if has('nvim-0.5.1') also work with later versions?

@ddeville
Copy link
Contributor Author

Yes it will. I’m running the nightly 0.6 version and it works fine.

@lbonn lbonn merged commit 1c3c4c6 into mhinz:master Aug 20, 2021
ddeville added a commit to ddeville/scripts that referenced this pull request Aug 20, 2021
ddeville added a commit to ddeville/scripts that referenced this pull request Jun 4, 2023
ddeville added a commit to ddeville/scripts that referenced this pull request Jun 4, 2023
ddeville added a commit to ddeville/scripts that referenced this pull request Jun 4, 2023
lbonn added a commit that referenced this pull request Feb 4, 2024
This should not be a problem after #250

Fixes #258
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.

Always No results with ripgrep 13.0
3 participants