Improve s:EscapeCtagsCmd() for windows cmd.exe to eliminate issues when a:ctags_bin calls a batch file that uses %~dp0

[darcyparker]

While developing a ctags parser for XSLT, I uncovered a subtle issue with s:EscapeCtagsCmd() in tagbar that causes problems on windows (cmd.exe) machines when the a:ctags_bin is a batch file that depends on evaluation of %~dp0 in order to run correctly.

See https://gist.github.com/darcyparker/5267063 that illustrates the problem with %~dp0. Using %~dp0 is a common practice in windows because batch files are written as wrappers and the batch file needs to know where to find other files required by the script.

I traced the problem to this line.

On Windows, I don't think shellescape() should be used on
shellescape(a:ctags_bin) when the command is just a single file name and not the full path. It doesn't hurt to have it for unix-y shells, but I don't think it is needed because there aren't really any commands that have spaces or need escaping. Only the additional arguments need to be escaped. So I think it is safe to just eliminate the shellescape() on the command. (not the args)

It is good to run shellescape() on each argument (but not the whole set of arguments at once, but each argument individually). See the findings in this pull request accepted for syntasic.

Side note: Currently ctagsargs in g:tagbar_type_xxx is a string of all arguments to be passed. I haven't seen a particular problem with it today, but based on findings in syntastic pull request mentioned above, I think it would be more robust if an array of each argument was passed. Then shellescape() should be run on each argument and the results joined rather than a single shellescape() on the full argument string. Maybe this would also eliminate the need for the if block that follows about the 'stupid cmd.exe quoting'?

So based on the findings in my gist mentioned above, it seems like it is not a good practice to escape the command or 0th argument.

Does this make sense?

I can submit a pull request, but wanted to get your input first. Maybe just the issue with not escaping the command name (0th argument), should be addressed first. And the suggestion to escape each of the passed arguments separately should be handled in the future. (A solution that supports the existing data structure that uses a single string for ctagsargs and the proposed array for ctagsargs is probably something that would be better for you to think about.)

With regards to a potential pull request: It won't take long because I can confirm that let ctags_cmd = a:ctags_bin . ' ' . a:args . ' ' . fname works correctly on windows based on my testing of my xsltctags tool that uses a .bat file without the workaround mentioned in my gist. But I have not added code to test if a:ctags_bin contains things that need to be escaped. (For example a full path that includes a space.)

[janlarres]

Thanks for the detailed report and the test case! I knew there was a reason I don't use Windows ...

There were several threads on vim-dev last year about the insane quoting rules of cmd.exe, and it seems that there is not really a nice way of handling all that. The reason why I added the shellescape() for the command is that it could be that someone installed ctags for example in C:\Program Files\, which does contain a space. Testing with your script shows that there is a difference between calling a program from %PATH% and with an absolute path:

C:\Documents and Settings\Jan>getthiscommandsdir
Command and Arguments: getthiscommandsdir
Where is this command using %~dp0: C:\WINDOWS\

C:\Documents and Settings\Jan>"getthiscommandsdir"
Command and Arguments: "getthiscommandsdir"
Where is this command using %~dp0: C:\Documents and Settings\Jan\

C:\Documents and Settings\Jan>c:\windows\GetThisCommandsDir.bat
Command and Arguments: c:\windows\GetThisCommandsDir.bat
Where is this command using %~dp0: c:\WINDOWS\

C:\Documents and Settings\Jan>"c:\windows\GetThisCommandsDir.bat"
Command and Arguments: "c:\windows\GetThisCommandsDir.bat"
Where is this command using %~dp0: c:\WINDOWS\

(I removed the second part of the script since where doesn't seem to exist in my XP VM)
So the best approach seems to be to add the quotes if the variable points to an absolute path, and not add them if it's a program in %PATH%. You should be able to easily distinguish between them by checking for backslashes (and probably forward slashes, since that's legal too).

After some tests it seems like the quoting conditional is indeed not needed, which is a bit surprising after having read those vim-dev threads. Oh well. Unfortunately using shellescape() on a program that contains one of the special characters is needed, which would make your bug show up again. So there doesn't seem to be a way to get %~dp0 right in a program that contains a special character in its file name. Let's just hope people give their executables sane names ...

Your idea about the ctagsargs sounds good, too. Since you can check whether the type of a variable is a string or an array it would even be possible to change it without breaking backwards compatibility. It should probably be handled in a separate commit though, if you would like to do that yourself.

[janlarres]

After some more experimenting it seems that there is no way to execute a command that includes special characters like &, but they can appear in normal file names and then need to be enclosed in quotes or escaped with ^. Since the file names are already enclosed in quotes the the explicit escaping conditional block does seem to be unnecessary.

I also realized that this seems to be the same issue as #95, but thanks to your information we should now be able to fix it.

And just for reference or if you haven't had enough insanity today, here's the main (long) thread on vim-dev about cmd.exe's brokenness: https://groups.google.com/forum/?fromgroups=#!topic/vim_dev/vVOynF5fKlA

Now I'm just curious if this has anything to do with #41 ...

[darcyparker]

Thanks for reviewing all my notes and test case. I know it was verbose, but sometimes the extra detail helps me digest/rationalize everything...

I am glad it may help #95 and #41. And you are right - cmd.exe is poorly designed... I wished we didn't need to use it, or that MS eliminated it. But that is wishful thinking.

BTW: where.exe must be Windows 7 only. (it works like whereis on unix-y systems)

d:\>where where
C:\Windows\System32\where.exe

