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 ~ normalization in Path:normalize() #279

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

fenuks
Copy link
Contributor

@fenuks fenuks commented Dec 6, 2021

This fixes nvim-telescope/telescope.nvim#1521. The problem is that if $HOME directory contains trailing slash, then Path:normalize() will convert /home/user/my_file into ~my_file.

lua/plenary/path.lua Outdated Show resolved Hide resolved
@l-kershaw
Copy link
Collaborator

I'm happy to merge this once the changes pointed out by stylua are made (we use double quotes for strings were possible)

@fenuks
Copy link
Contributor Author

fenuks commented Dec 14, 2021

Thanks, I (hopefully) fixed stylua problems.

@l-kershaw
Copy link
Collaborator

Looks good to me 🙂
Thanks!

@fenuks
Copy link
Contributor Author

fenuks commented Dec 15, 2021

Thank you for your help. ;)

@steve-lorimer
Copy link

Note that PR has introduced the arguably incorrect behaviour that you can now end up with double slashes in your normalized path.

eg: if path.home is /home/user and filename is /home/user/foo, this change results in the normalized path being ~//foo instead of the arguably correct ~/foo

I think a more correct approach will be to check if path.home has a trailing slash, and selectively apply it in the case it doesn't

I'll submit a new PR

@fenuks
Copy link
Contributor Author

fenuks commented Jan 21, 2022

It is expected in sense there is even test for that. Personally I think this is not a problem at all, it's a valid, doesn't cause any problems, and makes code simpler.

@Conni2461
Copy link
Collaborator

assert.are.same("~//./test_file", p:normalize())

This is not normalized. This shouldnt have been merged.

Ref: nvim-telescope/telescope.nvim#1662 nvim-telescope/telescope.nvim#1683

@steve-lorimer
Copy link

steve-lorimer commented Jan 21, 2022

@Conni2461 I have a change in a local branch which just checks for trailing slash in path.home, which will "fix" the above issue which is to indiscriminantly add path.sep in normalize

-- remove trailing slash from homedir if any
if path.home:sub(-1) == path.sep then
  path.home = path.home:sub(1,-2)
end

It will result in the following behaviour:

assert.are.same("~/./test_file", p:normalize())

So at least the double slash is removed, but arguably this isn't normalized either, as the ./ should be removed too, would you agree?

In any event, I can submit the above change as a PR if you wish?

@fenuks
Copy link
Contributor Author

fenuks commented Jan 21, 2022

It's not normalized, and I wouldn't have been looking for what caused double slashes in the first place if it "is not a problem at all"

Well, so I was wrong about it not being a problem…

@steve-lorimer
Copy link

@Conni2461 I've created a PR (#308) with the above mentioned change - if you feel it's not the correct approach more than happy to alter it...

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.

Picking a file to open outside the current directory can mangle the filename
4 participants