On Mon, 12 May 2025 10:16:33 GMT, David Beaumont <d...@openjdk.org> wrote:
>> Adding read-only support to ZipFileSystem.
>>
>> The new `accessMode` environment property allows for readOnly and readWrite
>> values, and ensures that the requested mode is consistent with what's
>> returned.
>>
>> This involved a little refactoring to ensure that "read only" state was set
>> initially and only unset at the end of initialization if appropriate.
>>
>> By making 2 methods return values (rather than silently set non-final fields
>> as a side effect) it's now clear in what order fields are initialized and
>> which are final (sadly there are still non-final fields, but only a split of
>> this class into two types can fix that, since determining multi-jar support
>> requires reading the file system).
>
> David Beaumont has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix comment based on current behaviour.
Thank. you David for your work here
A few comments on a quick pass in addition to those from Alan & Jai
The copyright will also need to be updated
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 95:
> 93:
> 94: private static final Set<PosixFilePermission> DEFAULT_PERMISSIONS =
> 95: PosixFilePermissions.fromString("rwxrwxrwx");
I am not convinced this needs to be changed as it becomes a personal style
preference
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 113:
> 111: final ZipCoder zc;
> 112: private final ZipPath rootdir;
> 113: // Start readOnly (safe mode) and maybe reset at end of
> initialization.
maybe -> may be
test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 219:
> 217: assertFalse(fs.isReadOnly());
> 218: if (!"Default
> version".equals(Files.readString(fs.getPath("file.txt"), UTF_8))) {
> 219: throw new RuntimeException("unexpected file content");
could also consider
fail("unexpected file content");
test/jdk/jdk/nio/zipfs/TestPosix.java line 434:
> 432: createTestZipFile(ZIP_FILE, ENV_DEFAULT).close();
> 433: // check entries on zipfs with default options
> 434: try (FileSystem zip = FileSystems.newFileSystem(ZIP_FILE,
> ENV_DEFAULT)) {
This tests an empty Map and should still be a test as it is different from
ENV_READ_ONLY
-------------
PR Review: https://git.openjdk.org/jdk/pull/25178#pullrequestreview-2840686940
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2089239326
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2089240538
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2089339910
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2089344316
