8371689: (fs) CopyMoveHelper.copyToForeignTarget use of sourcePosixView is confusing #28304

bplb · 2025-11-13T18:57:26Z

In CopyMoveHelper, replace PosixFileAttributeView with a boolean.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8371689: (fs) CopyMoveHelper.copyToForeignTarget use of sourcePosixView is confusing (Bug - P4)

Reviewers

Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28304/head:pull/28304
$ git checkout pull/28304

Update a local copy of the PR:
$ git checkout pull/28304
$ git pull https://git.openjdk.org/jdk.git pull/28304/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28304

View PR using the GUI difftool:
$ git pr show -t 28304

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28304.diff

Using Webrev

Link to Webrev Comment

…ew is confusing

bridgekeeper · 2025-11-13T18:58:34Z

👋 Welcome back bpb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-11-13T18:59:10Z

@bplb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8371689: (fs) CopyMoveHelper.copyToForeignTarget use of sourcePosixView is confusing

Reviewed-by: alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 61 new commits pushed to the master branch:

6e7eaf4: 8371591: VerifyJimage test incorrectly skips all tests when comparing directory structure
10f262a: 8371804: C2: Tighten up LoadNode::Value comments after JDK-8346184
466cb38: 8371885: Mark UseCompressedClassPointers as obsolete for JDK 27
... and 58 more: https://git.openjdk.org/jdk/compare/43afce54a7ecbd124f68f1f32d718f08b24ca61a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-11-13T19:00:08Z

@bplb The following label will be automatically applied to this pull request:

nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-11-13T19:04:39Z

Webrevs

AlanBateman · 2025-11-13T19:32:50Z

src/java.base/share/classes/java/nio/file/CopyMoveHelper.java

+        FileSystemProvider provider = source.getFileSystem().provider();
+
+        // retrieve whether source posix view is supported
+        FileStore fileStore = provider.getFileStore(source);


I'm concerned this will add overhead to each copy operation. There shouldn't be any need to use FileStore in this method. If the provider supports the the POSIX view then this method can use it.

So I think the summary is the JBS issue isn't really a bug, it's more than using sourcePosixView instead of boolean is confusing when looking at this code.

bplb · 2025-11-14T00:16:13Z

There shouldn't be any need to use FileStore in this method.

Removed in commit bebf4a5.

AlanBateman · 2025-11-14T12:41:27Z

src/java.base/share/classes/java/nio/file/CopyMoveHelper.java


-        // retrieve source posix view, null if unsupported
-        final PosixFileAttributeView sourcePosixView =
-            Files.getFileAttributeView(source, PosixFileAttributeView.class);


I don't think we should delete this. Instead, I think we can initialize sourceSupportsPosixAttributes with Files.getFileAttributeView(source, PosixFileAttributeView.class) != null.

I don't think we should delete this

So changed in ad29084.

…etrievable

AlanBateman · 2025-11-14T18:08:10Z

src/java.base/share/classes/java/nio/file/CopyMoveHelper.java

                                               PosixFileAttributes.class,
                                               linkOptions);
-        }
        if (sourceAttrs == null)


This could if-then-else too.

This could if-then-else too.

Since if the posix view is supported, then sourceAttrs could not be null here, correct?

Right, but it can be simplified to if (sourceSupportsPosixAttributes) { .. } else { ..}. What you have is okay too.

it can be simplified

So changed in e76d298.

AlanBateman

This update looks okay.

bplb · 2025-11-17T16:47:58Z

/integrate

openjdk · 2025-11-17T16:48:45Z

Going to push as commit 9ec773a.
Since your change was applied there have been 90 commits pushed to the master branch:

52ffe8a: 8371156: PBKDF2 default values should not be DER encoded
6385c66: 8371607: Remove GCSharedStringsDuringDumpWb.java after JDK-8362561
44087ea: 8371682: Suppress javac warning from ThreadPoolExecutorSubclassTest.java
... and 87 more: https://git.openjdk.org/jdk/compare/43afce54a7ecbd124f68f1f32d718f08b24ca61a...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-11-17T16:48:55Z

@bplb Pushed as commit 9ec773a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8371689: (fs) CopyMoveHelper.copyToForeignTarget use of sourcePosixVi…

fe0eeda

…ew is confusing

openjdk bot added the nio nio-dev@openjdk.org label Nov 13, 2025

openjdk bot added the rfr Pull request is ready for review label Nov 13, 2025

AlanBateman reviewed Nov 13, 2025

View reviewed changes

8371689: Remove use of FileStore

bebf4a5

AlanBateman reviewed Nov 14, 2025

View reviewed changes

8371689: Determine posix attributes support based on the view being r…

ad29084

…etrievable

AlanBateman reviewed Nov 14, 2025

View reviewed changes

AlanBateman approved these changes Nov 14, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 14, 2025

8371689: Simplify conditional

e76d298

openjdk bot removed the ready Pull request is ready to be integrated label Nov 14, 2025

AlanBateman approved these changes Nov 14, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 14, 2025

openjdk bot added the integrated Pull request has been integrated label Nov 17, 2025

openjdk bot closed this Nov 17, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 17, 2025

bplb deleted the CopyMoveHelper-8371689 branch November 17, 2025 17:29

8371689: (fs) CopyMoveHelper.copyToForeignTarget use of sourcePosixView is confusing #28304

8371689: (fs) CopyMoveHelper.copyToForeignTarget use of sourcePosixView is confusing #28304

Uh oh!

Conversation

bplb commented Nov 13, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 13, 2025

Uh oh!

openjdk bot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bplb commented Nov 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

bplb commented Nov 17, 2025

Uh oh!

openjdk bot commented Nov 17, 2025

Uh oh!

openjdk bot commented Nov 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

bplb commented Nov 13, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Nov 13, 2025 •

edited

Loading

openjdk bot commented Nov 13, 2025 •

edited

Loading

mlbridge bot commented Nov 13, 2025 •

edited

Loading