On Mon, 29 Mar 2021 14:04:10 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review for this patch which proposes to implement the >> enhancement request noted in >> https://bugs.openjdk.java.net/browse/JDK-8173970? >> >> The commit in this PR introduces the `-o` and `--output-dir` option to the >> `jar` command. The option takes a path to a destination directory as a value >> and extracts the contents of the jar into that directory. This is an >> optional option and the changes in the commit continue to maintain backward >> compatibility where the jar is extracted into current directory, if no `-o` >> or `--output-dir` option has been specified. >> >> As far as I know, there hasn't been any discussion on what the name of this >> new option should be. I was initially thinking of using `-d` but that is >> currently used by the `jar` command for a different purpose. So I decided to >> use `-o` and `--output-dir`. This is of course open for change depending on >> any suggestions in this PR. >> >> The commit in this PR also updates the `jar.properties` file which contains >> the English version of the jar command's `--help` output. However, no >> changes have been done to the internationalization files corresponding to >> this one (for example: `jar_de.properties`), because I don't know what >> process needs to be followed to have those files updated (if at all they >> need to be updated). >> >> The commit also includes a jtreg testcase which verifies the usage of this >> new option. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Alan's review feedback for -C help text > - Keep -xfP backward compatible but don't allow -C/--dir with -xfP Some additional comments basically suggesting that we test --extract in addition to -x test/jdk/tools/jar/JarExtractTest.java line 175: > 173: final String dest = "foo-bar"; > 174: System.out.println("Extracting " + testJarPath + " to " + dest); > 175: final int exitCode = JAR_TOOL.run(System.out, System.err, "-x", > "-f", testJarPath.toString(), Perhaps add a DataProvider so you can test --extract as well? test/jdk/tools/jar/JarExtractTest.java line 216: > 214: final Path jarPath = createJarWithPFlagSemantics(); > 215: // extract with -P flag without any explicit destination > directory (expect the extraction to work fine) > 216: final String[] args = new String[]{"-xvfP", jarPath.toString()}; Perhaps add a DataProvider so you can test --extract as well? test/jdk/tools/jar/JarExtractTest.java line 239: > 237: */ > 238: @Test > 239: public void testExtractWithDirPFlagNotAllowed() throws Exception { I would include --extract in your testing options test/jdk/tools/jar/JarExtractTest.java line 240: > 238: @Test > 239: public void testExtractWithDirPFlagNotAllowed() throws Exception { > 240: final String expectedErrMsg = "-P option cannot be used when > extracting a jar to a specific location"; Probably need to add a comment that this must match the entry in jar.properties test/jdk/tools/jar/JarExtractTest.java line 247: > 245: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), > "-P", "-C", "."}); > 246: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), > "-P", "--dir", "."}); > 247: cmdArgs.add(new String[]{"-xvfP", testJarPath.toString(), "-C", > tmpDir}); Perhaps add a DataProvider so you can test --extract as well? test/jdk/tools/jar/JarExtractTest.java line 262: > 260: */ > 261: @Test > 262: public void testLegacyCompatibilityMode() throws Exception { Perhaps add a DataProvider so you can test --extract as well? test/jdk/tools/jar/JarExtractTest.java line 282: > 280: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), > "-C", tmpDir, "-C", tmpDir}); > 281: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), > "--dir", tmpDir, "--dir", tmpDir}); > 282: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), > "--dir", tmpDir, "-C", tmpDir}); Perhaps use a DataProvider so you can test --extract as well? test/jdk/tools/jar/JarExtractTest.java line 300: > 298: public void testExtractPartialContent() throws Exception { > 299: final String tmpDir = Files.createTempDirectory(Path.of("."), > "8173970-").toString(); > 300: final String[] cmdArgs = new String[]{"-x", "-f", > testJarPath.toString(), "--dir", tmpDir, Perhaps add a DataProvider so you can test --extract as well? test/jdk/tools/jar/JarExtractTest.java line 332: > 330: */ > 331: private void testExtract(final String dest) throws Exception { > 332: final String[] args = new String[]{"-x", "-f", > testJarPath.toString(), "-C", dest}; Perhaps add a DataProvider so you can test --extract as well? ------------- PR: https://git.openjdk.java.net/jdk/pull/2752
