#18216 make moveDir work across partitions on windows (#18223)

* return false if AccessDeniedError in tryMoveFSObject - fixes #18216

* add moveDir & moveFile tests

* rename `isMoveDir` parameter to `isDir`

lib/pure/os.nim

@@ -2009,28 +2009,32 @@ proc removeFile*(file: string) {.rtl, extern: "nos$1", tags: [WriteDirEffect], n
   if not tryRemoveFile(file):
     raiseOSError(osLastError(), file)
 
-proc tryMoveFSObject(source, dest: string): bool {.noWeirdTarget.} =
-  ## Moves a file or directory from `source` to `dest`.
+proc tryMoveFSObject(source, dest: string, isDir: bool): bool {.noWeirdTarget.} =
+  ## Moves a file (or directory if `isDir` is true) from `source` to `dest`.
   ##
-  ## Returns false in case of `EXDEV` error.
+  ## Returns false in case of `EXDEV` error or `AccessDeniedError` on windows (if `isDir` is true).
   ## In case of other errors `OSError` is raised.
   ## Returns true in case of success.
   when defined(windows):
     when useWinUnicode:
       let s = newWideCString(source)
       let d = newWideCString(dest)
-      if moveFileExW(s, d, MOVEFILE_COPY_ALLOWED or MOVEFILE_REPLACE_EXISTING) == 0'i32: raiseOSError(osLastError(), $(source, dest))
+      result = moveFileExW(s, d, MOVEFILE_COPY_ALLOWED or MOVEFILE_REPLACE_EXISTING) != 0'i32
     else:
-      if moveFileExA(source, dest, MOVEFILE_COPY_ALLOWED or MOVEFILE_REPLACE_EXISTING) == 0'i32: raiseOSError(osLastError(), $(source, dest))
+      result = moveFileExA(source, dest, MOVEFILE_COPY_ALLOWED or MOVEFILE_REPLACE_EXISTING) != 0'i32
   else:
-    if c_rename(source, dest) != 0'i32:
-      let err = osLastError()
-      if err == EXDEV.OSErrorCode:
-        return false
+    result = c_rename(source, dest) == 0'i32
+
+  if not result:
+    let err = osLastError()
+    let isAccessDeniedError = 
+      when defined(windows):
+        const AccessDeniedError = OSErrorCode(5)
+        isDir and err == AccessDeniedError
       else:
-        # see whether `strerror(errno)` is redundant with what raiseOSError already shows
-        raiseOSError(err, $(source, dest, strerror(errno)))
-  return true
+        err == EXDEV.OSErrorCode
+    if not isAccessDeniedError:
+      raiseOSError(err, $(source, dest))
 
 proc moveFile*(source, dest: string) {.rtl, extern: "nos$1",
   tags: [ReadDirEffect, ReadIOEffect, WriteIOEffect], noWeirdTarget.} =
@@ -2051,8 +2055,10 @@ proc moveFile*(source, dest: string) {.rtl, extern: "nos$1",
   ## * `removeFile proc <#removeFile,string>`_
   ## * `tryRemoveFile proc <#tryRemoveFile,string>`_
 
-  if not tryMoveFSObject(source, dest):
-    when not defined(windows):
+  if not tryMoveFSObject(source, dest, isDir = false):
+    when defined(windows):
+      doAssert false
+    else:
       # Fallback to copy & del
       copyFile(source, dest, {cfSymlinkAsIs})
       try:
@@ -2060,6 +2066,7 @@ proc moveFile*(source, dest: string) {.rtl, extern: "nos$1",
       except:
         discard tryRemoveFile(dest)
         raise
+      
 
 proc exitStatusLikeShell*(status: cint): cint =
   ## Converts exit code from `c_system` into a shell exit code.
@@ -2614,11 +2621,10 @@ proc moveDir*(source, dest: string) {.tags: [ReadIOEffect, WriteIOEffect], noWei
   ## * `removeDir proc <#removeDir,string>`_
   ## * `existsOrCreateDir proc <#existsOrCreateDir,string>`_
   ## * `createDir proc <#createDir,string>`_
-  if not tryMoveFSObject(source, dest):
-    when not defined(windows):
-      # Fallback to copy & del
-      copyDir(source, dest)
-      removeDir(source)
+  if not tryMoveFSObject(source, dest, isDir = true):
+    # Fallback to copy & del
+    copyDir(source, dest)
+    removeDir(source)
 
 proc createHardlink*(src, dest: string) {.noWeirdTarget.} =
   ## Create a hard link at `dest` which points to the item specified

tests/stdlib/tos.nim

@@ -261,6 +261,51 @@ block fileOperations:
 
     removeDir(dname)
 
+block: # moveFile
+  let tempDir = getTempDir() / "D20210609T151608"
+  createDir(tempDir)
+  defer: removeDir(tempDir)
+
+  writeFile(tempDir / "a.txt", "")
+  moveFile(tempDir / "a.txt", tempDir / "b.txt")
+  doAssert not fileExists(tempDir / "a.txt")
+  doAssert fileExists(tempDir / "b.txt")
+  removeFile(tempDir / "b.txt")
+
+  createDir(tempDir / "moveFile_test")
+  writeFile(tempDir / "moveFile_test/a.txt", "")
+  moveFile(tempDir / "moveFile_test/a.txt", tempDir / "moveFile_test/b.txt")
+  doAssert not fileExists(tempDir / "moveFile_test/a.txt")
+  doAssert fileExists(tempDir / "moveFile_test/b.txt")
+  removeDir(tempDir / "moveFile_test")
+
+  createDir(tempDir / "moveFile_test")
+  writeFile(tempDir / "a.txt", "")
+  moveFile(tempDir / "a.txt", tempDir / "moveFile_test/b.txt")
+  doAssert not fileExists(tempDir / "a.txt")
+  doAssert fileExists(tempDir / "moveFile_test/b.txt")
+  removeDir(tempDir / "moveFile_test")
+
+block: # moveDir
+  let tempDir = getTempDir() / "D20210609T161443"
+  createDir(tempDir)
+  defer: removeDir(tempDir)
+
+  createDir(tempDir / "moveDir_test")
+  moveDir(tempDir / "moveDir_test/", tempDir / "moveDir_test_dest")
+  doAssert not dirExists(tempDir / "moveDir_test")
+  doAssert dirExists(tempDir / "moveDir_test_dest")
+  removeDir(tempDir / "moveDir_test_dest")
+
+  createDir(tempDir / "moveDir_test")
+  writeFile(tempDir / "moveDir_test/a.txt", "")
+  moveDir(tempDir / "moveDir_test", tempDir / "moveDir_test_dest")
+  doAssert not dirExists(tempDir / "moveDir_test")
+  doAssert not fileExists(tempDir / "moveDir_test/a.txt")
+  doAssert dirExists(tempDir / "moveDir_test_dest")
+  doAssert fileExists(tempDir / "moveDir_test_dest/a.txt")
+  removeDir(tempDir / "moveDir_test_dest")
+
 import times
 block modificationTime:
   # Test get/set modification times