Fixed problem with shellslash

[andreadev-it]

This should fix at least some of the problems caused by having shellslash active on vim (#254). I've tested it as a requirement for neo-tree.nvim, and now it works either with or without shellslash.

[andreadev-it]

Please wait before using this. I found a problem on linux distribution: the shellslash option is not disabled, it doesn't exists at all! This will cause an error in this updated code. I'm fixing this by using a pcall, then I'll try updating the PR. Sorry for this

[andreadev-it]

Tested on my linux laptop, the problem should now be fixed

[Conni2461]

The problem with doing this is that we use path.sep in so many places (in plenary and telescope and probably more projects i am not aware of) to distinguished between windows and unix (like) systems.

The best example is the function below

https://github.com/nvim-lua/plenary.nvim/blob/1da13add868968802157a0234136d5b1fbc34dfe/lua/plenary/path.lua#L34-L45

now path.root() will return / if shellslash is set. I dont know what shellslash does but i still expect the root to be C: (just with / as slash) or so.

This can not be done "just like that"

[andreadev-it]

Thank you conni2461 for the reference to this problem. In that specific situation, I believe it is actually wrong to rely on the path separator to handle the root path for different OS, so I guess it should be changed. I will take a deeper look into what functions uses the path separator for this kind of things and see what should be handled differently, then will refer here. The only thing that the shellslash option does is force the use of "/" in windows also. I do not have a lot of familiarity with the code for this plugin, so informations like this are very helpful in trying to fix this issue 👍

[andreadev-it]

I've taken a deeper look at the code, and found that the path separator has been used as a way to check if the os is Windows in the following files:

• curl.lua
• path.lua
• scandir.lua
• strings.lua

and in the tests:

• path_spec.lua

I would create a function called is_windows that will basically do the same check that the original code does to discern the os type (it uses jit) and then use the is_windows function instead of something like path.sep == "\\" whenever necessary based on the code and context.

This will not affect people who doesn't have shellslash enabled since it won't actually be any different for them, but at the same time it will fix different problems for those who actually use that option.

What do you think about this solution? It will cover different files, so I prefer to check with other people before making a commit, even if this is a PR.

[Conni2461]

yeah we can do a plenary.system (or so) module for system checking. And then we can use that module everywhere here, and in telescope, etc ...

Actually after reading your code i would argue for dropping the if jit then. Its easier to just check package.config:sub(1, 1) which gives us either a / or a \. I dont think vim.o.shellslash can change that. And we dont need to distinguish between "linux", "osx", "bsd", ... if we need to do that we should use vim.fn.has. Currently we just dont use that because its not :help api-fast and it always good to have functions api-fast and like i already said most of the times we just need a is_windows comparison :laughing:

[andreadev-it]

Yes, I think it is a good idea. Even if it will be a pretty small file right now :)

I've checked on windows with shellslash and indeed package.config is not modified. I've tested some methods for finding out the os, and jit.os along with your suggested solution using package.config are the fastest (though i'm still unsure about what APIs are included in :help api-fast)

I think that extracting that logic into a function will also make it easier in case in the future there will be some changes to be made to check for windows, so it seems like a good idea.

[zenshade]

With shellslash set, using the latest branch, I still get the following error message when attempting to open a file with :Telescope find_files:

E5108: Error executing lua ...site/pack/packer/start/plenary.nvim/lua/plenary/path.lua:635: Could not create file: C:\Users\zensh\AppData\Local\nvim\C:/Users/zensh/AppData/Local/nvim-data/telescope_history stack traceback: [C]: in function 'error' ...site/pack/packer/start/plenary.nvim/lua/plenary/path.lua:635: in function 'touch

Fortunately for me, I don't actually need shellslash set, so removing it fixes the issue. For those that do, a simple workaround is to just go to stdpath("data") in a terminal (on windows, something like "C:\Users\user\AppData\Local\nvim-data") and type: touch telescope_history.

[andreadev-it]

Hi @Conni2461 , It's been a while since I opened this PR, but I wanted to complete what I started. I created the "system" module that we have talked about and changed all files where I've found a "is windows" check in order to make them coherent and use the correct module. On my PCs (both windows and linux) it is working without issues, but I need another set of eyes on the code to avoid problems.

@zenshade Wow, that's weird. Thanks for sharing the tip though. Shellslash is basically unsupported in every plugin I could find. In the end, I stopped using it and tried to fix my original problem in a different way.

[Conni2461]

we have system checks in telescope too iirc. Could you prepare a pr to replace them with plenary.system? thanks.

PR mostly looks good to me, thanks :)

[andreadev-it]

Hi Conni, thank you for the review and the suggestions (don't know why I didn't do that immediatly, tbh). I've seen also some changes made by @tajtiattila which should also resolve an issue which is still existing in my PR. If for him is fine, I could try implementing his suggestion into this PR (I didn't see any other PR opened from him).

For Telescope, yeah, I will take a look at it once this PR is merged :)

[tajtiattila]

Hi @andreadev-it, I would be glad if you could integrate my changes into your PR, it almost fixed the problems with shellslash for me.

The missing idea was to make sure both '\' and '/' is accepted in a path on Windows, independent of shellslash. This is needed because the "wrong" separator can always end up in Neovim, when building paths from external sources like environment variables.

[tajtiattila]

I've just encountered a problem with my patch and Telescope. If a Windows path has spaces in it, the spaces will be prefixed with '/'; ruining it. Likely spaces in paths get prefixed backslashes somewhere, and it later gets cleaned where the backslashes get replaced with slashes.

[xarthurx]

upvote for this PR due to the unresolved "history file creation" issue from telescope.