RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes [v5]

Jaikiran Pai jpai at openjdk.org
Tue Jul 2 12:54:25 UTC 2024


On Sat, 29 Jun 2024 18:24:46 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

>> Please consider this PR which suggests we rename `ZipEntry.extraAttributes` to `ZipEntry.externalFileAttributes`.
>> 
>> This field was introduced in [JDK-8218021](https://bugs.openjdk.org/browse/JDK-8218021), originally under the name `ZipEntry.posixPerms`. [JDK-8250968](https://bugs.openjdk.org/browse/JDK-8250968) later renamed the field to `ZipEntry.extraAttributes` and extended its semantics to hold the full two-byte value of the `external file attributes` field, as defined by `APPNOTE.TXT`
>> 
>> The name `extraAttributes` is misleading. It has nothing to do with the `extra field` (an unrelated structure defined in `APPNOTE.TXT`), although the name indicates it does.
>> 
>> To prevent confusion and make life easier for future maintainers, I suggest we rename this field to `ZipEntry.externalFileAttributes` and update related methods, parameters and comments accordingly.
>> 
>> While this change is a straightforward renaming, reviewers should consider whether it carries its weight, especially considering it might complicate future backports. 
>> 
>> As a note to reviewers, this PR includes the following intended updates:
>> 
>> - Rename `ZipEntry.extraAttributes` and any references to this field to `ZipEntry.externalFileAttributes`
>> - Update `JavaUtilZipFileAccess` to similarly rename methods to `setExternalFileAttributes` and `getExternalFileAttributes` 
>> - Rename the field `JarSigner.extraAttrsDetected` to `JarSigner.externalFileAttrsDetected`
>> - Rename a local variable in `JarSigner.writeEntry` to `externalFileAttributes`
>> - Rename `s.s.t.jarsigner.Main.extraAttrsDetected` to `externalFileAttributesDetected`
>> - Rename resource string key names in `s.s.t.jarsigner.Resources` from `extra.attributes.detected` to `external.file.attributes.detected`
>> - Rename method `SymlinkTest.verifyExtraAttrs` to `verifyExternalFileAttributes`, also updated two references to 'extra attributes' in this method
>> - Updated copyright in all affected files
>> 
>> If the resource file changes should be dropped and instead handled via `msgdop` updates, let me know and I can revert the non-default files.
>> 
>> I did a search across the code base to find 'extraAttrs', 'extra.attr' after these updates, and found nothing related to zip/jar. The `zip` and `jar` tests run clean:
>> 
>> 
>> make test TEST="test/jdk/java/util/jar"
>> make test TEST="test/jdk/java/util/zip"
>
> Eirik Bjørsnøs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
> 
>  - Merge branch 'master' into zipentry-external-attributes
>  - Rename SymlinkTest.verifyExternalAttrs to verifyExternalFileAttributes
>  - Rename ZipFileSystem.Entry.posixPerms to externalFileAttributes
>  - For clarity, use "externalFileAttributes" instead of "externalAttributes"
>  - Merge branch 'master' into zipentry-external-attributes
>  - Update copyright years for 2024
>  - Merge branch 'master' into zipentry-external-attributes
>  - Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes

Marked as reviewed by jpai (Reviewer).

Hello Eirik, the latest changes in this PR (`292d6801`) look good to me. However, these changes cause some (expected) compilation failures in some of the internal classes within some Oracle specific JDK classes. Those tests aren't accessible for external contributors. I will be updating that code to address those failures. That would mean that the integration of this PR will have to be co-ordinated. 

Can you issue a `/integrate delegate` command on this PR so that it then allows me to do the actual integration along with the Oracle side of changes?

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

PR Review: https://git.openjdk.org/jdk/pull/16952#pullrequestreview-2153714037
PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-2203081198


More information about the nio-dev mailing list