Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes [v2]
On Fri, 2 Feb 2024 20:43:54 GMT, Lance Andersen wrote: > I think the proposed change above makes things clearer. Perhaps also make the > same change in zipfs as well while you are at it? I have pushed the rename to "ZipEntry.externalFileAttributes". Also renamed `ZipFileSystem.Entry.posixPerms` to `ZipFileSystem.Entry.externalFileAttributes`. Hopefully this makes things clearer. A side note: The Posix support in `ZipFileSystem` is somewhat odd in that it supports the notion of a `null` permission set. So setting the permissions attribute to `null` clears all the attributes, not just the permisson ones. But even so, I think using the full name here is also an improvement, since it signals that the field may also carry preexisting file type, setuid, setgid, sticky bits. - PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-1925403883
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes [v2]
On Fri, 2 Feb 2024 20:19:51 GMT, Eirik Bjørsnøs wrote: > > I think the change is OK, any reason to not make it > > `externalFileAttributes`? > > None other than length / verbosity. But since this refers to the _attributes > of the external file_, dropping any name element also loses some semantics, > introducing a potential for confusion. Perhaps verbosity is the sensible > choice here. > > If you agree to the above, I can update the PR to rename to the following > names instead: > > * `ZipFile.externalFileAttributes` > * `JavaUtilZipFileAccess.java.[set|get]ExternalFileAttributes` > * `JarSigner.externalFileAttributesDetected` > * `jarsigner/Main.externalFileAttributesDetected` > * `external.file.attributes.detected` in `Resources.java` > > WDYT? I think the proposed change above makes things clearer. Perhaps also make the same change in zipfs as well while you are at it? - PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-1924670785
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes [v2]
On Fri, 2 Feb 2024 19:55:51 GMT, Lance Andersen wrote: > I think the change is OK, any reason to not make it `externalFileAttributes`? None other than length / verbosity. But since this refers to the _attributes of the external file_, dropping any name element also loses some semantics, introducing a potential for confusion. Perhaps verbosity is the sensible choice here. If you agree to the above, I can update the PR to rename to the following names instead: - `ZipFile.externalFileAttributes` - `JavaUtilZipFileAccess.java.[set|get]ExternalFileAttributes` - `JarSigner.externalFileAttributesDetected` - `jarsigner/Main.externalFileAttributesDetected` - `external.file.attributes.detected` in `Resources.java` WDYT? - PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-1924638322
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes [v2]
On Tue, 30 Jan 2024 16:17:13 GMT, Eirik Bjørsnøs wrote: >> Please consider this PR which suggests we rename `ZipEntry.extraAttributes` >> to `ZipEntry.externalAttributes`. >> >> 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.externalAttributes` 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.externalAttributes` >> - Update `JavaUtilZipFileAccess` to similarly rename methods to >> `setExternalAttributes` and `getExternalAttributes` >> - Rename the field `JarSigner.extraAttrsDetected` to >> `JarSigner.externalAttrsDetected` >> - Rename a local variable in `JarSigner.writeEntry` to `externalAttrs` >> - Rename `s.s.t.jarsigner.Main.extraAttrsDetected` to `externalAttrsDetected` >> - Rename resource string key names in `s.s.t.jarsigner.Resources` from >> `extra.attributes.detected` to `external.attributes.detected` >> - Rename method `SymlinkTest.verifyExtraAttrs` to `verifyExternalAttrs`, >> 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 incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Update copyright years for 2024 > - Merge branch 'master' into zipentry-external-attributes > - Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes I think the change is OK, any reason to not make it `externalFileAttributes`? - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16952#pullrequestreview-1860228844
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes [v2]
> Please consider this PR which suggests we rename `ZipEntry.extraAttributes` > to `ZipEntry.externalAttributes`. > > 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.externalAttributes` 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.externalAttributes` > - Update `JavaUtilZipFileAccess` to similarly rename methods to > `setExternalAttributes` and `getExternalAttributes` > - Rename the field `JarSigner.extraAttrsDetected` to > `JarSigner.externalAttrsDetected` > - Rename a local variable in `JarSigner.writeEntry` to `externalAttrs` > - Rename `s.s.t.jarsigner.Main.extraAttrsDetected` to `externalAttrsDetected` > - Rename resource string key names in `s.s.t.jarsigner.Resources` from > `extra.attributes.detected` to `external.attributes.detected` > - Rename method `SymlinkTest.verifyExtraAttrs` to `verifyExternalAttrs`, 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 incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Update copyright years for 2024 - Merge branch 'master' into zipentry-external-attributes - Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes - Changes: - all: https://git.openjdk.org/jdk/pull/16952/files - new: https://git.openjdk.org/jdk/pull/16952/files/87db8646..481ae754 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16952&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16952&range=00-01 Stats: 155690 lines in 2884 files changed: 86094 ins; 58372 del; 11224 mod Patch: https://git.openjdk.org/jdk/pull/16952.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16952/head:pull/16952 PR: https://git.openjdk.org/jdk/pull/16952
