xolox / vim-easytags

Automated tag file generation and syntax highlighting of tags in Vim
http://peterodding.com/code/vim/easytags/
1.01k stars 109 forks source link

Async: stage 2 invocation fails when Vim command is shell-escaped. #51

Closed inkarkat closed 11 years ago

inkarkat commented 11 years ago

This may need to be addressed in xolox#misc#os#exec(), but I raise this here because (apart from my patch #50) this prevented me from using the async branch.

The execution fails (silently; I just see in the task manager that no second gvim process is launched) when the command executable is enclosed in double quotes:

"C:\Program Files\vim\vim73\gvim.exe" -u NONE -U NONE --noplugin -N --cmd "let &rtp = 'D:/A/.vim,C:\Program Files\vim/vimfiles,C:\Program Files\vim\vim73,C:\Program Files\vim/vimfiles/after,D:/A/.vim/after' | call xolox#easytags#update_remote({'cmdline': 'ctags --fields=+l --c-kinds=+p --c++-kinds=+p --sort=no -f- ""--language-force=vim"" ""D:\A\Unixhome\.vim\autoload\xolox\misc\path.vim""', 'by_filetype': '', 'tagsfile': '\A\Unixhome\.vim\tags', 'servername': 'GVIM', 'verbose': 1, 'have_args': 0, 'starttime': [400, 326843564], 'silent': 1, 'filter_tags': 0, 'cache': {}})"

When I replace "C:\Program Files\vim\vim73\gvim.exe" with gvim, it works. Also, when the entire command line is additionally quoted: ""C:\Program Files\vim\vim73\gvim.exe" ... ". This has to do with the strange quoting rules of the Windows shell. The built-in system() suffered from this a long time, too (and I had to use the quote-all there), until patches starting with 7.3.443 made this better. I leave it to you whether you just double-quote the entire command (and whether you do that in the exec() function or its callers), or look into Vim's new quoting.

xolox commented 11 years ago

The built-in system() suffered from this a long time, too (and I had to use the quote-all there), until patches starting with 7.3.443 made this better.

I've read about the quote-all rule before. I'm now reading through the discussion around patch 7.3.443, what a mess:

Summary what I remember so far, and a suggestion at the end. Please correct mistakes.


Bug (before the patch): shellcmdflag: /c shellxquote: empty

user executes :!"my editor.exe" "my file.txt" vim executes cmd /c "my editor.exe" "my file.txt" cmd executes my editor.exe" "my file.txt (obviously wrong)

