-
Notifications
You must be signed in to change notification settings - Fork 713
IO-845 PathUtils.copyDirectory copies or follows symbolic links #827
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
IO-845 PathUtils.copyDirectory copies or follows symbolic links #827
Conversation
…ding on the NOFOLLOW_LINKS option
garydgregory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @peterdemaeyer !
I have a comment in the copy visitor, and a small implementation issue about comparing enums.
| Set<FileVisitOption> fileVisitOptions = FOLLOW_LINKS_FILE_VISIT_OPTIONS; | ||
| if (copyOptions != null) { | ||
| for (CopyOption copyOption : copyOptions) { | ||
| if (LinkOption.NOFOLLOW_LINKS.equals(copyOption)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to use == when comparing enum values.
| */ | ||
| public CopyDirectoryVisitor(final PathCounters pathCounter, final Path sourceDirectory, final Path targetDirectory, final CopyOption... copyOptions) { | ||
| super(pathCounter); | ||
| super(pathCounter, TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have interactions to define for users:
- before the PR, we were calling
super(pathCounter), which means the default file filer isnew SymbolicLinkFileFilter(FileVisitResult.TERMINATE, FileVisitResult.CONTINUE)and the default dir filter isTrueFileFilter.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(whereLinkOption 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.
I should add that if coming up with a good solution means adding APIs and/or deprecating existing ones, I'm OK with that.
I also would like to create a release candidate to pick up existing fixes (for other Commons components and projects) but I don't want to rush this issue. If you see a release candidate tag or emails on the dev mailing list, it's for that reason, not because I don't think this is important :)
| */ | ||
| protected void updateFileCounters(final Path file, final BasicFileAttributes attributes) { | ||
| pathCounters.getFileCounter().increment(); | ||
| pathCounters.getByteCounter().add(attributes.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great documentation. This should be in the Javadoc of this method IMO.
Defined the behavior of
PathUtils.copyDirectoryfor symbolic links. It depends on the optionLinkOption.NOFOLLOW_LINKS. Non-symbolic (hard) links are out of scope, given that the copy behavior for them was already well-defined: they're treated as regular files.For the sake of conciseness, the following paragraphs use the term "link" for "symbolic link".
Without
NOFOLLOW_LINKSoption:FileSystemLoopException.With
NOFOLLOW_LINKSoption:All of this is also explained in the updated JavaDoc.
Thanks for your contribution to Apache Commons! Your help is appreciated!
Before you push a pull request, review this list:
mvn; that'smvnon the command line by itself.