FileManager: Fix copyItem(atPath:toPath:) for directories and symlinks. #1515
Conversation
- When copying directories dont recurse into copyItems() just iterate through the results of subpathsOfDirectory(). - Copy the contents of the symlink not the file that it points to, creating a new symlink.
|
@swift-ci please test |
|
@swift-ci please test and merge |
1 similar comment
|
@swift-ci please test and merge |
| let destination = try destinationOfSymbolicLink(atPath: srcPath) | ||
| try createSymbolicLink(atPath: dstPath, withDestinationPath: destination) | ||
| } else if fileType == .typeRegular { | ||
| if createFile(atPath: dstPath, contents: contents(atPath: srcPath), attributes: nil) == false { |
There was a problem hiding this comment.
This works, but a proper copy engine will read then write a limited portion of the source file at a time. For large files, this will result in an extreme high-water mark for memory usage.
There was a problem hiding this comment.
Good point, this was how the previous version was doing it but I will create a PR to fix it.
| if fileType == .typeDirectory { | ||
| try createDirectory(atPath: dstPath, withIntermediateDirectories: false, attributes: nil) | ||
| let subpaths = try subpathsOfDirectory(atPath: srcPath) | ||
| for subpath in subpaths { | ||
| try copyItem(atPath: srcPath + "/" + subpath, toPath: dstPath + "/" + subpath) | ||
| let src = srcPath + "/" + subpath | ||
| let dst = dstPath + "/" + subpath |
There was a problem hiding this comment.
I'm sorry, I don't recall—Do we not have an equivalent of -stringByAppendingPathComponent: in SCL?
There was a problem hiding this comment.
Only on NSString it appears, maybe a helper on String could be added in NSPathUtilities.
| let dst = dstPath + "/" + subpath | ||
| if let attrs = try? attributesOfItem(atPath: src), let fileType = attrs[.type] as? FileAttributeType { | ||
| if fileType == .typeDirectory { | ||
| try createDirectory(atPath: dst, withIntermediateDirectories: false, attributes: nil) |
There was a problem hiding this comment.
This looks wrong. No recursion at all means we only go one extra level deep, no? (There should be a test for this.)
There was a problem hiding this comment.
The original implementation was wrong because it was recursing in every directory and then additional recursing in every sub directory. It now uses subpathsOfDirectory(atPath:) at the top level to get all of the subitems. There is are tests for it at https://github.com/apple/swift-corelibs-foundation/pull/1515/files/3c454b7358312d8d10dcb68a22d577e9ec816b58#diff-fffbc0319b30adac9f635327a750b82fR555
There was a problem hiding this comment.
Ah, I see. I misread this as contentsOfDirectory.
Please note that subpathsOfDirectory is very expensive, as it does all the directory enumeration at once and potentially creates a VERY large array. This is a high-water mark issue, similar to the contentsAtPath one. Using an enumerator would be much preferable here, and should be a fairly simple drop-in replacement.
When copying directories dont recurse into copyItems() just iterate
through the results of subpathsOfDirectory().
Copy the contents of the symlink not the file that it points to,
creating a new symlink.
Found during testing of #1510