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 Hi Jaikiran Overall this looks good. I have a few comments below and will look at the CSR shortly. src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1427: > 1425: return rc; // leading '/' or 'dot-dot' only path > 1426: } > 1427: File f = new File(xdestDir, name.replace('/', > File.separatorChar)); Could you please add a comment regarding xdestDir and also correct the typo on line 1418 'requres' -> 'requires' src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 62: > 60: Could not create a temporary file > 61: error.extract.multiple.dest.dir=\ > 62: You may not specify more than one directory for extracting the jar Perhaps something similar to: You may not specify the '-C' or '--dir' option more than once with the '-x' option src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 64: > 62: You may not specify more than one directory for extracting the jar > 63: error.extract.pflag.not.allowed=\ > 64: -P option cannot be used when extracting a jar to a specific > location Perhaps something similar to You may not specify '-Px' with the '-C' or '--dir' options src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 167: > 165: (in = {0}) (out= {1}) > 166: out.extract.dir=\ > 167: extracting to {0} Perhaps 'Extracting to directory: {0}' src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 249: > 247: \ -C DIR Change to the specified directory and > include the\n\ > 248: \ following file. When used in extract mode, > extracts\n\ > 249: \ the jar to the specified directory Should this be updated on line 187 as well in the compatibility mode section? test/jdk/tools/jar/JarExtractTest.java line 152: > 150: return abs; > 151: } > 152: Please add a comment to each test giving a high level overview of what it does as it will help future maintainers test/jdk/tools/jar/JarExtractTest.java line 307: > 305: // make sure only the specific files were extracted > 306: final Stream<Path> paths = Files.walk(Path.of(tmpDir)); > 307: Assert.assertEquals(paths.count(), 6, "Unexpected number of > files/dirs in " + tmpDir); Should '6' be in a local variable to make it clearer or at a minimum a comment test/jdk/tools/jar/JarExtractTest.java line 367: > 365: } > 366: > 367: private static Path createJarWithPFlagSemantics() throws IOException > { Perhaps add a comment as to what the method does ------------- PR: https://git.openjdk.java.net/jdk/pull/2752
