On Fri, 18 Oct 2024 18:40:13 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address PR feedback
>
> src/java.base/share/classes/java/util/zip/ZipEntry.java line 528:
>
>> 526: * extra field data, the {@linkplain #getName() entry name},
>> 527: * the {@linkplain #getComment() entry comment}, and the
>> 528: * {@linkplain #CENHDR CEN Header size}, exceeds 65,535 bytes.
>
> Is the comma before "exceeds" intentional? Question also applies to
> `setComment`. The constructor does not have a comma at this position.
>
> Suggestion:
>
> * {@linkplain #CENHDR CEN Header size} exceeds 65,535 bytes.
removed
> test/jdk/java/util/zip/ZipEntry/MaxZipEntryFieldSizeTest.java line 28:
>
>> 26: * @summary Verify that ZipEntry(String), ZipEntry::setComment, and
>> 27: * ZipEntry::setExtra throws a IllegalArgumentException when the
>> 28: * length of the field exceeds 65,489 bytes
>
> The summary seems slightly wrong now:
>
> Suggestion:
>
> * combined length of the CEN header fields exceeds 65,489 bytes
updated
> test/jdk/java/util/zip/ZipEntry/MaxZipEntryFieldSizeTest.java line 93:
>
>> 91: boolean expectException = length > MAX_NAME_COMMENT_EXTRA_SIZE;
>> 92: ZipEntry zipEntry = new ZipEntry(ENTRY_NAME);
>> 93: String comment = new String(bytes, StandardCharsets.UTF_8);
>
> Could this be just?
>
>
> String comment = "a".repeat(length);
>
>
> Applies to the name test method as well.
sure done
> test/jdk/java/util/zip/ZipEntry/MaxZipEntryFieldSizeTest.java line 96:
>
>> 94: System.out.printf("Comment Len= %s, exception: %s%n",
>> comment.length(), expectException);
>> 95: // The comment length will trigger the IllegalArgumentException
>> 96: if (expectException) {
>
> Should there be an `else` here which sets the comment successfully?
>
> This applies to all three tests methods.
done
> test/jdk/java/util/zip/ZipEntry/MaxZipEntryFieldSizeTest.java line 112:
>
>> 110: MAX_FIELD_LEN_MINUS_ENTRY_NAME,
>> 111: MAX_FIELD_LEN_MINUS_ENTRY_NAME - 1})
>> 112: void setNameLengthTest(int length) {
>
> Perhaps just 'nameLengthTest', as this does not test a setter.
>
> Suggestion:
>
> void nameLengthTest(int length) {
sure done
> test/jdk/java/util/zip/ZipEntry/MaxZipEntryFieldSizeTest.java line 135:
>
>> 133: MAX_FIELD_LEN_MINUS_ENTRY_NAME - 1})
>> 134: void setExtraLengthTest(int length) {
>> 135: final byte[] bytes = new byte[length];
>
> I think the purpose of this test may be clearer if the construction of the
> extra byte array was extracted as a separate method. (The exact contents
> seems irrelevant as longs as its well-formed)
Sure, done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806961065
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806959754
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806960501
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806960667
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806960010
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806960336
