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

Grepper isn't properly quoting search queries on Windows #149

Open
cheathcott opened this issue Feb 10, 2018 · 12 comments
Open

Grepper isn't properly quoting search queries on Windows #149

cheathcott opened this issue Feb 10, 2018 · 12 comments

Comments

@cheathcott
Copy link

In my .vimrc, the only configuration related to Grepper is the following two mappings:

nmap gs <plug>(GrepperOperator)
xmap gs <plug>(GrepperOperator)

Grepper chooses ag by default, and I'll use ag in my examples below, though I've also tried swapping ag for rg with the same results.

I created a very simple test case of a single folder named "test" whose contents are the single file named "test.txt" whose contents are the single line "test1 test2". In all further discussion, this is my present working directory and only buffer.

In Windows, if I position the cursor on column 1 and type "gs2w" (grep search two words), or if I visually select "test1 test2" and then type "gs" (grep search visual selection), then the following error is generated:

|| ERR: Error opening directory test2: No such file or directory

Strangely, the command displayed in the status line does appear to be properly (double) quoted:

[Quickfix List] ag --vimgrep -- "test1 test2"

That same command returns the expected result when typed directly at the Windows shell.

However, the error in Vim certainly indicates that ag as called by Grepper is treating the second word after the space as a path argument rather than part of the search pattern.

When I do the exact same experiments in macOS, the Grepper search is successful and no error is generated:

test.txt|1 col 1| test1 test2

Here the command displayed in the status bar is similar to Windows, though the quotes are single rather than double, which makes sense since I'm using Bash on macOS:

[Quickfix List] ag --vimgrep -- 'test1 test2'

I also did some experimentation using the Grepper prompt instead of the operator.

In macOS, I can search at the Grepper prompt for 'test1 test2' or "test1 test2" (single or double quotes) and I'll get the same successful result. This makes sense because there aren't any special characters which need to be escaped and Bash can handle both types of quotes.

However, in Windows, only the Grepper prompt search for 'test1 test2' returns a successful result, whereas "test1 test2" returns the same error as when trying to use the operator.

To me, this is particularly odd behavior since the Windows shell is only supposed to work with the double quotes rather than the single. Using single quotes directly at the Windows shell generates the same error as using double quotes at the Grepper prompt.

Hence, Grepper appears to be sending to the Windows shell the opposite quotes of what is being displayed on the Vim status line.

Here's my environment:

Windows 10 Enterprise 16299.192
vim 8.0.1473 (via Chocolatey)
ag 2.1.0 (via Chocolatey)
rg 0.7.1 (via Chocolatey)
vim-grepper 1.4 (via vim-plug)

macoS 10.13.2
vim 8.0.1450 (via Homebrew)
ag 2.1.0 (via Homebrew)
rg 0.7.1 (via Homebrew)
vim-grepper 1.4 (via vim-plug)

Let me know if there is anything else I can provide that would help debug. Cheers.

@mhinz
Copy link
Owner

mhinz commented Feb 12, 2018

Hey,

I'm sorry for all these Windows bugs. Since I left uni I don't even have access to Windows anymore and I'm pondering to declare this plugin to only have experimental Windows support.

I simply don't have the capacities to do it myself right now. If any Windows person was willed to dive into the code, I'd grant them commit rights immediately.

But until then it will probably take a while to get this fixed. Sorry for the inconveniences. 🙏

@cheathcott
Copy link
Author

cheathcott commented Feb 20, 2018

I'm stuck on Windows for work, and I want to fix this plugin, so I'm diving into the code
now. However, I don't have prior experience with Vimscript or the Vim debugger, so it will take me a while to grok. Also, paths, quoting, and escaping on Windows shell is just ridiculous compared to any other *nix shell, so there is also a possibility of my head exploding before a pull request.

Anyhow, as the plugin is currently written for Windows, the command is actually being passed along to powershell.exe through cmd.exe, and Powershell is stripping out the double quotes.

Thus, I tried forcing the use of cmd.exe, and this works for my example above, but it breaks when used with the -buffer and -buffers flags because single quotes are used for escaping those.

Digging in further, it appears shellslash is being set to true by the plugin, which causes the shellescape function to use single quotes rather than double quotes, which is inappropriate for Windows.

I tried commenting out all the places where shellslash was being set to true along with forcing the use of cmd.exe, and so far, this is working as expected with some basic examples.

I will continue to test this approach and report my results here.

In case anyone else is interested, this document seems to be the best guide out there regarding Vim shell escaping:

https://github.com/airblade/vim-system-escape

@mhinz
Copy link
Owner

mhinz commented Feb 22, 2018

Thanks for all your input so far. I finally dusted off an old Win 10 VM.

Let's start with a fundamental question.. is there any need to ever use Powershell? Is there any downside to using cmd.exe directly even when there's Powershell available?

