Skip to content

fix(path): improve & fix bugs in Path:rename() #485

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 54 additions & 13 deletions lua/plenary/path.lua
Original file line number Diff line number Diff line change
Expand Up @@ -515,30 +515,71 @@ function Path:rmdir()
uv.fs_rmdir(self:absolute())
end

---Rename this file or directory to the given path (`opts.new_name`), and
---return a new Path instance pointing to it. Relative paths are interpreted
---relative to the current working directory. The rename is aborted if the new
---path already exists.
---@generic T: Path
---@param opts { new_name: Path|string }
---@return T
function Path:rename(opts)
-- TODO: For reference, Python's `Path.rename()` actually says/does this:
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to CI test this.

--
-- > On Unix, if target exists and is a file, it will be replaced silently
-- > if the user has permission.
-- >
-- > On Windows, if target exists, FileExistsError will be raised. target
-- > can be either a string or another path object.
--
-- The behavior here may differ, as an error will be thrown regardless.

local self_lstat, new_lstat, status, errmsg
opts = opts or {}
if not opts.new_name or opts.new_name == "" then
error "Please provide the new name!"
end
assert(opts.new_name and opts.new_name ~= "", "Please provide the new name!")
self_lstat, errmsg = uv.fs_lstat(self.filename)

-- Cannot rename a non-existing path (lstat is needed here, `Path:exists()`
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont understand the difference to lstat from this text. Do you mean uv.lstat? Do I need to look into Path:exit()?

-- uses stat)
assert(self_lstat, ("%s: %s"):format(errmsg, self.filename))

-- 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, #opts.new_name),
opts.new_name:sub(4),
}
end

local new_path = Path:new(opts.new_name)

if new_path:exists() then
error "File or directory already exists!"
end
Comment on lines -534 to -536
Copy link
Author

@tmillr tmillr May 3, 2023

Choose a reason for hiding this comment

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

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).


local status = uv.fs_rename(self:absolute(), new_path:absolute())
self.filename = new_path.filename

return status
new_lstat, errmsg = uv.fs_lstat(new_path.filename)

-- This allows changing only case (e.g. fname -> Fname) on case-insensitive
-- file systems, otherwise throwing if `new_name` exists as a different file.
--
-- NOTE: to elaborate, `uv.fs_rename()` wont/shouldn't do anything if old
-- and new both exist and are both hard links to the same file (inode),
Copy link
Contributor

Choose a reason for hiding this comment

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

missing word: new both?

-- however, it appears to still allow you to change the case of a filename
Copy link
Contributor

Choose a reason for hiding this comment

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

s/allow/allows

-- on case-insensitive file systems (e.g. if `new_name` doesn't _actually_
-- exist as a separate file but would otherwise appear to via an lstat call;
-- if it does actually exist (in which case the fs must be case-sensitive)
-- idk 100% what happens b/c it needs to be tested on a case-sensitive fs,
-- but it should simply result in a successful no-op according to rename(2)
-- docs, at least on Linux anyway)
assert(not new_lstat or (self_lstat.ino == new_lstat.ino), "File or directory already exists!")

status, errmsg = uv.fs_rename(self:absolute(), new_path:absolute())
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...we're not changing
-- `self.filename` if it didn't.
if not uv.fs_lstat(self.filename) then
self.filename = new_path.filename
end

-- TODO: Python returns a brand new instance here, should we do the same?
return self
end

--- Copy files or folders with defaults akin to GNU's `cp`.
Expand Down