RFR: Add tool for importing webrev patches
Erik Duveblad via github.com
duke at openjdk.java.net
Thu Sep 5 04:33:30 UTC 2019
On Fri, 5 Jul 2019 10:01:57 GMT, JornVernee via github.com <duke at openjdk.java.net> wrote:
> As discussed on the mailing list: https://mail.openjdk.java.net/pipermail/skara-dev/2019-July/000176.html
>
> This PR adds a tool for importing webrev `.patch` files, either from a webrev url or local file path into a repository.
>
> I've named the tool `wimport` as short for `webrev-import` (I thought the full name sounded a bit too much like a sub-command of `webrev`). This will first checkout a new branch, then apply the patch, and then create a commit for the patch with the message `Imported patch 'NAME'`, similar to what is done by mq. Afterwards a user can just delete the new branch when they're done with e.g. reviewing the webrev. Although, there is also an option to directly apply the webrev without creating a branch or commit.
>
> Currently the following options are supported:
>
> usage: git wimport [options] <webrev url|file path>
> -n, --name NAME Use NAME as a name for the patch (default is patch file name)
> -f, --file Input is a file path
> -k, --keep Keep downloaded patch file in current directory
> -d, --direct Directly apply patch, without creating a branch or commit
>
> ----------------
>
> Commits:
> - 7b20ce75: Wimport prototype
>
> Pull request:
> http://git.openjdk.java.net/skara/pull/32
>
> Webrev:
> https://openjdk.github.io/cr/skara/32/webrev.00
>
> Patch:
> http://git.openjdk.java.net/skara/pull/32.diff
>
> Fetch command:
> git fetch https://github.com/openjdk/skara.git pull/32/head:pull/32
This PR has been reviewed by Erik Duveblad via github.com - changes are approved. Review comment:
Great work @JornVernee! Just a few tiny comments that you can fix before integrating :+1:
PR: https://git.openjdk.java.net/skara/pull/32
args/src/main/java/org/openjdk/skara/args/MultiCommandParser.java line 57:
> 56: public Executable parse(String[] args) {
> 57: if (args.length != 0) {
> 58: var p = subCommands.get(args[0]);
Super tiny comment, and you can fix this before integrating, but I always prefer a positive check. In this case it would be `args.length > 0` to show that we can read the first argument.
PR: https://git.openjdk.java.net/skara/pull/32
cli/src/main/java/org/openjdk/skara/cli/GitWebrev.java line 300:
> 299: .main(GitWebrev::apply)
> 300: );
> 301:
Since you are using `HttpClient` now we should make a call here to `HttpProxy.setup()`
PR: https://git.openjdk.java.net/skara/pull/32
webrev/src/main/java/org/openjdk/skara/webrev/WebrevMetaData.java line 45:
> 44:
> 45: public static WebrevMetaData fromWebrevURL(String uri) throws IOException, URISyntaxException, InterruptedException {
> 46: var sanatizedUri = sanitizeURI(uri);
Maybe you want to rename this to `from` and instead take a `java.net.URL` as type for the argument?
PR: https://git.openjdk.java.net/skara/pull/32
More information about the skara-dev
mailing list