RFR: 8350880: (zipfs) Add support for read-only zip file systems [v3]

David Beaumont duke at openjdk.org
Thu May 15 17:18:56 UTC 2025


On Tue, 13 May 2025 12:32:56 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix comment based on current behaviour.
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 147:
> 
>> 145:     // this could exist somewhere common, but if it's definitely never going to
>> 146:     // be shared, it could be made public here.
>> 147:     private enum AccessMode {
> 
> Hello David, I think having it `private` is fine and in fact more appropriate. I would suggest removing the comment though.

Done. And if it's not getting made visible (i.e. values are always strings) I can simplify things too.

> src/jdk.zipfs/share/classes/module-info.java line 277:
> 
>> 275:  *   <td>
>> 276:  *       A value defining the desired read/write access mode of the file system
>> 277:  *       (either <em>read-write</em> or <em>read-only</em>).
> 
> This line is slightly confusing. Initially I thought `read-write` and `read-only` are the actual values that this environment property will accept. But further review suggests that the actual literals supported are `readOnly` and `readWrite` and this line here I think is trying to explain the supported semantics of the file system.
> 
> Perhaps we could reword this to something like:
> 
>>
>> A value defining the desired access mode of the file system. A ZIP file system can be created to allow for read-write access or read-only access.

Yeah, I tried to distinguish the accessMode flags (for which there are 3 states, ro, rw, n/a) and the final state of the file system from fs.isReadOnly(). Hence using `<em>...</em>` for whenever the actually resulting state is mentioned. I guess it wasn't clear enough.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2091645807
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2091649006


More information about the core-libs-dev mailing list