Improvements to the various file system programs (rm, mv, rename) (#440)

This enforces several sanity checks before actually attempting
the move, allowing us to produce friendlier error messages.

doc/stub/fs.lua

@@ -19,6 +19,18 @@ function getFreeSpace(path) end
 function find(pattern) end
 function getDir(path) end
 
+--- Returns true if a path is mounted to the parent filesystem.
+--
+-- The root filesystem "/" is considered a mount, along with disk folders and
+-- the rom folder. Other programs (such as network shares) can exstend this to
+-- make other mount types by correctly assigning their return value for getDrive.
+--
+-- @tparam string path The path to check.
+-- @treturn boolean If the path is mounted, rather than a normal file/folder.
+-- @throws If the path does not exist.
+-- @see getDrive
+function isDriveRoot(path) end
+
 --- Get the capacity of the drive at the given path.
 --
 -- This may be used in conjunction with @{getFreeSpace} to determine what

src/main/resources/assets/computercraft/lua/bios.lua

@@ -797,6 +797,12 @@ function fs.complete(sPath, sLocation, bIncludeFiles, bIncludeDirs)
     return tEmpty
 end
 
+function fs.isDriveRoot(sPath)
+    expect(1, sPath, "string")
+    -- Force the root directory to be a mount.
+    return fs.getDir(sPath) == ".." or fs.getDrive(sPath) ~= fs.getDrive(fs.getDir(sPath))
+end
+
 -- Load APIs
 local bAPIError = false
 local tApis = fs.list("rom/apis")

src/main/resources/assets/computercraft/lua/rom/programs/delete.lua

@@ -9,9 +9,18 @@ for i = 1, args.n do
     local files = fs.find(shell.resolve(args[i]))
     if #files > 0 then
         for _, file in ipairs(files) do
-            local ok, err = pcall(fs.delete, file)
-            if not ok then
-                printError((err:gsub("^pcall: ", "")))
+            if fs.isReadOnly(file) then
+                printError("Cannot delete read-only file /" .. file)
+            elseif fs.isDriveRoot(file) then
+                printError("Cannot delete mount /" .. file)
+                if fs.isDir(file) then
+                    print("To delete its contents run rm /" .. fs.combine(file, "*"))
+                end
+            else
+                local ok, err = pcall(fs.delete, file)
+                if not ok then
+                    printError((err:gsub("^pcall: ", "")))
+                end
             end
         end
     else

src/main/resources/assets/computercraft/lua/rom/programs/move.lua

@@ -7,12 +7,35 @@ end
 local sSource = shell.resolve(tArgs[1])
 local sDest = shell.resolve(tArgs[2])
 local tFiles = fs.find(sSource)
+
+local function sanity_checks(source, dest)
+    if fs.exists(dest) then
+        printError("Destination exists")
+        return false
+    elseif fs.isReadOnly(dest) then
+        printError("Destination is read-only")
+        return false
+    elseif fs.isDriveRoot(source) then
+        printError("Cannot move mount /" .. source)
+        return false
+    elseif fs.isReadOnly(source) then
+        printError("Cannot move read-only file /" .. source)
+        return false
+    end
+    return true
+end
+
 if #tFiles > 0 then
     for _, sFile in ipairs(tFiles) do
         if fs.isDir(sDest) then
-            fs.move(sFile, fs.combine(sDest, fs.getName(sFile)))
+            local dest = fs.combine(sDest, fs.getName(sFile))
+            if sanity_checks(sFile, dest) then
+                fs.move(sFile, dest)
+            end
         elseif #tFiles == 1 then
-            fs.move(sFile, sDest)
+            if sanity_checks(sFile, sDest) then
+                fs.move(sFile, sDest)
+            end
         else
             printError("Cannot overwrite file multiple times")
             return

src/main/resources/assets/computercraft/lua/rom/programs/rename.lua

@@ -10,6 +10,12 @@ local sDest = shell.resolve(tArgs[2])
 if not fs.exists(sSource) then
     printError("No matching files")
     return
+elseif fs.isDriveRoot(sSource) then
+    printError("Can't rename mounts")
+    return
+elseif fs.isReadOnly(sSource) then
+    printError("Source is read-only")
+    return
 elseif fs.exists(sDest) then
     printError("Destination exists")
     return

src/test/resources/test-rom/spec/apis/fs_spec.lua

@@ -12,6 +12,21 @@ describe("The fs library", function()
         end)
     end)
 
