Skip to content

fix(#2961): windows: escape brackets and parentheses when opening file #2962

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

Merged
merged 8 commits into from
Oct 25, 2024
17 changes: 8 additions & 9 deletions lua/nvim-tree/actions/node/open-file.lua
Original file line number Diff line number Diff line change
Expand Up @@ -331,9 +331,9 @@ local function open_in_new_window(filename, mode)

local fname
if M.relative_path then
fname = vim.fn.fnameescape(utils.path_relative(filename, vim.fn.getcwd()))
fname = utils.escape_special_chars(vim.fn.fnameescape(utils.path_relative(filename, vim.fn.getcwd())))
else
fname = vim.fn.fnameescape(filename)
fname = utils.escape_special_chars(vim.fn.fnameescape(filename))
end

local command
Expand Down Expand Up @@ -370,36 +370,35 @@ end
---@param mode string
---@param filename string
function M.fn(mode, filename)
local fname = utils.escape_special_chars(filename)
Copy link
Member

Choose a reason for hiding this comment

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

This is a change in behaviour that will affect linux and macos users.

Please put this behind the windows feature flag, to ensure that it only applies to windows users.

Something like

local fname
if utils.is_windows then
  fname = filename
else
  fname = utils.escape_special_chars(filename)
end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @alex-courtis , change of this line is caused by the revert of previous fix. I think there wasn't such line at the begining.

Copy link
Member

Choose a reason for hiding this comment

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

Please accept my apologies, I did not Seek First To Understand.

git diff bd4881660bf0ddfa6acb21259f856ba3dcb26a93 lua/nvim-tree/actions/node/open-file.lua

shows that the change #2903 has been completely reverted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I reverted all changes of #2903 .

In #2903 util.escape_special_chats is called here and impact all file names below, which are not correct. (my tests failed)

Now and before #2903 utils.escape_special_chars is called in the open_in_new_window function (line 334 and 336), so I think I'm just following the logic before #2903 . And the is_windows flag is already in the utils.escape_special_chars function, so not added outside utils.escape_special_chars

if type(mode) ~= "string" then
mode = ""
end

if mode == "tabnew" then
return open_file_in_tab(fname)
return open_file_in_tab(filename)
end

if mode == "drop" then
return drop(fname)
return drop(filename)
end

if mode == "tab_drop" then
return tab_drop(fname)
return tab_drop(filename)
end

if mode == "edit_in_place" then
return edit_in_current_buf(fname)
return edit_in_current_buf(filename)
end

local buf_loaded = is_already_loaded(fname)
local buf_loaded = is_already_loaded(filename)

local found_win = utils.get_win_buf_from_path(filename)
if found_win and (mode == "preview" or mode == "preview_no_picker") then
return
end

if not found_win then
open_in_new_window(fname, mode)
open_in_new_window(filename, mode)
else
vim.api.nvim_set_current_win(found_win)
vim.bo.bufhidden = ""
Expand Down
32 changes: 28 additions & 4 deletions lua/nvim-tree/utils.lua
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@ function M.path_basename(path)
return path:sub(i + 1, #path)
end

--- Check if there are parentheses before brackets, it causes problems for windows.
--- Refer to issue #2862 and #2961 for more details.
local function has_parentheses_and_brackets(path)
Copy link
Member

@alex-courtis alex-courtis Oct 20, 2024

Choose a reason for hiding this comment

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

Great work fixing this one.

local _, i_parentheses = path:find("(", 1, true)
local _, i_brackets = path:find("[", 1, true)
if i_parentheses and i_brackets then
return true
end
return false
end

--- Get a path relative to another path.
---@param path string
---@param relative_to string|nil
Expand All @@ -68,13 +79,18 @@ function M.path_relative(path, relative_to)
return path
end

local _, r = path:find(M.path_add_trailing(relative_to), 1, true)
local p = path
local norm_path = path
if M.is_windows and has_parentheses_and_brackets(path) then
norm_path = path:gsub("/", "\\")
end

local _, r = norm_path:find(M.path_add_trailing(relative_to), 1, true)
local p = norm_path
Copy link
Member

Choose a reason for hiding this comment

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

This path is executed for all OS, which may cause issues for such paths. Please execute only when is_windows

This injected error is definitely thrown on linux, showing that the code path is executed:

--- Check if there are parentheses before brackets, it causes problems for windows.
--- Refer to issue #2862 and #2961 for more details.
local function has_parentheses_and_brackets(path)
  error("has_parentheses_and_brackets")
E5108: Error executing lua: ...d/site/pack/packer/start/ljie-PI/lua/nvim-tree/utils.lua:65: has_parentheses_and_brackets called
stack traceback:
        [C]: in function 'error'
        ...d/site/pack/packer/start/ljie-PI/lua/nvim-tree/utils.lua:65: in function 'has_parentheses_and_brackets'
        ...d/site/pack/packer/start/ljie-PI/lua/nvim-tree/utils.lua:84: in function 'path_relative'
        .../packer/start/ljie-PI/lua/nvim-tree/explorer/filters.lua:161: in function 'custom'
        .../packer/start/ljie-PI/lua/nvim-tree/explorer/filters.lua:247: in function 'should_filter_as_reason'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added flag on line 84, so that norm_path will keep unchanged for non-windows OS.

if r then
-- take the relative path starting after '/'
-- if somehow given a completely matching path,
-- returns ""
p = path:sub(r + 1)
p = norm_path:sub(r + 1)
end
return p
end
Expand Down Expand Up @@ -272,14 +288,22 @@ function M.canonical_path(path)
return path
end

--- Escapes special characters in string for windows, refer to issue #2862 and #2961 for more details.
local function escape_special_char_for_windows(path)
if has_parentheses_and_brackets(path) then
return path:gsub("\\", "/"):gsub("/ ", "\\ ")
end
return path:gsub("%(", "\\("):gsub("%)", "\\)")
end

--- Escapes special characters in string if windows else returns unmodified string.
---@param path string
---@return string|nil
function M.escape_special_chars(path)
if path == nil then
return path
end
return M.is_windows and path:gsub("\\", "/") or path
return M.is_windows and escape_special_char_for_windows(path) or path
Copy link
Member

Choose a reason for hiding this comment

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

Thank you, this one definitely should be behind the windows feature flag.

end

--- Create empty sub-tables if not present
Expand Down
Loading