RFR: Use positional argument if present

Erik Duveblad via github.com duke at openjdk.java.net
Fri Aug 23 09:11:15 UTC 2019

On Fri, 23 Aug 2019 08:57:48 GMT, Leo Korinth via github.com <duke at openjdk.java.net> wrote:

> I hope this implements what is documented in the help message:
> ```usage: git webrev [options] [<rev>]```
> ----------------
> Commits:
>  - fb4660f0:	Use positional argument if present
> Pull request:
> https://git.openjdk.java.net/skara/pull/75
> Webrev:
> https://webrevs.openjdk.java.net/skara/75/webrev.00
> Patch:
> https://git.openjdk.java.net/skara/pull/75.diff
> Fetch command:
> git fetch https://git.openjdk.java.net/skara pull/75/head:pull/75

Hi Leo, thanks for contributing! The problem here is actually that the help text is wrong, not the code...We want to keep backwards compatability with `webrev.ksh` and `webrev.ksh` only accepts an optional revision to compare against by using the `-r, --rev` argument.

With regards to the patch itself, it actually doesn't do what you think it does. The `Webrev` API is a bit cumbersome (thanks to me, I wrote it), but the arguments to `generate` are actually `from` and `to`. If the user does not use the `-r,--rev` option then the `rev` variable becomes `HEAD` and the call to `generate` would then become `generate(HEAD, <REV>)`, where `<REV>` is what the user supplied as the positional argument 0. That wouldn't work, think of a `git diff` expression like `git diff HEAD..<R>`, you can't compare from `HEAD`, because `HEAD` does not have descendants (that is the definition of `HEAD` after all).

All in all, I would suggest closing this PR. I _know_ that some people want to explicitly create a webrev between two commits, and the `Webrev` API supports it, I'm just hesitant to lose backwards compatability with `webrev.ksh` (webrev.ksh has never supported this). A `git` style solution would be to only support a positional argument (`git webrev` used to do this), a `hg` style solution would be to support multiple `-r` arguments. The `git` style solution would mean breaking backward compatability, the `hg` style solution could work.

The workaround today is the same as for `git webrev` as for `webrev.ksh`: if you want to generate a webrev from commit A to commit B, you checkout/update B and then do `git webrev -r A` (or `webrev.ksh -r A`).

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

More information about the skara-dev mailing list