-
Notifications
You must be signed in to change notification settings - Fork 199
Fix: Windows path conversion fails for forward-slash paths #686
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
base: master
Are you sure you want to change the base?
Conversation
stevearc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about using vim.fs.normalize and one small req for the tests
| ---@param path string | ||
| ---@return string | ||
| local function normalized_path_seperators(path) | ||
| local leading_slashes, rem = path:match("^([\\/]*)(.*)$") | ||
| local normalized_rem = rem:gsub("[\\/]+", M.sep) | ||
| local normalized_leading = "" | ||
| if #leading_slashes >= 2 then | ||
| normalized_leading = M.sep .. M.sep | ||
| elseif #leading_slashes == 1 then | ||
| normalized_leading = M.sep | ||
| end | ||
| return string.format("%s%s", normalized_leading, normalized_rem) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make use of vim.fs.normalize for any of this functionality? It seems to do some similar things (preserving leading double slashes) and I'd like to rely on the Neovim libs whenever possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make use of
vim.fs.normalizefor any of this functionality?
In nightly it looks like it would work. I still need to try it out in the oil code but from some quick testing I think it would work, However, I don't think it would work if you want to continue supporting Neovim 0.8+. There were a lot of improvements to vim.fs.normalize in 2024 and one improvement in 2025 that we may need for it to function correctly.
https://github.com/neovim/neovim/blob/b1ae775de618e3e87954a88d533ec17bbef41cdf/runtime/lua/vim/fs.lua#L209-L231
https://github.com/neovim/neovim/blob/0ef27180e31671a043b28547da327cd52f1a87c4/runtime/lua/vim/fs.lua#L306-L352
https://github.com/neovim/neovim/blob/0c995c0efba092f149fc314a43327db7d105e1ad/runtime/lua/vim/fs.lua#L504-L612
https://github.com/neovim/neovim/blob/c1c007813884075a970198fbba918d568428d739/runtime/lua/vim/fs.lua#L587-L688
https://github.com/neovim/neovim/blob/5370b7a2e0a0484c9005cb5a727dffa5ef13b1ed/runtime/lua/vim/fs.lua#L596-L697
I could also do the normal if has version use vim.fs.normalize else use a copy of nightly's version if that sounds better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neovim 0.9 came out almost 3 years ago, so I'm fine to drop support for 0.8 now. I only kept it this long because there was no strong motivation to remove it, but now there is!
tests/url_spec.lua
Outdated
| local expected = "oil-ssh://user@hostname:8888//" | ||
| local output, basename = oil.get_buffer_parent_url(input, true) | ||
| assert.equals(expected, output, string.format('Parent url for path "%s" failed', input)) | ||
| assert.equals(nil, basename, string.format('Basename for path "%s" failed', input)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of exploding this test, could we just have a
if not fs.is_win then
-- add the cases that fail on windows
endThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is something like this what you have in mind?
it("get_url_for_path", function()
local cases = {
{ "", "oil://" .. util.addslash(vim.fn.getcwd()), skip_on_windows = true },
{
"term://~/oil.nvim//52953:/bin/sh",
"oil://" .. vim.loop.os_homedir() .. "/oil.nvim/",
skip_on_windows = true,
},
{ "/foo/bar.txt", "oil:///foo/", "bar.txt", skip_on_windows = true },
{ "oil:///foo/bar.txt", "oil:///foo/", "bar.txt" },
{ "oil:///", "oil:///" },
{ "oil-ssh://user@hostname:8888//bar.txt", "oil-ssh://user@hostname:8888//", "bar.txt" },
{ "oil-ssh://user@hostname:8888//", "oil-ssh://user@hostname:8888//" },
}
for _, case in ipairs(cases) do
local is_skip = case.skip_on_windows and uv.os_uname().version:match("Windows")
if not is_skip then
local input, expected, expected_basename = unpack(case)
local output, basename = oil.get_buffer_parent_url(input, true)
assert.equals(expected, output, string.format('Parent url for path "%s" failed', input))
assert.equals(
expected_basename,
basename,
string.format('Basename for path "%s" failed', input)
)
end
end
end)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's closer, but I think it would be cleaner to do something like
local cases = { ... }
if not uv.os_uname().version:match("Windows") then
-- skip these tests on windows
vim.list_extend(cases, {
...
})
end
Did not address comment #493 (review). Imo, we can handle that in the future.