garydgregory commented on code in PR #827:
URL: https://github.com/apache/commons-io/pull/827#discussion_r2779441487
##########
src/main/java/org/apache/commons/io/file/CopyDirectoryVisitor.java:
##########
@@ -53,7 +54,7 @@ private static CopyOption[] toCopyOption(final CopyOption...
copyOptions) {
* @param copyOptions Specifies how the copying should be done.
*/
public CopyDirectoryVisitor(final PathCounters pathCounter, final Path
sourceDirectory, final Path targetDirectory, final CopyOption... copyOptions) {
- super(pathCounter);
+ super(pathCounter, TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE);
Review Comment:
I think we have interactions to define for users:
- before the PR, we were calling `super(pathCounter)`, which means the
default file filer is `new SymbolicLinkFileFilter(FileVisitResult.TERMINATE,
FileVisitResult.CONTINUE)` and the default dir filter is
`TrueFileFilter.INSTANCE`
- In this PR, we set both to `TrueFileFilter.INSTANCE`, so no change for
dirs.
It seems both could be "wrong" in the sense that they compete with
`CopyOption...` which can contain `java.nio.file.LinkOption.NOFOLLOW_LINKS`.
I'm sorry this is all confusing! We have:
- `java.nio.file.LinkOption.NOFOLLOW_LINKS` (where `LinkOption implements
OpenOption, CopyOption`)
- `java.nio.file.FileVisitOption.FOLLOW_LINKS`
- An IO file filter
- An IO dir filter
I understand we have a bug, but we should take this opportunity to document
what this change means.
##########
src/main/java/org/apache/commons/io/file/PathUtils.java:
##########
@@ -377,7 +400,22 @@ public static long copy(final IOSupplier<InputStream> in,
final Path target, fin
*/
public static PathCounters copyDirectory(final Path sourceDirectory, final
Path targetDirectory, final CopyOption... copyOptions) throws IOException {
final Path absoluteSource = sourceDirectory.toAbsolutePath();
- return visitFileTree(new
CopyDirectoryVisitor(Counters.longPathCounters(), absoluteSource,
targetDirectory, copyOptions), absoluteSource)
+ // CopyOption.NOFOLLOW_LINKS is the explicit option, "follow symlinks"
the implicit default.
+ // For FileVisitOption it's the other way around:
FileVisitOption.FOLLOW_LINKS is the explicit option, and "do not follow
symlinks" is the implicit
+ // default.
+ // If they're not in sync, the behavior is inconsistent, so we have to
make sure they're in sync.
+ // CopyOption is given by the caller, FileVisitOption is under our
control here, so we sync the latter to the former.
+ Set<FileVisitOption> fileVisitOptions =
FOLLOW_LINKS_FILE_VISIT_OPTIONS;
+ if (copyOptions != null) {
+ for (CopyOption copyOption : copyOptions) {
+ if (LinkOption.NOFOLLOW_LINKS.equals(copyOption)) {
Review Comment:
I think we want to use `==` when comparing enum values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
