fix(path): improve & fix bugs in Path:rename()
#485
Conversation
tmillr
changed the title
Path:rename() bugs
lua/plenary/path.lua
Outdated
| -- BUG | ||
| -- handles `.`, `..`, `./`, and `../` | ||
| if opts.new_name:match "^%.%.?/?\\?.+" then | ||
| opts.new_name = { | ||
| uv.fs_realpath(opts.new_name:sub(1, 3)), | ||
| opts.new_name:sub(4), | ||
| } | ||
| end | ||
|
|
There was a problem hiding this comment.
I removed this as it contains a bug. Secondly, it is unnecessary, as uv.fs_rename already handles such relative paths.
| if new_path:exists() then | ||
| error "File or directory already exists!" | ||
| end |
There was a problem hiding this comment.
I changed this. This was a bug because Path:exists() uses stat, not lstat.
For example, new_path could be a bad symlink, in which case it could incorrectly think that the file doesn't exist when it actually does (rename(2) does not resolve the final path component if it's a symlink, the symlink would be replaced instead).
1. this is what python does 2. the underlying path has changed, and a new instance helps to reflect that
tmillr
changed the title
Path:rename() bugsPath:rename()
| function Path:rename(opts) | ||
| -- TODO: For reference, Python's `Path.rename()` actually says/does this: |
There was a problem hiding this comment.
You should be able to CI test this.
| opts.new_name:sub(4, #opts.new_name), | ||
| } | ||
| end | ||
| -- Cannot rename a non-existing path (lstat is needed here, `Path:exists()` |
There was a problem hiding this comment.
I dont understand the difference to lstat from this text. Do you mean uv.lstat? Do I need to look into Path:exit()?
| -- a different file. | ||
| -- | ||
| -- NOTE: To elaborate, `uv.fs_rename()` won't/shouldn't do anything if old | ||
| -- and new both exist and are both hard links to the same file (inode), |
| -- | ||
| -- NOTE: To elaborate, `uv.fs_rename()` won't/shouldn't do anything if old | ||
| -- and new both exist and are both hard links to the same file (inode), | ||
| -- however, it appears to still allow you to change the case of a filename |
| -- NOTE: To elaborate, `uv.fs_rename()` won't/shouldn't do anything if old | ||
| -- and new both exist and are both hard links to the same file (inode), | ||
| -- however, it appears to still allow you to change the case of a filename | ||
| -- on case-insensitive file systems (i.e. if `new_name` doesn't _actually_ |
There was a problem hiding this comment.
why the word actually? This reads like a code example would be much much simpler.
lua/plenary/path.lua
Outdated
| assert(status, ("%s: Rename failed!"):format(errmsg)) | ||
|
|
||
| -- NOTE: `uv.fs_rename()` _can_ return success even if no rename actually | ||
| -- occurred (see rename(2)), and this is not an error. So we're not changing |
There was a problem hiding this comment.
why is this not an error? This does not describe how it does still uphold the users expectation of the api.
|
|
||
| -- NOTE: `uv.fs_rename()` _can_ return success even if no rename actually | ||
| -- occurred (see rename(2)), and this is not an error. | ||
| return Path:new(new_path) |
There was a problem hiding this comment.
Due to the implementation of Path:new, this actually ends up returning opts.new_name (i.e. the very same Path instance) if a Path was passed as the argument. What should we do here?
- Leave it as is?
- Same, but force a new Path instance to be created?
- Revert to the previous behavior of mutating
selfby modifyingself.filename(in which case we'd probably want to returnself)?
Python's Path.rename() does number 2.
Improve
Path:rename(). Mainly just focuses on fixing a few bugs that were in the implementation, but also adds types/annotations, comments, doc comments, etc., and brings the behavior much closer in line with that of Python'sPath.rename().Included in this pr is a change which could be considered breaking: upon successful rename,
Path:rename()now returns a new path instance instead of changingPath.filenameofself. This is how python does it, but I can change this back to how it was.Fixes #484
Fixes bugs introduced in #90
TODO