fix: improve path escaping for commands on Windows

[bwpge]

This PR changes the handling of paths on Windows systems to address a few open issues. This builds on the work originally introduced in #1023 (to address #889). Realistically, this is another attempt to deal with a (neo)vim issue as mentioned in neovim/neovim#3912 (no real update since 2015).

I've added some doc comments to help with maintainability, but if it's too verbose I'm happy to edit.

I've run tests locally and all pass, but I would really love to add regression tests for path escaping so we don't run into more problems in the future. However, I'm having trouble figuring out how to deal with platform-specific behavior of vim.fn.fnameescape (e.g., if you run tests on WSL or macOS, you won't get Windows path escaping). If anyone has any ideas for adding tests, I'm definitely all ears. Ideally, we would test both Windows and Unix-style path escaping regardless of the platform running the tests.

This PR fixes the following issues I could find:

• BUG: directories beginning or ending with symbol cause error in filesystem command set_root #1352 (opened by me when testing this fix)
• BUG: modified status and diagnostic does not change for buffers that opened by neo-tree #1264

This also fixes the following external issues:

• Bug: Not detecting branch on files in subdirectories nvim-lualine/lualine.nvim#1183

The following issues may be fixed, but I have not confirmed:

• BUG: attempt to concatenate local 'cwd' (a nil value) #1157

I think there are definitely more issues that could be addressed by this, but I'm reaching my issue-searching capability limits :)

I feel like I have a good grasp of how paths are being used in this limited capacity (such as with set_cwd and open_file) but I know this change could have cascading effects because it's modifying the input to commands like :cd, :b, :vs, etc. If anyone sees anything problematic with more context of the full codebase, I'm happy to discuss.

[cseickel]

@pysan3 Can you review this? I know we have planned to incorporate pathlib but this is probably a good patch while we wait for that. Besides, making that library makes you the resident expert on path issues, and it would also be good for you to know about this bug to make sure it is handled in pathlib.

[pysan3]

@bwpge Could you test it with vim.fn.shellescape(string, true) instead? Or maybe the second argument can be omitted.

Unfortunately I don't have a Windows machine at hand and I can't test it right now.

This might bring what you want, but we need to make sure that this doesn't start breaking things in other places.

If shellescape doesn't do the trick, I think this approach is good enough if it works. Sorry again, I don't have a way to test it locally, so I can't say for sure...

[pysan3]

@bwpge Could you also test simply passing the path to vim.cmd.cd without constructing a cmd string in the first place?

• vim.cmd.cd(state.path)
• vim.cmd.lcd(state.path)
• vim.cmd.tcd(state.path)

[bwpge]

@pysan3 sure thing, I am headed to bed but I will test these in the morning and post findings. And I totally understand the apprehension, this is a big change so we should take extra time to explore its effects.

[bwpge]

I tried a couple things just in command mode to avoid any extra complications with the plugin. To setup:

• Created the nested directories foo\(bar) on a test drive X:
• Created baz.txt in (bar) with some text in it
• Opened neovim in the root of X:\

Output of vim.fn.shellescape (just to see what strings get generated):

• :lua =vim.fn.shellescape([[X:\foo\(bar)\baz.txt]]): "X:\foo\(bar)\baz.txt" (note the double quotes)
• :lua =vim.fn.shellescape([[X:\foo\(bar)\baz.txt]], true): same as above

Attempting to set working directory with:

• :lua vim.cmd.cd(vim.fn.shellescape[[X:\foo\(bar)]])
• :lua vim.cmd.cd(vim.fn.shellescape([[X:\foo\(bar)]], true))
• Same permutations with lcd, tcd

All cause the same error, I believe due to the double quotes introduced by shellescape:

E5108: Error executing lua [string ":lua"]:1: Vim:E344: Can't find directory ""X:\foo(bar)"" in cdpath
stack traceback:
        [C]: in function 'cd'
        [string ":lua"]:1: in main chunk

Just to note from the shellescape() help:

On Windows when 'shellslash' is not set, encloses {string} in double-quotes and doubles all double-quotes within {string}. Otherwise encloses {string} in single-quotes and replaces all "'" with "'\''".

