Skip to content

Conversation

@peterdemaeyer
Copy link

Defined the behavior of PathUtils.copyDirectory for symbolic links. It depends on the option LinkOption.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_LINKS option:

  • Links are followed, so that there are no links in the target directory.
  • Cyclic links result in FileSystemLoopException.
  • Broken links are ignored, they are not copied at all.

With NOFOLLOW_LINKS option:

  • Links are copied as links, so that there are links in the target directory for every link in the source directory.
  • Links that link to a file or directory inside the source directory are copied to relative links inside the target directory, linking to the copied file or directory.
  • Links that link to a file or directory outside the source directory are copied to links inside the target directory, linking to the same file or directory as the link in the source directory.
  • Cyclic links are preserved, mirrored to cyclic links in the target directory.
  • Broken links are ignored, they are not copied at all.

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:

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

Copy link
Member

@garydgregory garydgregory left a 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)) {
Copy link
Member

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);
Copy link
Member

@garydgregory garydgregory Feb 8, 2026

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 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.

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());
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants