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

lua/plenary/path.lua

@@ -515,30 +515,69 @@ function Path:rmdir()
   uv.fs_rmdir(self:absolute())
 end
 
+---Rename this file or directory to the provided path (`opts.new_name`),
+---returning a new Path instance upon success.  The rename is aborted if the
+---new path already exists. Relative paths are interpreted relative to the
+---current working directory.
+---@param opts { new_name: Path|string } options table containing the new name
+---@return Path # Path representing the new name
 function Path:rename(opts)
+  -- TODO: For reference, Python's `Path.rename()` actually says/does 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
+  vim.validate { opts = { opts, "t" } }
+  vim.validate {
+    ["opts.new_name"] = {
+      opts.new_name,
+      function(val)
+        return Path.is_path(val) or (type(val) == "string" and val ~= "")
+      end,
+      "string or Path object",
+    },
+  }
   opts = opts or {}
-  if not opts.new_name or opts.new_name == "" then
-    error "Please provide the new name!"
-  end
+  self_lstat, errmsg = uv.fs_lstat(self.filename)
 
-  -- 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),
-    }
-  end
+  -- Cannot rename a non-existing path (lstat is needed here, `Path:exists()`
+  -- uses stat)
+  assert(self_lstat, ("%s: %s"):format(errmsg, self.filename))
 
   local new_path = Path:new(opts.new_name)
