search

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

Quoting of argument is not working correctly when adding wslpath #91

Closed carlolars closed 4 years ago

carlolars commented 4 years ago

When injecting wslpath in an argument then the argument to wslpath is quoted with single-quotes, this cause the argument be considered as "already quoted" at https://github.com/andy-5/wslgit/blob/3dd36bd198419ce33c67a02e3d131bc33d5c920a/src/main.rs#L132 and no quotes are added around the argument even if actually needed. Simple test below:

>wslgit.exe -c "test.value=C:\Program Files (x86)\SmartGit" config test.value
bash: -c: line 0: syntax error near unexpected token `('
bash: -c: line 0: `git -c test.value=/c/Program Files (x86)/SmartGit config test.value'
# missing quotes!         ^                                        ^

The quoting of arguments must become smarter, maybe ignore the quotes found in $(wslpath ...)

carlolars commented 4 years ago

When are quotes actually used?
Quoting is used on the command line when an argument string contains for example spaces to prevent the shell from splitting the string in several arguments. The shell (cmd.exe or PowerShell) then passes the argument string(s) to the process as an array of strings without the quotes.
When invoking a process directly without using a shell then the arguments are passed as an array of strings and no quoting is required.

So wslgit.exe should never get arguments that contain quotes, unless someone really wants a literal " and pass them using \", if so they should be passed to the WSL side correctly escaped.

@andy-5 What do you think? I have implemented a possible fix for this, I'll push it for review.

andy-5 commented 4 years ago

I think your line of thought is correct. If there are quotes in arguments, they are explicitly added as you described, so wslgit should escape them again. Your PR looks fine, though I haven't actually tested it. I guess it fixes the example given above? Can we close this issue, or should we wait and test it for longer before closing?

carlolars commented 4 years ago

Yes the PR fixes the example above.
I've used a build with this fix for over a week at work and home without any errors so I don't think that my usage pattern will be able to find any errors.