On Tue, 24 Sep 2024 19:16:17 GMT, Lance Andersen <lan...@openjdk.org> wrote:
>> Henry Jen has updated the pull request with a new target base due to a merge
>> or a rebase. The pull request now contains one commit:
>>
>> 8335912: Add an operation mode to the jar command when extracting to not
>> overwriting existing files
>
> src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 169:
>
>> 167: extracted: {0}
>> 168: out.kept=\
>> 169: \ \ skipped: {0}
>
> We might want to add a bit more wording to indicate it is being skipped due
> to the file already existing on disk
I follow existing pattern with short status update. Open to suggestions.
> src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 316:
>
>> 314: main.help.opt.extract.keep-old-files=\
>> 315: \ -k, --keep-old-files Do not overwrite existing files.\n\
>> 316: \ In particular, if a file appears more
>> than once in an\n\
>
> In addition, we need to update the help to clarify -x. --extract behavior
> that it will overwrite by default.
>
> Here is the verbiage from tar to give you a start
>
> ` -x Extract to disk from the archive. If a file with the same name
> appears more than
> once in the archive, each copy will be extracted, with later
> copies overwriting
> (replacing) earlier copies. The long option form is --extract.`
Updated.
> test/jdk/tools/jar/ExtractFilesTest.java line 32:
>
>> 30: * @build jdk.test.lib.Platform
>> 31: * jdk.test.lib.util.FileUtils
>> 32: * @run testng ExtractFilesTest
>
> Please convert the test to use junit as we are moving away from testng
Done. Also add a test case for multiple manifest files in the same archive as
we mentioned such use case in the help text.
> test/jdk/tools/jar/ExtractFilesTest.java line 94:
>
>> 92:
>> 93: @Test
>> 94: public void testExtract() throws IOException {
>
> Please consider adding comments introducing the methods used by the test
Updated.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21141#discussion_r1777910329
PR Review Comment: https://git.openjdk.org/jdk/pull/21141#discussion_r1777909703
PR Review Comment: https://git.openjdk.org/jdk/pull/21141#discussion_r1777911075
PR Review Comment: https://git.openjdk.org/jdk/pull/21141#discussion_r1777910602
