RFR: Add tool for importing webrev patches

Erik Duveblad via github.com duke at openjdk.java.net
Wed Sep 4 12:33:31 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

args/src/main/java/org/openjdk/skara/args/MultiCommandParser.java line 41:

> 40:         if (defaults.size() != 1) {
> 41:             throw new IllegalStateException("Expecting exactly one default command");
> 42:         }

`IllegalArgumentException` instead? The `commands` are passed in :)

PR: https://git.openjdk.java.net/skara/pull/32

args/src/main/java/org/openjdk/skara/args/MultiCommandParser.java line 70:

> 69:         } else {
> 70:             return this::showUsage;
> 71:         }

Shouldn't this return the default command if one is present? Otherwise how will just `git webrev` work?

PR: https://git.openjdk.java.net/skara/pull/32

args/src/main/java/org/openjdk/skara/args/MultiCommandParser.java line 61:

> 60:             if (p != null) {
> 61:                 String[] forwardedArgs = Arrays.copyOfRange(args, 1, args.length);
> 62:                 return () -> p.main(forwardedArgs);

This is a nice place for `var` ;)

PR: https://git.openjdk.java.net/skara/pull/32

vcs/src/main/java/org/openjdk/skara/vcs/hg/HgRepository.java line 940:

> 939:         apply(patchFile, force);
> 940:         //Files.delete(patchFile);
> 941:     }

We should probably uncomment this line :)

PR: https://git.openjdk.java.net/skara/pull/32


More information about the skara-dev mailing list