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(#2794): sshfs compatibility #2922

Merged
merged 5 commits into from
Sep 28, 2024
Merged

fix(#2794): sshfs compatibility #2922

merged 5 commits into from
Sep 28, 2024

Conversation

mxple
Copy link
Collaborator

@mxple mxple commented Sep 22, 2024

fixes #2794

Re-add sshfs compatibility. Update to use fs_lstat instead of fs_stat so symlinks aren't automatically followed.

@mxple
Copy link
Collaborator Author

mxple commented Sep 22, 2024

We may need to update ---@type uv.fs_stat.result|nil to reflect the correct type.

@alex-courtis
Copy link
Member

alex-courtis commented Sep 23, 2024

Looks good. Instrumenting shows no mismatch on lstat, mismatch when changed back to stat:

      log.line("dev", "reload            %s t = %s type = %s abs = %s", t == type and "OK      " or "MISMATCH", t, type, abs)
        log.line("dev", "populate_children %s t = %s type = %s abs = %s", t == type and "OK      " or "MISMATCH", t, type, abs)

Over sshfs t is always nil (expected) but type is correct.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

This is looking good; great find on lstat!

No major changes needed.

We do, however, need to test this:

  • I've tested linux local and remote.
  • Have you tested macos? I am assuming yes from the bug report.

Windows is always a problem and we do need to check it; moving to lstat is a larger change that needs to be tested. I don't have access to windows.
I'd be most grateful if you (or a volunteer) tested on windows: WSL and powershell.

@@ -151,11 +154,11 @@ function Explorer:reload(node, git_status)

if not nodes_by_path[abs] then
local new_child = nil
if t == "directory" and vim.loop.fs_access(abs, "R") and Watcher.is_fs_event_capable(abs) then
if type == "directory" and vim.loop.fs_access(abs, "R") and Watcher.is_fs_event_capable(abs) then
Copy link
Member

Choose a reason for hiding this comment

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

To avoid any future refactor/rebase fails, and to keep git history simple, please rename type to t

if t == "directory" then

-- Type must come from fs_stat and not fs_scandir_next to maintain sshfs compatibility
local stat = vim.loop.fs_stat(new_cwd)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be fs_lstat?

Copy link
Collaborator Author

@mxple mxple Sep 23, 2024

Choose a reason for hiding this comment

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

Not necessary but we can change for consistency.

table.insert(parent_nodes, node)
end
end)
:iterate()
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary whitespace change.

You can make style-fix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably a result of using :Format. I'll use the make style next time.

@mxple
Copy link
Collaborator Author

mxple commented Sep 23, 2024

I am running Linux as well. I don't have access to a Windows/MacOS machine to test.

@alex-courtis
Copy link
Member

I am running Linux as well. I don't have access to a Windows/MacOS machine to test.

I can find someone with macos however I don't know anyone with windows.

If we can't test on windows we will need to use if M.is_windows or M.is_wsl then or similar to execute the original codepath on windows. They will lose the sshfs functionality but that's preferable to the risk of breaking all functionality.

@alex-courtis
Copy link
Member

Whoops, apologies, closed by accident (ctrl-enter...)

@alex-courtis alex-courtis reopened this Sep 28, 2024
@alex-courtis
Copy link
Member

@mxple I do very much wish to merge this.

@mxple
Copy link
Collaborator Author

mxple commented Sep 28, 2024

@alex-courtis I got a Windows installation up and running. I've tested nvim-tree on both Windows 11 and WSL. I've tested normal functionality, deleting files/folders, and symlinks. I have not tested sshfs compatibility on either Windows or WSL.

image

@alex-courtis
Copy link
Member

@alex-courtis I got a Windows installation up and running. I've tested nvim-tree on both Windows 11 and WSL. I've tested normal functionality, deleting files/folders, and symlinks. I have not tested sshfs compatibility on either Windows or WSL.

You legend!

We now have verified base functionality on all 3.5 OS. I've done similar basic tests on macos.

We have tested sshfs compatibility on linux; that's new functionality we can be optimistic about.

@alex-courtis alex-courtis changed the title fix: sshfs compatibility with symlinks (#2794) fix(#2794): sshfs compatibility Sep 28, 2024
Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Many thanks @mxple I really appreciate all your work on this difficult one.

@alex-courtis alex-courtis merged commit 9650e73 into nvim-tree:master Sep 28, 2024
7 checks passed
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.

nvim-tree allows the creation of files over sshfs, but they can't be accessed
2 participants