Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: continue improving Windows path escaping for commands #1388

Merged
merged 1 commit into from Mar 9, 2024

Conversation

bwpge
Copy link
Contributor

@bwpge bwpge commented Mar 9, 2024

This PR addresses more edge cases with path escaping and vim commands, as noted in #1382.

After some testing, I found that only a small number of punctuation characters required the extra escape after vim.fn.fnameescape. They are: &, (, ), ;, ^, and `.

Details

Testing method used was running :luafile % with the following test.lua on a Windows machine:

-- copy/paste from neo-tree
local is_windows = vim.fn.has("win32") == 1 or vim.fn.has("win32unix") == 1

local escape_path_for_cmd = function(path)
  local escaped_path = vim.fn.fnameescape(path)
  if is_windows then
    for _, c in ipairs({ --[[ add characters as cases fail ]] }) do
      escaped_path = escaped_path:gsub("\\%" .. c, "\\%1")
    end
  end
  return escaped_path
end
-- end copy/paste

vim.fn.mkdir("tests", "p")

-- known allowed NTFS punctuation
local symbols = [[!#$%&'()+,-;=@[]^_`{}~ ]]

-- write file with a leading punctuation to a nested directory
for char in string.gmatch(symbols, ".") do
  -- double the punctuation to ensure it actually gets escaped correctly
  local f = string.format([[.\tests\%sfoo.txt]], char .. char)
  local p = escape_path_for_cmd(f)
  vim.cmd("silent w " .. p)
end

-- files outside of `tests` directory didn't get escaped correctly
for _, f in ipairs(vim.fn.glob("tests*.txt", false, true)) do
  vim.notify("Failed: " .. f, vim.log.levels.ERROR)
  os.remove(f)
end

-- these files were written to the correct path
for _, f in ipairs(vim.fn.glob("tests/*.txt", false, true)) do
  os.remove(f)
end

This testing logic could certainly be improved, I just quickly slapped this together.

The only reason I included the above is because it may be worth creating Windows path tests in the future and didn't want to lose this information.

I manually verified this fixes the issue in #1382, and tried to recreate some issues I've reported/worked on (such as #1352) and did not see any regressions. That being said, I would feel a bit better if someone could get a second pair of eyes on a Windows machine with this PR.

@bwpge
Copy link
Contributor Author

bwpge commented Mar 9, 2024

/cc @pysan3

@burgr033
Copy link

burgr033 commented Mar 9, 2024

@bwpge asked me in #1382 to pull their fork and see if I find anything odd. I worked with their fork for half an hour and couldn't really find anything out of the ordinary.

I even checked with a few other characters.

_workspace
;workspace
(workspace
)workspace
&workspace
%workspace
`workspace
´workspace
^workspace
~workspace

Everything that I could check, works fine :)

Copy link
Contributor

@cseickel cseickel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

@cseickel cseickel merged commit 230e938 into nvim-neo-tree:main Mar 9, 2024
3 checks passed
@pysan3 pysan3 added this to the v4.0 milestone Mar 9, 2024
@pysan3
Copy link
Collaborator

pysan3 commented Mar 9, 2024

Thanks for working on this @bwpge !

Amazing work!

@bwpge
Copy link
Contributor Author

bwpge commented Mar 9, 2024

Happy to help! And just a reminder to @burgr033 to change your config back, thanks for the assist :)

@burgr033
Copy link

burgr033 commented Mar 9, 2024

thank you all <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: opening up a file via Neotree results in gopls not honoring the package environment (on Windows)
4 participants