+    describe("fs.isDriveRoot", function()
+        it("validates arguments", function()
+            fs.isDriveRoot("")
+
+            expect.error(fs.isDriveRoot, nil):eq("bad argument #1 (expected string, got nil)")
+        end)
+
+        it("correctly identifies drive roots", function()
+            expect(fs.isDriveRoot("/rom")):eq(true)
+            expect(fs.isDriveRoot("/")):eq(true)
+            expect(fs.isDriveRoot("/rom/startup.lua")):eq(false)
+            expect(fs.isDriveRoot("/rom/programs/delete.lua")):eq(false)
+        end)
+    end)
+
     describe("fs.list", function()
         it("fails on files", function()
             expect.error(fs.list, "rom/startup.lua"):eq("/rom/startup.lua: Not a directory")

src/test/resources/test-rom/spec/programs/delete_spec.lua

@@ -42,7 +42,15 @@ describe("The rm program", function()
 
     it("errors when trying to delete a read-only file", function()
         expect(capture(stub, "rm /rom/startup.lua"))
-            :matches { ok = true, output = "", error = "/rom/startup.lua: Access denied\n" }
+            :matches { ok = true, output = "", error = "Cannot delete read-only file /rom/startup.lua\n" }
+    end)
+
+    it("errors when trying to delete the root mount", function()
+        expect(capture(stub, "rm /")):matches {
+            ok = true,
+            output = "To delete its contents run rm /*\n",
+            error = "Cannot delete mount /\n",
+        }
     end)
 
     it("errors when a glob fails to match", function()

src/test/resources/test-rom/spec/programs/move_spec.lua

@@ -1,11 +1,13 @@
 local capture = require "test_helpers".capture_program
 
 describe("The move program", function()
+    local function cleanup() fs.delete("/test-files/move") end
     local function touch(file)
         io.open(file, "w"):close()
     end
 
     it("move a file", function()
+        cleanup()
         touch("/test-files/move/a.txt")
 
         shell.run("move /test-files/move/a.txt /test-files/move/b.txt")
@@ -14,11 +16,57 @@ describe("The move program", function()
         expect(fs.exists("/test-files/move/b.txt")):eq(true)
     end)
 
-    it("try to move a not existing file", function()
+    it("moves a file to a directory", function()
+        cleanup()
+        touch("/test-files/move/a.txt")
+        fs.makeDir("/test-files/move/a")
+
+        expect(capture(stub, "move /test-files/move/a.txt /test-files/move/a"))
+            :matches { ok = true }
+
+        expect(fs.exists("/test-files/move/a.txt")):eq(false)
+        expect(fs.exists("/test-files/move/a/a.txt")):eq(true)
+    end)
+
+    it("fails when moving a file which doesn't exist", function()
         expect(capture(stub, "move nothing destination"))
             :matches { ok = true, output = "", error = "No matching files\n" }
     end)
 
+    it("fails when overwriting an existing file", function()
+        cleanup()
+        touch("/test-files/move/a.txt")
+
+        expect(capture(stub, "move /test-files/move/a.txt /test-files/move/a.txt"))
+            :matches { ok = true, output = "", error = "Destination exists\n" }
+    end)
+
+    it("fails when moving to read-only locations", function()
+        cleanup()
+        touch("/test-files/move/a.txt")
+
+        expect(capture(stub, "move /test-files/move/a.txt /rom/test.txt"))
+            :matches { ok = true, output = "", error = "Destination is read-only\n" }
+    end)
+
+    it("fails when moving from read-only locations", function()
+        expect(capture(stub, "move /rom/startup.lua /test-files/move/not-exist.txt"))
+            :matches { ok = true, output = "", error = "Cannot move read-only file /rom/startup.lua\n" }
+    end)
+
+    it("fails when moving mounts", function()
+        expect(capture(stub, "move /rom /test-files/move/rom"))
+            :matches { ok = true, output = "", error = "Cannot move mount /rom\n" }
+    end)
+
+    it("fails when moving a file multiple times", function()
+        cleanup()
+        touch("/test-files/move/a.txt")
+        touch("/test-files/move/b.txt")
+        expect(capture(stub, "move /test-files/move/*.txt /test-files/move/c.txt"))
+            :matches { ok = true, output = "", error = "Cannot overwrite file multiple times\n" }
+    end)
+
     it("displays the usage with no arguments", function()
         expect(capture(stub, "move"))
             :matches { ok = true, output = "Usage: mv <source> <destination>\n", error = "" }

src/test/resources/test-rom/spec/programs/rename_spec.lua

@@ -26,13 +26,23 @@ describe("The rename program", function()
             :matches { ok = true, output = "", error = "Destination exists\n" }
     end)
 
-    it("fails when copying to read-only locations", function()
+    it("fails when renaming to read-only locations", function()
         touch("/test-files/rename/d.txt")
 
         expect(capture(stub, "rename /test-files/rename/d.txt /rom/test.txt"))
             :matches { ok = true, output = "", error = "Destination is read-only\n" }
     end)
 
+    it("fails when renaming from read-only locations", function()
+        expect(capture(stub, "rename /rom/startup.lua /test-files/rename/d.txt"))
+            :matches { ok = true, output = "", error = "Source is read-only\n" }
+    end)
+
+    it("fails when renaming mounts", function()
+        expect(capture(stub, "rename /rom /test-files/rename/rom"))
+            :matches { ok = true, output = "", error = "Can't rename mounts\n" }
+    end)
+
     it("displays the usage when given no arguments", function()
         expect(capture(stub, "rename"))
             :matches { ok = true, output = "Usage: rename <source> <destination>\n", error = "" }