On Fri, 9 Feb 2024 10:33:30 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Please review this PR which suggests we speed up the `Zip64SizeTest` using a >> small-sized ZIP64 ZIP file specifically created to reproduce the issue being >> tested. >> >> The disk space requirement of this test is known to cause problems in some >> builds, see [JDK-8259866](https://bugs.openjdk.org/browse/JDK-8259866) >> >> By using a sparse file, we reduce consumed disk space from 5GB to 266 bytes >> and also reduce the runtime from ~35 seconds to ~1 seconds on my Macbook Pro. >> >> The PR also fixes the `@summary` tag, which seems to have been copied from >> an unrelated test. > > Eirik Bjørsnøs has updated the pull request incrementally with two additional > commits since the last revision: > > - Spell fix for "Contents" > - Add comments to explain how we temporarily use the 'unknown' tag to > prevent ZipEntry.setExtra0 from processing the extra field. thank you for the latest updates Eirik. Overall it is good a few more comment suggestions for extra clarity. test/jdk/java/util/zip/ZipFile/Zip64SizeTest.java line 114: > 112: > 113: // Make an extra field with the correct size for an 8-byte > 'uncompressed size' > 114: // Zip64 field. Temporarily use the 'unknown' tag 0x9902 to > make I would suggest adding a reference to the following in the APPNOTE.TXT to make clearer where that value came from > 4.6.1 Third party mappings commonly used are: another suggestion would be to show the CEN here with this change I think would make it easier for someone who is not as familiar with APPNOTE.TXT test/jdk/java/util/zip/ZipFile/Zip64SizeTest.java line 140: > 138: * @param zip the ZIP file to update to ZIP64 > 139: */ > 140: private static void updateCENHeaderToZip64(byte[] zip) { Again minor but perhaps articulate that after this call, the extra data will be(obvious to you and I but probably not to others...) > * 00B4 Extra ID #0001 0001 'ZIP64' > * 00B6 Length 0008 > * 00B8 Uncompressed Size 0000000000000005 ------------- Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/12948#pullrequestreview-1872219560 PR Review Comment: https://git.openjdk.org/jdk/pull/12948#discussion_r1484225117 PR Review Comment: https://git.openjdk.org/jdk/pull/12948#discussion_r1484229268