I play around with the code a bit and try to pay special attention to..

  • 'shellslash'
  • quoting:
    • paths with spaces
    • / and \ as path separators are handled the same
    • does -buffers still work with multiple buffers
  • does findstr.exe still work? (It's a much simpler tool than the other supported grep tools)

@mhinz
Copy link
Owner

mhinz commented Feb 22, 2018

@cheathcott

It would be nice if you could test the improve-win-support branch. The code is rather simple, but it seems to work for my arguably stupid test cases.

Would be good to know if it works for you as well and if you can figure out any corner cases that breaks it.

Ref: #151

@cheathcott
Copy link
Author

Awesome! I'll thoroughly test the improve-win-support branch this weekend and report my findings.

Incidentally, with regard to the experiment I described earlier, I also ran into the problem with needing to escape backslash path separators. I tried decomposing the map which does both shellescape and fnamemodify into two steps.

In the first step I left shellslash=1 so forward slashes were used as path separators. Then I set shellslash=0 so double quotes were used for the shellescape.

Anyhow, the way you handled it by setting shellslash=0 first and then simply escaping the backslash path separators before doing the shellescape seems like the better approach.

@mhinz
Copy link
Owner

mhinz commented Mar 1, 2018

Now that the changes are merged onto master.. any things left to fix?

@mhinz mhinz added solved? and removed help wanted labels Mar 1, 2018
@cheathcott
Copy link
Author

I apologize for disappearing for a bit. I'm back to testing this change, and I definitely think it's a positive improvement since the file arguments passed to the grep tools are now properly quoted with double quotes, so the buffer and buffers flag work.

However, there remain some search pattern quoting issues with the operator because of cmd.exe's ridiculous quoting rules.

For example, if using the operator to search for a pattern which ends with a literal backslash, then my understanding is that it has to be escaped with four backslashes rather than only two backslashes like when it appears in the middle of the pattern.

Assume a file contains three lines, "a", "a\b", and "*?ab".

If you visually select "a" and use the operator, the escaped search pattern is "a\", which doesn't work.

If you visually select "a\b" and use the operator, the escaped search pattern is "a\b", which does work.

If you use the prompt to search for "a\\", then both "a" and "a\b" are found.

Also, there are other special characters, like star or question mark, for which single backslash escape does not seem to work.

If you visually select "*" and use the operator, the escaped search pattern is "\*", which does not match.

Unfortunately, I'm not even sure how to properly escape star and question mark when calling ag or rg directly from cmd.exe.

Perhaps the better approach to take with the operator is to do minimal escaping and to use the literal string flags of ag or rg instead rather than these tools assuming the pattern is a regex - which is something I think I should be doable via my grepper config.

@mhinz
Copy link
Owner

mhinz commented Mar 13, 2018

Hmmmm. I can reproduce everything you said.

For the prompt the manual escaping is fine, since you would have to do that in the shell as well. The prompt is supposed to feel like a pre-populated shell.

But the operator should always find the selected text, of course.

I found that "\*\?" matches the *? in your example. Only "\*" alone won't match *. We need to do additional escaping with ^ in that case: "^\*".

So, that's something that could be implemented for the operator, but this SO post makes me think that using cmd.exe in favour of powershell is the wrong way. Apparently we will never be able to escape " within our implicit double quotes. That's kind of a no-go.

EDIT:

"\"test\"" (or """test""") matches "test" just fine. Let me add the "*" and "foo\" corner cases and see where it heads.

@mhinz
Copy link
Owner

mhinz commented Mar 13, 2018

The foo\ case can be worked around by escaping the \ twice. So it will be found by foo\\\\. foo\\ will be found by foo\\\\\\\\\ and so on.

But I haven't found any way to escape * or ? without using ag -Q. Neither with cmd.exe nor Powershell.

@mhinz mhinz removed the solved? label Mar 20, 2018
@blueyed
Copy link
Contributor

blueyed commented May 2, 2018

Just for reference: quoting could be avoided / forwarded to Neovim/Vim by not wrapping it manually in a shell (cmd.exe) probably. See #166 about using Python's shlex.split to split a string into argv.

@janlazo
Copy link

janlazo commented Nov 24, 2018

I suggest writing all commands to a temporary batchfile like what I did https://github.com/junegunn/fzf/blob/master/plugin/fzf.vim. It works for the grep and ag commands in https://github.com/junegunn/fzf.vim.

If not, use /s and wrap the entire command with double quotes so that cmd.exe dequotes " and runs the dequoted command as is. Check cmd.exe defaults for Neovim and the functional tests for system and jobs.

@mhinz
Copy link
Owner

mhinz commented Nov 24, 2018

I don't work at Windows-related issues, sorry. I'll review small PRs that are VimL-only, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants