On Fri, 27 Sep 2024 01:41:33 GMT, Henry Jen <henry...@openjdk.org> wrote:
>> This PR support a -k, --keep options like tar that allows jar to avoid
>> override existing files.
>
> Henry Jen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review feedbacks
test/jdk/tools/jar/ExtractFilesTest.java line 236:
> 234: PrintStream err = new PrintStream(baes);
> 235: PrintStream saveErr = System.err;
> 236: System.setErr(err);
Given that we are updating the runtime level state, I think we should use
`/othervm` in this test's definition to avoid any interference with other parts
of the runtime.
test/jdk/tools/jar/ExtractFilesTest.java line 238:
> 236: System.setErr(err);
> 237: int rc = JAR_TOOL.run(out, err, cmdline.split(" +"));
> 238: System.setErr(saveErr);
Doing a
try {
int rc = JAR_TOOL.run(out, err, cmdline.split(" +"));
}finally {
System.setErr(saveErr);
}
would be safer.
test/jdk/tools/jar/ExtractFilesTest.java line 241:
> 239: if (rc != 0) {
> 240: String s = baes.toString();
> 241: if (s.startsWith("java.util.zip.ZipException: duplicate
> entry: ")) {
This method runs any arbitrary `jar` "command". Is there any significance why
we are checking for a "duplicate entry" in the exception message? Maybe we
could just remove this entire if block and just throw a `new IOException(s)`?
As far as I can see in this test, we don't do any exception type checks on the
thrown exception from this method.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21141#discussion_r1779499876
PR Review Comment: https://git.openjdk.org/jdk/pull/21141#discussion_r1779500135
PR Review Comment: https://git.openjdk.org/jdk/pull/21141#discussion_r1779501074