I could be wrong but I think this behavior comes from cmd.exe (default neovim shell on Windows) being a bit goofy -- IIRC it uses double quotes somewhat similar to most other shells' single quotes for literal strings (or whatever the proper term is). I think this would be problematic since users can change the shellslash option and it would affect how the plugin manages state.

[pysan3]

Thank you very much @bwpge !

So after all my ideas are not working so doing gsub("\\%p", ... is the best we can do now.

I'll remember this issue and will try to fix it in the rewrite but this is a good fix for now.

[bwpge]

Just adding some extra notes someone can reference when tackling this in a rewrite:

• The main issue to fix is the mixing/matching of how paths are handled in Neo-tree on Windows (\ vs /)
  • Both path separators are "valid" insofar as they can point to a file or directory
  • (Neo)vim clearly handles them inconsistently
  • Makes it difficult to reliably do things like checking if paths contain other paths, dealing with relative paths, etc.
  • Sounds like pathlib aims to deal with this problem which would be great
• Default behavior of Neovim on windows is to use \ separators
  • Such as if you tab-complete a path with :e foo/b<TAB>, you will get :e foo\bar.txt filled in
  • Other plugins tend to expect this behavior, as an example with lualine trying to detect git branch from the current buffer name
• Setting the working directory with / separators does not seem to be an issue
  • The value returned by vim.fn.getcwd() seems to always use \ separators, even if set with vim.cmd.cd[[X:/foo/bar]]
• Opening a file with / will have side-effects, most notably changing the buffer name
  • NOTE: This only seems to happen if an absolute path with / separators is used
  • Editing a file like :e foo/bar/baz.txt will still name the buffer X:\foo\bar\baz.txt (checking the name with vim.api.nvim_buf_get_name(0))
  • Editing a file :e X:/foo/bar/baz.txt does change the buffer name: X:/foo/bar/baz.txt

[bwpge]

@pysan3 I apologize to hit you with so many pings, but I just stumbled across another Windows-path issue that can be fixed relatively easily.

The way vim.lsp.buf.definition() opens a file vs. what Neotree is using to compare buffers for modified status: Windows paths have a mismatch on drive letters (upper vs. lowercase) which breaks the buffer status on Neotree. This is a one-line change in utils.normalize_path, change upper() to lower() to fix the problem:

neo-tree.nvim/lua/neo-tree/utils/init.lua

Line 763 in f3941c5

path = path:sub(1, 1):upper() .. path:sub(2)

Unfortunately this affects the rendered path at the top of Neotree -- drive letters will be lowercase, which is a bit ugly. I suppose that could be cleaned up in the UI/renderer (just transform the string before rendering without changing the node path), but would require more changes.

Would you want me to add to this PR (since this is still in the same spirit of improving Windows path stuff), or move to another PR?

EDIT: Never mind, that change breaks git status colors in filesystem view. I'll open an issue to track separately.

[pysan3]

Could you instead capitalize the input from lsp?

That's what is done in vim.fs.normalize so it should be better to align all drive letters as capital.

vim.fs.normalize has too many flaws and can't be used here but it's good to keep consistency with upstream.

[bwpge]

I've been playing with a simple solution this morning/afternoon to avoid touching too many things, I think it's a "good enough" solution without touching too much of the plugin behavior. I've added bwpge@8afd209 to this PR so you can see what I've changed (the explanation in a comment was getting too long and hard to follow) -- if we don't like it I can revert.

Basically we just change how we try to index into state.diagnostics_lookup and state.opened_buffers for nodes. This way everything in the plugin behaves the same, and we only have to do string manipulation for Windows clients, on a much more limited number of items.

The benefit of this solution is it catches other case-sensitivity issues Windows people might see since Windows doesn't treat different casing as different files. Also Linux/macOS users wouldn't have impact from this change, the function would just fall through and basically only pay the cost of two if checks.

[pysan3]

Amazing work.
If you add the todo comment, I also agree this is a good enough workaround @cseickel ;)

I'll remember to revisit this after I manage to fully move to pathlib. (I'm not gonna say a due date here, as it might be forever lol).

Done is better than perfect.

[pysan3]

LGTM @cseickel

[cseickel]

Thanks @bwpge and @pysan3.

[pysan3]

@bwpge Looks like we unfortunately hit an edge case: #1382

