-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8278165: Clarify that ZipInputStream does not access the CEN fields for a ZipEntry #10102
Conversation
👋 Welcome back lancea! A progress list of the required criteria for merging this PR into |
@LanceAndersen The following label will be automatically applied to this pull request:
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. |
…to JDK-8278165
|
Webrevs
|
* | ||
* The {@link #getNextEntry()} method is used to read the next ZIP file entry | ||
* (Local file (LOC) header record in the ZIP format) and position the stream at | ||
* the entry's file data. The file data may read using one of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of -> of the?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
* <pre> | ||
* {@code | ||
* try (FileInputStream fis = new FileInputStream(jar.toFile()); | ||
* ZipInputStream zis = new ZipInputStream(fis)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation seems weird in this code sample.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reformatted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine.
@LanceAndersen 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:
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 52 new commits pushed to the
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 |
* } | ||
* } | ||
* } | ||
* </pre> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated class description looks okay. For the example then I assume you make this a code snippet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converted the example to a snippet. Thanks for the suggestion
* For example: | ||
* {@snippet : | ||
* try (FileInputStream fis = new FileInputStream(jar.toFile()); | ||
* ZipInputStream zis = new ZipInputStream(fis)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other comment on the snippet is that the type of "jar" may not be obvious to readers. I think you'll need Path jar = ... in which case changing it Files.newInputStream(jar) might be simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other comment on the snippet is that the type of "jar" may not be obvious to readers. I think you'll need Path jar = ... in which case changing it Files.newInputStream(jar) might be simpler.
Updated per your suggestion
* does not read the Central directory (CEN) header for the entry and therefore | ||
* will not have access to its metadata such as the external file attributes. | ||
* {@linkplain ZipFile} may be used when the information stored within | ||
* the CEN header is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this paragraph should be an apiNote. It's not wrong as is but it feels like a note to remind developers that this is an "input stream" for iterating through the ZIP entries. The external files attributes are in the CEN at the end of the ZIP file so this is why they aren't available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this paragraph should be an apiNote. It's not wrong as is but it feels like a note to remind developers that this is an "input stream" for iterating through the ZIP entries. The external files attributes are in the CEN at the end of the ZIP file so this is why they aren't available.
converted to an apiNote
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think version e15660e looks okay.
/integrate |
Going to push as commit a8f0f57.
Your commit was automatically rebased without conflicts. |
@LanceAndersen Pushed as commit a8f0f57. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi,
Please review this update to the ZipInputStream class description to clarify that ZipInputStream walks sequentially through each Zip Entry contained within the Zip File. As a result, the CEN header for the Zip file entries are not read resulting in ZipInputStream not having access to information that is stored within the CEN Header fields such the as the external file attributes which can be used to store optional data such as Posix Permissions.
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10102/head:pull/10102
$ git checkout pull/10102
Update a local copy of the PR:
$ git checkout pull/10102
$ git pull https://git.openjdk.org/jdk pull/10102/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10102
View PR using the GUI difftool:
$ git pr show -t 10102
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10102.diff