Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use positional argument if present #75

Closed
wants to merge 1 commit into from

Conversation

lkorinth
Copy link

@lkorinth lkorinth commented Aug 23, 2019

I hope this implements what is documented in the help message:
usage: git webrev [options] [<rev>]

Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

@bridgekeeper bridgekeeper bot added the oca label Aug 23, 2019
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 23, 2019

Hi lkorinth, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user lkorinth as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@openjdk openjdk bot added the cli label Aug 23, 2019
@bridgekeeper bridgekeeper bot removed the oca label Aug 23, 2019
@openjdk openjdk bot added the rfr label Aug 23, 2019
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 23, 2019

Webrevs

@edvbld
Copy link
Member

@edvbld edvbld commented Aug 23, 2019

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).

@lkorinth
Copy link
Author

@lkorinth lkorinth commented Aug 23, 2019

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.

@lkorinth lkorinth closed this Aug 23, 2019
@lkorinth lkorinth deleted the _positional branch Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants