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