So that is good for me to know... I will have to find another workaround for XP in my script. (gnuwin32 whereis for example)

Thanks for the reference to the thread in the vim-dev group. As one takeaway, I liked and agreed with Bram's comment. "Using & in a command is unusual. Having the command work with spaces in the command name and argument name is much more important. But we would like to make every command work..."

It clearly won't be easy to fix all possible use cases for escaping the ctags command. But like you said, strange file names are uncommon. So supporting all cases is not as important. Supporting the realistic/reasonable cases are the priority. To help with prioritizing requirements for an improved solution, I am listing the following items that I believe are true.

(This continuous to be verbose and includes a bit of repetition.. but I hope it helps summarize our findings. As an exercise in vimscript, I'd like to attempt the fix and submit a pull request if you haven't started and you don't mind.)

Some assumed truths to help focus on the important requirements for shell escaping of ctags command

• It's a better practice to shellescape() each argument separately so that spaces used as a separator between arguments can be distinguished with spaces used inside a single argument.
  • Support both a single args string (as you do today) and an args array. The args array will provide more robustness when building the full command for tagbar to execute because shellescape() can't decipher if the space is a part of an argument, or an argument separator.
• Vim users who want configurations that work on all OSes, will likely use a single executable name like ctags and set their $PATH or %PATH% appropriately on each OS.
• A full abs path to an executable not in the %PATH% or $PATH is sometimes necessary
  • Of course the abs path may be platform or computer specific so it may not be portable without has() condition testing.
• Spaces in an actual executable file name are rare, but possible
  • Make reasonable efforts to support it, while also enabling %~dp0 to work as expected on Windows
• Spaces in a full abs path to the executable file name is more common, but an executable name in the $PATH or %PATH% is still the most common use case.
• Sometimes it is not easy or possible to decipher whether a special character should be escaped or not, so don't attempt to guess. Instead, require users to escape these characters as required for their scenario.
  • Example, consider % or $.
    • Sometimes these may be to identify an environment variable. In which case it should not be escaped. ex: %programfiles%
    • Other times maybe the % is in the path. In which case it should be escaped like this: ^%programfiles^%
    • Of course using special characters (for a file name or directory name) other than [A-Za-z0-9_-]+ is generally a bad idea because it is confusing...
  • In the case where environment variables are necessary, advise resolving environment variables before vim executes the command. Don't expect/require the shell to do it.

Rare situations and why it is likely rare:

• Strange characters (ie characters other than [A-Za-z0-9_-]+ ) in the executable's file name.
  • I don't think this fact needs to be argued or proven to many people.
  • Looking at all the candidates on a machine using find / -type f -perm /a+x doesn't show any counter-examples in my experience.
  • Particularly for tagbar, the real need for executing multiple executables in a single command is rare.
    • For example hypothetical commands structured like cmd /c "(echo a & echo b)" and cmd /c ( echo a ) & ( echo b ) mentioned in thread
    • Also note, it wouldn't be very portable between shell types because the syntax varies for each shell type.
    • If a tagbar users really need to run multiple commands (perhaps to create ctags and then filter it), they should write a wrapper script or batch file. Don't let users be fancy and try to execute multiple commands in one.
• Use of a relative path to the executable file name
  • (ie when not in $PATH or %PATH% and path is relative to current dir)
  • I suppose it could be argued that different executable files with the same name are present and the one relative to the current directory should be executed... but for tagbar, I can't imagine a realistic situation where an executable file in the $PATH or %PATH%, or an abs path to the executable file would be acceptable.
  • And if there was a realistic situation, I think a wrapper batch file or script could handle the situation. (kind of like in the js world where grunt-cli works for different versions of grunt that may be used from project to project.)

Some notes about escaping characters that I made:

• This entry in the vim-dev thread mentions a partial list and notes that it may not be complete.

• It also interesting that the URL they give to MS documentation is more complete today than it apparently was when the comment was made in the vim-dev thread. (Notice it mentions more characters that must be escaped than what is mentioned in the vim-dev thread above.)

• help cmd in windows has some notes on characters to be escaped. An excerpt is quoted below: (It says wrap in quotes, but like you said escaping is also acceptable. For example, often escaping with ^ works.)

  The special characters that require quotes are:
       <space>
       &()[]{}^=;!'+,`~
  

• Some additional points of confusion (for me at least) is that on windows, sometimes % needs to be escaped as ^% and other times as %%... and there is also """ to represent a single double-quote: ". (I have learned by trial and error about when to use which escaping type... but I wished it would be documented more definitively.)

• Anyways - based on the above, the list of chars here is incomplete if I am interpreting help cmd correctly. So this code could be updated too...?

  • But this could be mute ... because after thinking about these special characters, I think it is better to require the user escape them. It would be super-rare to actually have the special chars, and if they did occur, it could be ambiguous about whether they should be escaped or not. Let the user tell tagbar how they should be escaped.

[janlarres]

Wow, you really went the extra mile! I don't think we have to do anything wildly complicated; we don't really have to support arbitrary pipelines, only relatively simple invocations of a single executable. Like you said, if something more complicated needs to be done then putting it into a script would probably be better anyway. It may even be enough to just wrap the escaping of ctags_bin in a conditional like this (untested): if has('unix') || ctags_bin =~ ' '. And apparently the escaping block isn't needed at all, so you could just try removing that.

If you also want to take a shot at the ctagsargs issue, have a look at :h type() to be able to support both a simple string and a list. Thanks!

[darcyparker]

Closing because the issue is duplicated with pull request for #135.