-
-  if new_path:exists() then
-    error "File or directory already exists!"
-  end
-
-  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)
+  local same_inode = false
+  if new_lstat then
+    same_inode = self_lstat.ino == new_lstat.ino and self_lstat.dev == new_lstat.dev and self_lstat.gen == new_lstat.gen
+  end
+
+  -- The following 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()` 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_
+  -- 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 for certain 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 the
+  -- `rename(2)` docs, at least on Linux anyway).
+  assert(not new_lstat or same_inode, "File or directory already exists!")
+
+  status, errmsg = uv.fs_rename(tostring(self), tostring(new_path))
+  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.
+  return Path:new(new_path)
 end
 
 --- Copy files or folders with defaults akin to GNU's `cp`.
@@ -560,11 +599,14 @@ function Path:copy(opts)
   local dest = opts.destination
   -- handles `.`, `..`, `./`, and `../`
   if not Path.is_path(dest) then
-    if type(dest) == "string" and dest:match "^%.%.?/?\\?.+" then
-      dest = {
-        uv.fs_realpath(dest:sub(1, 3)),
-        dest:sub(4, #dest),
-      }
+    if type(dest) == "string" then
+      local m = dest:match "^%.%.?/?\\?.+"
+      if m then
+        dest = {
+          uv.fs_realpath(dest:sub(1, #m)),
+          dest:sub(#m + 1),
+        }
+      end
     end
     dest = Path:new(dest)
   end

tests/plenary/path_spec.lua

@@ -1,6 +1,38 @@
 local Path = require "plenary.path"
 local path = Path.path
 
+---Construct a temporary environment which keeps track of which Paths have
+---been created for easier cleanup. Use `new_path()` to construct a Path.
+local function new_env()
+  local env, trash = {}, {}
+
+  ---Create and return a new Path instance with a non-existing temporary filename,
+  ---or `opts.filename` if provided. The temporary filename will be an absolute path.
+  ---Pass `opts.touch` to create the file as well.
+  function env.new_path(opts)
+    opts = opts or {}
+    local ret = Path:new(opts.filename or vim.fn.tempname())
+    if opts.touch then
+      ret:touch()
+      assert(ret:exists())
+    end
+    table.insert(trash, ret)
+    return ret
+  end
+
+  ---Remove from the filesystem all paths created by `new_path()`. A
+  ---reference to this function may be passed directly to `after_each()`.
+  function env.cleanup()
+    for _, v in ipairs(trash) do
+      if type((v or {}).rm) == "function" then
+        pcall(v.rm, v)
+      end
+    end
+  end
+
+  return env
+end
+
 describe("Path", function()
   it("should find valid files", function()
     local p = Path:new "README.md"
@@ -390,53 +422,130 @@ describe("Path", function()
   end)
 
   describe("rename", function()
-    it("can rename a file", function()
-      local p = Path:new "a_random_filename.lua"
-      assert(pcall(p.touch, p))
-      assert(p:exists())
-
-      assert(pcall(p.rename, p, { new_name = "not_a_random_filename.lua" }))
-      assert.are.same("not_a_random_filename.lua", p.filename)
-
-      p:rm()
-    end)
-
-    it("can handle an invalid filename", function()
-      local p = Path:new "some_random_filename.lua"
-      assert(pcall(p.touch, p))
-      assert(p:exists())
+    local env = new_env()
+    after_each(env.cleanup)
 
-      assert(not pcall(p.rename, p, { new_name = "" }))
-      assert(not pcall(p.rename, p))
-      assert.are.same("some_random_filename.lua", p.filename)
-
-      p:rm()
+    it("can rename a file", function()
+      local before, after = env.new_path { touch = true }, env.new_path()
+      -- Can pass another Path object
+      before:rename { new_name = after }
+      assert.is.False(before:exists())
+      assert.is.True(after:exists())
+      before, after = env.new_path { touch = true }, env.new_path()
+      -- Also works with string
+      before:rename { new_name = after.filename }
+      assert.is.False(before:exists())
+      assert.is.True(after:exists())
+    end)
+
+    it("should throw on invalid args", function()
+      local before = env.new_path { touch = true }
+      assert.errors(function()
+        before:rename { new_name = "" }
+      end)
+      assert.errors(function()
+        before:rename {}
+      end)
+      assert.errors(function()
+        before:rename()
+      end)
+      assert.is.True(before:exists())
+    end)
+
+    it("should throw if old name doesn't exist", function()
+      local before, after = env.new_path { touch = false }, env.new_path { touch = false }
+      assert.errors(function()
+        before:rename { new_name = after }
+      end)
     end)
 
     it("can move to parent dir", function()
-      local p = Path:new "some_random_filename.lua"
-      assert(pcall(p.touch, p))
-      assert(p:exists())
-
-      assert(pcall(p.rename, p, { new_name = "../some_random_filename.lua" }))
-      assert.are.same(vim.loop.fs_realpath(Path:new("../some_random_filename.lua"):absolute()), p:absolute())
-
-      p:rm()
+      local before, after = env.new_path { filename = "random_file" }, env.new_path { filename = "../random_file" }
+      assert.is.False(before:exists())
+      assert.is.False(after:exists())
+      before:touch()
+      assert.is.True(before:exists())
+      before:rename { new_name = after }
+      assert.is.False(before:exists())
+      assert.is.True(after:exists())
+    end)
+
+    it("should throw on rename to existing filename", function()
+      local before, after = env.new_path { touch = true }, env.new_path { touch = true }
+      assert.errors(function()
+        before:rename { new_name = after }
+      end)
+    end)
+
+    it("shouldn't throw on rename to same filename", function()
+      local before = env.new_path { touch = true }
+      assert.does.Not.error(function()
+        before:rename { new_name = before }
+      end)
+    end)
+
+    it("should handle . or .. or ./ or ../ prefix", function()
+      for _, pre in ipairs { ".", "..", "./", "../" } do
+        local before, after =
+          env.new_path { filename = "random_file" }, env.new_path { filename = pre .. "random_file2" }
+        assert.is.False(before:exists())
+        assert.is.False(after:exists())
+        before:touch()
+        assert.is.True(before:exists())
+        before:rename { new_name = after }
+        assert.is.False(before:exists())
+        assert.is.True(after:exists())
+      end
     end)
 
-    it("cannot rename to an existing filename", function()
-      local p1 = Path:new "a_random_filename.lua"
-      local p2 = Path:new "not_a_random_filename.lua"
-      assert(pcall(p1.touch, p1))
-      assert(pcall(p2.touch, p2))
-      assert(p1:exists())
-      assert(p2:exists())
-
-      assert(not pcall(p1.rename, p1, { new_name = "not_a_random_filename.lua" }))
-      assert.are.same(p1.filename, "a_random_filename.lua")
-
-      p1:rm()
-      p2:rm()
+    it("should consider bad symlink as existing, and throw", function()
+      local before, after, non_existing = env.new_path { touch = true }, env.new_path(), env.new_path()
+      assert(vim.loop.fs_symlink(non_existing.filename, after.filename))
+      assert.is.True(not not vim.loop.fs_lstat(after.filename))
+      assert.errors(function()
+        before:rename { new_name = after }
+      end)
+    end)
+
+    it("should return result as new Path instance with the new filename", function()
+      local before, after = env.new_path { touch = true }, env.new_path()
+      local before_filename = before.filename
+      local new = before:rename { new_name = after }
+      assert.is.False(before:exists())
+      assert.is.True(after:exists())
+      assert.is.True(Path.is_path(new))
+      assert.is.True(new:exists())
+      assert.are.equal(before.filename, before_filename)
+      assert.are.equal(after.filename, new.filename)
+    end)
+
+    it("should allow changing only case of filename, regardless of fs case-sensitivity", function()
+      local before = env.new_path { filename = ".__some_file" }
+      assert.is.False(before:exists())
+      before:touch()
+      local before_filename_realpath = assert(vim.loop.fs_realpath(before.filename))
+      local after = env.new_path { filename = before.filename:upper() }
+      assert.does.Not.error(function()
+        before:rename { new_name = after }
+      end)
+      assert.are.equal(
+        before_filename_realpath:sub(1, #before_filename_realpath - #before.filename) .. after.filename,
+        assert(vim.loop.fs_realpath(after.filename))
+      )
+    end)
+
+    it("rename to hardlink of the same file should be a successful no-op", function()
+      local before, after = env.new_path { touch = true }, env.new_path {}
+      assert(vim.loop.fs_link(before.filename, after.filename))
+      assert.is.True(after:exists())
+      local new
+      assert.does.Not.error(function()
+        new = before:rename { new_name = after }
+      end)
+      assert.is.True(before:exists())
+      assert.is.True(after:exists())
+      assert.is.True(Path.is_path(new))
+      assert.are.equal(new.filename, after.filename)
     end)
   end)