These are the chars what will be matched with "%p" but could you instead test all out which ones will not cooperate with vim's :e?

-- The entire list [[!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~]]

-- This is from a google search; What characters are forbidden in Windows and Linux directory names?
-- < (less than)
-- > (greater than)
-- : (colon - sometimes works, but is actually NTFS Alternate Data Streams)
-- " (double quote)
-- / (forward slash)
-- \ (backslash)
-- | (vertical bar or pipe)
-- ? (question mark)
-- * (asterisk)

-- I deleted those from the list. But added `<Space>`. 
local puncts = [[!#$%&'()+,-;=@[]^_`{}~ ]]

-- Please remove characters that don't need to be escaped from the list.

-- Here's an easy way to test.
for char in string.gmatch(puncts, ".") do
  vim.cmd.edit(string.format([[.\tests\%sfoo.txt]], char))
end

-- See the buffer list `:ls!` and if no need to escape, buffer name will be `.\tests\_foo.txt`
-- but if it needs to be escaped, buffer name will be like `.\tests'foo.txt` (outside tests dir). (tests-foo altogether is the file name).

From my experiments, I see the following letters NOT working i.e. needs another backslash to escape. Is it the same for your case as well?

Not Working: #%&'();^` and <Space>
Working:     !$+,-=@[]_{}~\

[bwpge]

@pysan3 here's the raw output from :ls! with your test above (very clever!) -- just a heads up you need add another closing paren on the vim.cmd.edit line:

  8  h   ".\tests\!foo.txt"             line 1
  9  h   ".\tests#foo.txt"              line 1
 10  h   ".\tests\$foo.txt"             line 1
 11  h   ".\tests%foo.txt"              line 1
 12  h   ".\tests&foo.txt"              line 1
 13  h   ".\tests'foo.txt"              line 1
 14  h   ".\tests(foo.txt"              line 1
 15  h   ".\tests)foo.txt"              line 1
 16  h   ".\tests\+foo.txt"             line 1
 17  h   ".\tests\,foo.txt"             line 1
 18  h   ".\tests\-foo.txt"             line 1
 19  h   ".\tests;foo.txt"              line 1
 20  h   ".\tests\=foo.txt"             line 1
 21  h   ".\tests\@foo.txt"             line 1
 22  h   ".\tests\[foo.txt"             line 1
 23  h   ".\tests\]foo.txt"             line 1
 24  h   ".\tests^foo.txt"              line 1
 25  h   ".\tests\_foo.txt"             line 1
 26  h   ".\tests`foo.txt"              line 1
 27  h   ".\tests\{foo.txt"             line 1
 28  h   ".\tests\}foo.txt"             line 1                                                                                                                                                                                    29 #h   ".\tests\~foo.txt"             line 1                                                                                                                                                                                    30 %a   ".\tests foo.txt"              line 1

Looks like the following is our list:

Status	Symbol
Not working	!$+,-=@[]_{}~
Working	#%&'();^` (last is <Space>)

I'll work on fixing this today and do some testing to make sure we don't have any regressions.

[pysan3]

@bwpge When you are done, could you change the regarding code using vim.fn.escape instead and test it and submit a PR. My bad I cannot easily reach a Windows device...

escaped_path = vim.fn.escape(escaped_path, puncts)

[bwpge]

Just to sanity check, my intent would be to do something like this:

M.escape_path_for_cmd = function(path)
   -- this is working on unix paths, don't touch for now
  local escaped_path = vim.fn.fnameescape(path)

  if M.is_windows then
    local puncts = [[!$+,-=@[]_{}~]]
    -- do extra windows escapes
  end
  return escaped_path
end

[pysan3]

Yes and :h escape() should do exactly that as I mentioned above.

Under -- do ...

Please double check if it actually works tho. Thanks in advance.

[bwpge]

Sorry meant to send this earlier:

Regarding :h escape(), part of our problem is that we only need the extra escape character if the symbol is preceded by a \. So for example, .\foo\(bar\baz.txt needs an extra escape on (, but .\foo\bar)\baz.txt does not need it on ). So I think it will need a more targeted approach than vim.fn.escape.

Will post when I have more info to share.

[pysan3]

Oh shoot. You are right. I guess we need some kind of a complex regex.