andy-5 / wslgit

Use Git installed in Bash on Windows/Windows Subsystem for Linux (WSL) from Windows and Visual Studio Code (VSCode)
MIT License
1.18k stars 59 forks source link

parentheses and pipes in parameters cause failures #54

Closed diablodale closed 5 years ago

diablodale commented 5 years ago

parenthesis )( and pipe | can not be used in arguments to wslgit. It fails to run git.

A bug seen in gitlens has exposed the need for richer shell escaping in wslgit. The parenthesis )( and pipe | should be supported as text and not as command processor special characters. The related GitLens bug is https://github.com/eamodio/vscode-gitlens/issues/526

Setup

Repo

  1. Setup wsl with code from above repo commit
  2. You can use the basic setup or the advanced BASH_ENV setup. Both reproduce the problem
  3. In CMD, change directory to a valid git repo
  4. wslgit.exe config --get-regex "user.(name|email)"

Expected

user.name Dale Phurrough
user.email dale@hidale.com

Actual Result

Basic setup

bash: syntax error near unexpected token `('

Advanced BASH_ENV setup

/bin/bash: -c: line 0: syntax error near unexpected token `('
/bin/bash: -c: line 0: `cd /mnt/n/opsworkspace/zfs-auto-snapshot && git config --get-regex user.(name|email)'

Workaround/fix

You can force it to work by escaping as follows. The quotes get it past the CMD interpreter, then the backslashes work in BASH. Naturally, this is messy and should be automated in code.

wslgit.exe config --get-regex "user.\(name\|email\)"
andy-5 commented 5 years ago

Yeah, I've expected something like this, but never run into a problem myself, so thanks for bringing this up.

Interestingly, directly running

wsl git config --get-regex "user.(name|email)"

from CMD works without escaping. But using wslgit, this does not work, as the regex part will not be quoted (only arguments with spaces are quoted, and as of the latest commit, only for an interactive shell).

So, the possible solutions would be to either escape these special characters, or to properly quote the arguments (without re-introducing the problems from #46).

diablodale commented 5 years ago

I worked part of a day on an escaping fix but didn't like the direction/results. I started with a cascade of replace(), then optimized to a Regex replace_all(). But... it didn't work across normal/advanced usage and in all my test cases (though did in more than half).

There's something both the Windows and WSL interpreters are doing with parsing that I haven't yet identified. In some cases I needed to use the blackslash and in others I didn't. Only the parameter string would change. For example, I couldn't get my ideas to simultaneously work with both these cases with both normal/adv usage. These both work when used with windows git.

wslgit.exe config --get-regex "user.(name|email|`\$&)"
wslgit.exe for-each-ref "--format=%(refname) %(objectname)" --sort -committerdate

While researching, I found https://github.com/sfackler/shell-escape though have not experimented with it.

andy-5 commented 5 years ago

That crate looks useful. As far as I can see, it quotes every string containing special characters using single quotes ' (so shells shouldn't interpret the content) and escapes all single quotes inside the string.

It is probably a good idea to use that, instead of trying to come up with another solution. Also, that crate seems to be widely used, as it is a dependency of cargo itself.

Different behavior between using bash vs. wsl directly could complicate this, as you experienced. Maybe we should just drop the functionality for using an interactive bash shell in a future release and provide some recommendation for setting up tools like ssh-agent or gpg-agent. At least according to the reported issues, many seem to use the non-interactive mode anyway.

andy-5 commented 5 years ago

With #74 merged, your initial example now works correctly for both interactive and non-interactive settings.

But your examples from https://github.com/andy-5/wslgit/issues/54#issuecomment-430574324 only work with the non-interactive shell setting. Using an interactive bash shell, they still fail.

I'll keep this issue open for the interactive shell use case.

carlolars commented 5 years ago

After I posted PR #74 I started to click around trying git-stuff in VSCode and also in Fork (where I've replaced their own git.exe with a renamed wslgit) and I found some stuff that was not working.

But now I think I have solved it, or at least it works for me, both VSCode's git and gitlense, and Fork.
I'll make a PR so you can review and try it if you'd like.

Basically, instead of quoting I append a single space-character to arguments that contain invalid characters. A space character in an argument seems to make it become correctly quoted on the way somewhere, unfortunately I don't know where.

andy-5 commented 5 years ago

That's an interesting and helpful observation. Is that maybe a feature of wsl.exe? Have you tested it for both the interactive and the non-interactive shell configurations?

Anyway, thank you for testing this and creating a PR, I'll take a look at it when I have some time.

andy-5 commented 5 years ago

With the unified quoting for interactive/non-interactive shells from #76, everything works now for both configurations, except the example with the ` character (which works without the `):

wslgit.exe config --get-regex "user.(name|email|`\$&)"

But as commented on the PR, this example does not work on Linux/WSL either, so I think it is not really a problem. If you do find a real example where this is a problem, please file a new issue.