RFR: Use positional argument if present

Leo Korinth via github.com duke at openjdk.java.net
Fri Aug 23 12:13:53 UTC 2019


On Fri, 23 Aug 2019 09:11:15 GMT, Erik Duveblad via github.com <duke at openjdk.java.net> wrote:

> 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

I am willing to try to create the hg solution (x)or the git one. I should fix the help message _and_ err on positional argument regardless, right?

I do not see how the git one breaks backward compatibility with the current solution, but I will try out the hg -r -r approach as it works for _both_ git and hg easily and we can see if I succeed to create something acceptable.

Today's workaround of mutating the working copy is IMO quite limiting --- especially in scripting.

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


More information about the skara-dev mailing list