Update fs_actions.lua fixes #1399 #1400
Conversation
| @@ -496,7 +496,7 @@ M.delete_node = function(path, callback, noconfirm) | |||
| -- first try using native system commands, which are recursive | |||
| local success = false | |||
| if utils.is_windows then | |||
| local result = vim.fn.system({ "cmd.exe", "/c", "rmdir", "/s", "/q", path }) | |||
| local result = vim.fn.system({ "cmd.exe", "/c", "rmdir", "/s", "/q", '"', path, '"' }) | |||
There was a problem hiding this comment.
Sorry about the bug and thanks for fixing, but I think there may be a better way to go about quoting. Isn't there a vim api to shellescape paths? My concern is that there may still be other file path issues we are not handling here and we should take this opportunity to address them all.
@pysan3 what do you think?
There was a problem hiding this comment.
Dang it. I explicitly said we need to add check for <Space> on top of the punctuations but then completely forgot about that when reviewing the PR: #1388
There was a problem hiding this comment.
It's that cmd.exe /C treats all following arguments as single string and runs that command so it forgets that path is one argument.
There was a problem hiding this comment.
I'll definitely take a look when I get some time tonight.
To address @cseickel's point, the user can change their shell/shell escaping options (example :h shell-powershell), so if we're deliberately using cmd for Windows (which is a good approach for compatibility like if a user can't use powershell on corporate machines) we'll probably have to escape that ourselves.
And of course cmd plays by its own made up rules when it comes to escapes and quoting...
There was a problem hiding this comment.
Would it be less pain to not run cmd.exe when path contains space or puncts and fallback to recursively vim.loop.fs_rmdir?
There was a problem hiding this comment.
Using vim.fn.shellescape(path) might be better than '"' .. path .. '"' to say the least.
There was a problem hiding this comment.
After some amount of testing, I think shellescape does the job. I could not find any edge cases.
@WillEhrendreich Could you change your fix to use vim.fn.shellescape(path) instead? It would be nice if you could test your issue with this change.
@bwpge May I get a confirmation from you as well?
There was a problem hiding this comment.
@pysan3 just a note, vim.fn.shellescape relies on the user's shellslash option (only applies to Windows). While I highly doubt in general people will mess with this setting, and it is off by default, it can create the following (difficult to troubleshoot) issue with a path like foo/bar's (note the use of single vs double quotes):
- With
shellslashfalse:"foo/bar's"-> valid - With
shellslashtrue:'foo/bar'\''s'-> invalid
Obviously is a bit contrived, and is definitely better than what we had before, but is something to be aware of.
from review feedback. tested quick, seems fine.
There was a problem hiding this comment.
Thanks everyone for helping out with this.
@WillEhrendreich @pysan3 @bwpge
No description provided.