search

gregsexton / gitv

gitk for Vim.
938 stars 59 forks source link

Gitv fails to open browser with shellslash on Windows #80

Closed idbrii closed 9 years ago

idbrii commented 10 years ago

I have 'shellslash' set on Windows to make my paths compatible with cygwin (but I don't have 'shell' set to bash -- I use the default).

When I try to open :Gitv --all the right pane displays this message:

fatal: ambiguous argument ''--all'': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

It's because the shellescape call in the Gitv command wraps the whole command in single quotes. With noshellslash, it uses double quotes and works correctly. With shellslash it uses single quotes and the command parser doesn't strip the quotes properly.

I'm not sure what arguments need quoting (b1561640 introduced it but is light on details). Removing the shellescape and testing commands like :Gitv -G"[a-m]m" work on Ubuntu 14.04 and Win8.1.

idbrii commented 10 years ago

I have a branch fix-80-shellslash (idbrii/gitv@f7933db) that fixes this issue.

Removing shellescape is a simpler solution, but I don't know why it's necessary. Since I don't know why it's necessary, I also don't know if my change breaks that use case.

It's removing quotes from both ends, so it seems reasonable. (Although if you had a quoted arg at the beginning and end, then I guess it would break?)

idbrii commented 10 years ago

My above report is on git version 1.8.4.msysgit.0 (from Github for Windows), but it works on the older git version 1.7.9 (from cygwin). I'd guess that's because cygwin git actually handles the quotes like unix and Github for Windows handles them like Windows and not because of the difference in git version.

gregsexton commented 9 years ago

Ugh, this stuff was a nightmare to write and make work on both windows and *nix. I no longer remember all the details but I do recall spending a lot of time fixing this stuff when I added the range support. I'm not surprised that in your mashup of windows and nix that this breaks.

I don't think taking the shellescape out is the right way to go. From a semantic point of view, we do want those arguments escaped. I'm happy to take your change (idbrii/gitv@f7933db) as a PR. Can you add a comment explaining the context and motivation?