Problem: cmd sometimes strips outer quotes (and doesn't provide an option to force keeping the quotes) Solution: always add surrounding quotes and force removing them with /s

Patch: shellcmdflag: /s /c shellxquote: "

Yeah, this works now: user executes :!"my editor.exe" "my file.txt" vim executes cmd /s /c ""my editor.exe" "my file.txt"" cmd executes "my editor.exe" "my file.txt"


Bug (with the patch) shellcmdflag: /s /c shellxquote: "

user executes :!editor "my&file.txt" vim executes cmd /s /c "editor "my&file.txt"" cmd executes "editor "my & file.txt"" (two commands, separated by `&')

Problem: cmd doesn't always strip outer quotes, but stops before &' Solution: make cmd's argument "atomic" by enclosing it in('...`)'

Patch (by RoDo): shellcmdflag: /c (no need to strip quotes) shellxquote: ( (and automatically use `)' at the end)

Yeah, this works now (also did without patch 7.4.443): user executes :!editor "my&file.txt" vim executes cmd /c (editor "my&file.txt") cmd executes (editor "my&file.txt")


Bug (with RoDo's patch) shellcmdflag: /c shellxquote: (

user executes :!echo a & echo b vim executes cmd /c (echo a & echo b) cmd executes (echo a & echo b) -> output: two lines, a' andb)'

Problem: echo does not ignore the trailing )' Solution: enclose cmd's argument in"('...`)"' (I have no idea why this works)

Patch: shellcmdflag: /c (not sure if /s /c' would make a difference, I'd expect/c' to always remove outer quotes) shellxquote: "( or maybe: shellxquote: "(,)" (new syntax!)

Yeah, this works now (also did without patch 7.3.443): user executes :!echo a & echo b

The whole thread is full of gems like this:

So you're saying, we must escape all special characters, INCLUDING QUOTES, and surround in parentheses?

Yes, including quotes!

That's...special.

But solves all problems so far.

Yes, but that doesn't mean I need to like it :-P

I see you also joined the discussion :-). But then it just stops, there's no conclusion to the thread? After reading the whole thread I'm still left wondering how to properly escape my command lines on Windows... The irony is that I recently revived an old Windows XP VM (which I hadn't used in over a year) only so I could test my Vim plug-ins on Windows.

Okay, clearly something needs to be done. You indicated that applying the quote-all rule to a full command line works. Before and after the patch? As in, can I just unconditionally apply the quote-all rule on Windows, whether the command line is executed by xolox#shell#execute_with_dll() or system()?

inkarkat commented 11 years ago

Unfortunately, this quoting on Windows is a mess. I've only recently reported a minor problem with the new, improved Vim setting.

A quick tests shows that no, the quote-all cannot be applied to commands executed via system(); I get

'""c:\Program' is not recognized as an internal or external command, operable program or batch file.

whereas the same command works through xolox#shell#execute_with_dll(). The reason is that the crummy Windows shell has a special case for when all is quoted, but Vim additionally wraps the command-line with (...), so the special case doesn't apply.

I think it would be best if xolox#shell#execute_with_dll() would emulate the Vim behavior (i.e. surround with 'shellxquote' and escape characters with 'shellxescape'). On the other hand, I think you're always using cmd.exe to start the processes, but what if the user has reconfigured the 'shell' setting to use a different shell for system()?! You couldn't use the actual option values then, since they don't apply to your shell.

On the other hand, your xolox#shell#execute_with_dll() function is presumably used far less, mostly by your own plugins. If the quote-all is sufficient for that, why complicate things arbitrarily?! I'd suggest to first try that; you can always implement the alternative if users start complaining about command launch failures :-)

xolox commented 11 years ago

Ingo, thanks for the bug report! I just committed a change to vim-shell that should resolve the problem you reported.

I wasted too much of my weekend on the quoting hell that is Windows :-) and in the end opted to go with the dumb "just double quote the whole command line and be done with it" rule, because it's very simple (although it's also very counterintuitive) and it seems to work reliably (although I've only been able to test it on Windows XP, because that's the only Microsoft Windows installation I have access to).

I'm closing this issue now because the problem seems to be resolved, but if it's not then feel free to reopen the issue.

inkarkat commented 11 years ago

Thanks, I can confirm it's working for me on Windows 7 64-bit, too. I'm now using the unmodified version from your async-cleanup branch without any customizations or patches, just as it should be. I'll try to exercise the plugin on Linux soon, to get more data about the crashes you've experienced.

Oh, and I'm happy to see that you've started including unit tests for vim-misc. Did you know that there are existing plugins / frameworks for that (one from me: runVimTests; it has links to others), or why did you roll your own testing framework?

xolox commented 11 years ago

Thanks, I can confirm it's working for me on Windows 7 64-bit, too. I'm now using the unmodified version from your async-cleanup branch without any customizations or patches, just as it should be. I'll try to exercise the plugin on Linux soon, to get more data about the crashes you've experienced.

I'm glad to hear the async-cleanup branch is now working well for you without needing any modifications! Hopefully we can get the Linux issues sorted out soon, then it can probably be merged into master. Maybe as an option at first, and after collecting some more feedback it can become the default.

Oh, and I'm happy to see that you've started including unit tests for vim-misc.

Yeah, should have started with that years ago, but now is still better than later :-).

Did you know that there are existing plugins / frameworks for that (one from me: runVimTests; it has links to others), or why did you roll your own testing framework?

There are some minor advantages to keeping the testing code in vim-misc, for example my other plug-ins can freely use the testing framework (if I can even call it that, it's quite small) without adding more dependencies (my intention is to write tests for all of my plug-ins, but this is a long term plan :-).

However to be honest the test runner was just a fun little project. I was writing automated tests for xolox#misc#os#exec() and running them manually on Mac OS X, Windows XP and Ubuntu Linux 12.04 at the same time. That got very boring very quickly, so I wrote a Python script to simultaneously run the tests on Mac OS X, Windows XP and Ubuntu Linux 12.04 and report the summarized results to me.

Evidently I got a bit carried away :-). It grew organically from a single file containing the xolox#misc#os#exec() tests and some helpers into a minimal testing framework that I eventually extracted into a separate Vim script. I'll see where it goes, in the end I might decide to scrap it and go with another test runner, like one of the ones you mentioned.

I did find your plug-in and the links to the others and compared the "APIs" to see if any of them fit my expectations/needs, but they didn't exactly do that. For example when I expect two Vim script values to be the same and they are not, I find it very useful to get the pretty printed values right inside the message which reports the assertion failure. I didn't find that in any of the listed plug-ins (maybe I simply didn't look hard enough?).