Fix Windows symlink handling in FileManager APIs (#858)

* Fix Windows symlink handling in FileManager APIs

* Address feedback

Author: jmschonfeld

Sources/FoundationEssentials/FileManager/FileManager+Basics.swift

@@ -78,14 +78,14 @@ internal struct _FileManagerImpl {
     ) -> Bool {
 #if os(Windows)
         return (try? path.withNTPathRepresentation {
-            let hLHS = CreateFileW($0, GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nil, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nil)
+            let hLHS = CreateFileW($0, GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nil, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS, nil)
             if hLHS == INVALID_HANDLE_VALUE {
                 return false
             }
             defer { CloseHandle(hLHS) }
 
             return (try? other.withNTPathRepresentation {
-                let hRHS = CreateFileW($0, GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nil, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nil)
+                let hRHS = CreateFileW($0, GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nil, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS, nil)
                 if hRHS == INVALID_HANDLE_VALUE {
                     return false
                 }
@@ -122,11 +122,21 @@ internal struct _FileManagerImpl {
                     return false
                 }
 
-                if fbiLHS.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT,
-                   fbiRHS.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT {
+                let lhsIsReparsePoint = fbiLHS.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT
+                let rhsIsReparsePoint = fbiRHS.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT
+                let lhsIsDirectory = fbiLHS.FileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY
+                let rhsIsDirectory = fbiRHS.FileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY
+                
+                guard lhsIsReparsePoint == rhsIsReparsePoint, lhsIsDirectory == rhsIsDirectory else {
+                    // If they aren't the same "type", then they cannot be equivalent
+                    return false
+                }
+                
+                if lhsIsReparsePoint {
+                    // Both are symbolic links, so they are equivalent if their destinations are equivalent
                     return (try? fileManager.destinationOfSymbolicLink(atPath: path) == fileManager.destinationOfSymbolicLink(atPath: other)) ?? false
-                } else if fbiLHS.FileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY,
-                          fbiRHS.FileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY {
+                } else if lhsIsDirectory {
+                    // Both are directories, so recursively compare the directories
                     guard let aLHSItems = try? fileManager.contentsOfDirectory(atPath: path),
                           let aRHSItems = try? fileManager.contentsOfDirectory(atPath: other),
                           aLHSItems == aRHSItems else {
@@ -153,6 +163,7 @@ internal struct _FileManagerImpl {
 
                     return true
                 } else {
+                    // Both must be standard files, so binary compare the contents of the files
                     var liLHSSize: LARGE_INTEGER = .init()
                     var liRHSSize: LARGE_INTEGER = .init()
                     guard GetFileSizeEx(hLHS, &liLHSSize), GetFileSizeEx(hRHS, &liRHSSize), LARGE_INTEGER._equals(liLHSSize, liRHSSize) else {

Sources/FoundationEssentials/FileManager/FileOperations.swift

@@ -817,7 +817,16 @@ enum _FileOperations {
             guard delegate.shouldPerformOnItemAtPath(src, to: dst) else { return }
 
             try dst.withNTPathRepresentation { pwszDestination in
-                if faAttributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY {
+                // Check for reparse points first because symlinks to directories are reported as both reparse points and directories, and we should copy the symlink not the contents of the linked directory
+                if faAttributes.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT {
+                    do {
+                        let linkDest = try fileManager.destinationOfSymbolicLink(atPath: src)
+                        try fileManager.createSymbolicLink(atPath: dst, withDestinationPath: linkDest)
+                    } catch {
+                        try delegate.throwIfNecessary(error, src, dst)
+                        return
+                    }
+                } else if faAttributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY {
                     do {
                         try fileManager.createDirectory(atPath: dst, withIntermediateDirectories: true)
                     } catch {
@@ -826,10 +835,10 @@ enum _FileOperations {
                     for item in _Win32DirectoryContentsSequence(path: src, appendSlashForDirectory: true) {
                         try linkOrCopyFile(src.appendingPathComponent(item.fileName), dst: dst.appendingPathComponent(item.fileName), with: fileManager, delegate: delegate)
                     }
-                } else if bCopyFile || faAttributes.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT {
+                } else if bCopyFile {
                     var ExtendedParameters: COPYFILE2_EXTENDED_PARAMETERS = .init()
                     ExtendedParameters.dwSize = DWORD(MemoryLayout<COPYFILE2_EXTENDED_PARAMETERS>.size)
-                    ExtendedParameters.dwCopyFlags = COPY_FILE_FAIL_IF_EXISTS | COPY_FILE_COPY_SYMLINK | COPY_FILE_NO_BUFFERING | COPY_FILE_OPEN_AND_COPY_REPARSE_POINT
+                    ExtendedParameters.dwCopyFlags = COPY_FILE_FAIL_IF_EXISTS | COPY_FILE_NO_BUFFERING
 
                     if FAILED(CopyFile2(pwszSource, pwszDestination, &ExtendedParameters)) {
                         try delegate.throwIfNecessary(GetLastError(), src, dst)

Tests/FoundationEssentialsTests/FileManager/FileManagerPlayground.swift

@@ -46,6 +46,22 @@ struct File : ExpressibleByStringLiteral, Buildable {
     }
 }
 
+struct SymbolicLink : Buildable {
+    fileprivate let name: String
+    private let destination: String
+    
+    init(_ name: String, destination: String) {
+        self.name = name
+        self.destination = destination
+    }
+    
+    fileprivate func build(in path: String, using fileManager: FileManager) throws {
+        let linkPath = path.appendingPathComponent(name)
+        let destPath = path.appendingPathComponent(destination)
+        try fileManager.createSymbolicLink(atPath: linkPath, withDestinationPath: destPath)
+    }
+}
+
 struct Directory : Buildable {
     fileprivate let name: String
     private let attributes: [FileAttributeKey : Any]?
@@ -70,11 +86,13 @@ struct FileManagerPlayground {
     enum Item : Buildable {
         case file(File)
         case directory(Directory)
+        case symbolicLink(SymbolicLink)
 
         fileprivate func build(in path: String, using fileManager: FileManager) throws {
             switch self {
             case let .file(file): try file.build(in: path, using: fileManager)
             case let .directory(dir): try dir.build(in: path, using: fileManager)
+            case let .symbolicLink(symlink): try symlink.build(in: path, using: fileManager)
             }
         }
     }
@@ -92,6 +110,10 @@ struct FileManagerPlayground {
         static func buildExpression(_ expression: Directory) -> Item {
             .directory(expression)
         }
+        
+        static func buildExpression(_ expression: SymbolicLink) -> Item {
+            .symbolicLink(expression)
+        }
     }
 
     private let directory: Directory

Tests/FoundationEssentialsTests/FileManager/FileManagerTests.swift

@@ -214,10 +214,20 @@ final class FileManagerTests : XCTestCase {
                     File("Baz", contents: randomData())
                 }
             }
+            Directory("symlinks") {
+                File("Foo", contents: randomData())
+                SymbolicLink("LinkToFoo", destination: "Foo")
+            }
+            Directory("EmptyDirectory") {}
+            "EmptyFile"
         }.test {
             XCTAssertTrue($0.contentsEqual(atPath: "dir1", andPath: "dir1_copy"))
             XCTAssertFalse($0.contentsEqual(atPath: "dir1/dir2", andPath: "dir1/dir3"))
             XCTAssertFalse($0.contentsEqual(atPath: "dir1", andPath: "dir1_diffdata"))
+            XCTAssertFalse($0.contentsEqual(atPath: "symlinks/LinkToFoo", andPath: "symlinks/Foo"), "Symbolic link should not be equal to its destination")
+            XCTAssertFalse($0.contentsEqual(atPath: "symlinks/LinkToFoo", andPath: "EmptyFile"), "Symbolic link should not be equal to an empty file")
+            XCTAssertFalse($0.contentsEqual(atPath: "symlinks/LinkToFoo", andPath: "EmptyDirectory"), "Symbolic link should not be equal to an empty directory")
+            XCTAssertFalse($0.contentsEqual(atPath: "symlinks/EmptyDirectory", andPath: "EmptyFile"), "Empty directory should not be equal to empty file")
         }
     }
 
@@ -253,21 +263,30 @@ final class FileManagerTests : XCTestCase {
                     "Baz"
                 }
             }
+            Directory("symlinks") {
+                "Foo"
+                SymbolicLink("Bar", destination: "Foo")
+                SymbolicLink("Parent", destination: "..")
+            }
         }.test {
             XCTAssertEqual(try $0.subpathsOfDirectory(atPath: "dir1").sorted(), ["dir2", "dir2/Bar", "dir2/Foo", "dir3", "dir3/Baz"])
             XCTAssertEqual(try $0.subpathsOfDirectory(atPath: "dir1/dir2").sorted(), ["Bar", "Foo"])
             XCTAssertEqual(try $0.subpathsOfDirectory(atPath: "dir1/dir3").sorted(), ["Baz"])
+            
+            XCTAssertEqual(try $0.subpathsOfDirectory(atPath: "symlinks").sorted(), ["Bar", "Foo", "Parent"])
+            
             XCTAssertThrowsError(try $0.subpathsOfDirectory(atPath: "does_not_exist")) {
                 XCTAssertEqual(($0 as? CocoaError)?.code, .fileReadNoSuchFile)
             }
 
-            let fullContents = ["dir1", "dir1/dir2", "dir1/dir2/Bar", "dir1/dir2/Foo", "dir1/dir3", "dir1/dir3/Baz"]
+            let fullContents = ["dir1", "dir1/dir2", "dir1/dir2/Bar", "dir1/dir2/Foo", "dir1/dir3", "dir1/dir3/Baz", "symlinks", "symlinks/Bar", "symlinks/Foo", "symlinks/Parent"]
             let cwd = $0.currentDirectoryPath
             XCTAssertNotEqual(cwd.last, "/")
             let paths = [cwd, "\(cwd)/", "\(cwd)//", ".", "./", ".//"]
             for path in paths {
                 XCTAssertEqual(try $0.subpathsOfDirectory(atPath: path).sorted(), fullContents)
             }
+            
         }
     }
 
@@ -345,6 +364,32 @@ final class FileManagerTests : XCTestCase {
             XCTAssertEqual($0.delegateCaptures.shouldCopy, [.init("foo", "bar")])
             XCTAssertEqual($0.delegateCaptures.shouldProceedAfterCopyError, [.init("foo", "bar", code: .fileWriteFileExists)])
         }
+        
+        try FileManagerPlayground {
+            "foo"
+            SymbolicLink("bar", destination: "foo")
+        }.test(captureDelegateCalls: true) {
+            XCTAssertTrue($0.delegateCaptures.isEmpty)
+            try $0.copyItem(atPath: "bar", toPath: "copy")
+            XCTAssertEqual($0.delegateCaptures.shouldCopy, [.init("bar", "copy")])
+            XCTAssertEqual($0.delegateCaptures.shouldProceedAfterCopyError, [])
+            let copyDestination = try $0.destinationOfSymbolicLink(atPath: "copy")
+            XCTAssertEqual(copyDestination.lastPathComponent, "foo", "Copied symbolic link points at \(copyDestination) instead of foo")
+        }
+
+        try FileManagerPlayground {
+            Directory("dir") {
+                "foo"
+            }
+            SymbolicLink("link", destination: "dir")
+        }.test(captureDelegateCalls: true) {
+            XCTAssertTrue($0.delegateCaptures.isEmpty)
+            try $0.copyItem(atPath: "link", toPath: "copy")
+            XCTAssertEqual($0.delegateCaptures.shouldCopy, [.init("link", "copy")])
+            XCTAssertEqual($0.delegateCaptures.shouldProceedAfterCopyError, [])
+            let copyDestination = try $0.destinationOfSymbolicLink(atPath: "copy")
+            XCTAssertEqual(copyDestination.lastPathComponent, "dir", "Copied symbolic link points at \(copyDestination) instead of foo")
+        }
     }
 
     func testCreateSymbolicLinkAtPath() throws {