1089685 - Allow client to specify base commit to review

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Gregory Szorc [:gps]]

The review workflow via `hg push` doesn't currently allow clients to specify the base commit to review. Instead, we take a tip and take the non-published ancestors of that tip. We should allow clients to specify the base commit to review so clients can specify a narrower range of commits to review.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://1089685/gps

Comment 2

[Gregory Szorc [:gps]]

/r/393 - reviewboard: test that specifying a revision range for reviews work
/r/395 - reviewboard: change multiple head detection
/r/397 - reviewboard: support specifying base revision for review (bug 1089685)

Pull down these commits:

hg pull review -r c659ca39112e04b95415236e553ea878898d6b89

Comment 3

[Gregory Szorc [:gps]]

Blair: You said this feature was important to you. I just want to make sure the proposed syntax of `hg push -r <base> -r <tip>` or `hg push -r base::tip` to select commits to review is acceptable.

Comment 4

[Gregory Szorc [:gps]]

/r/393 - reviewboard: test that specifying a revision range for reviews work
/r/395 - reviewboard: change multiple head detection
/r/397 - reviewboard: support specifying base revision for review (bug 1089685)

Pull down these commits:

hg pull review -r 02366dac6f3717424b213b286a2d824c7e450bca

Comment 5

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/393/#review161

Ship It!

Comment 6

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/395/#review163

Looks good, just the one doc nit.

::: hgext/reviewboard/client.py
(Diff revision 1)
> -                raise util.Abort(_('cannot push multiple heads to remote; limit '
> +                realmissingheads = local.revs('children(%ln) & head()',

I think it might be worth sticking the code link in here as a comment (The one you gave me in irc[1]). It might get stale and not reflect the current state of the internal revset stuff, but at least it's *some* explanation.

Or if you'd prefer, a comment explaining the %ln syntax pulls in the hex from the list argument we pass would also be acceptable.

[1] http://selenic.com/repo/hg/file/18cc87e4375a/mercurial/revset.py#l2077

Comment 7

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/397/#review165

::: hgext/reviewboard/tests/test-push.t
(Diff revision 2)
> +  $ hg push -r 84e8a1584aad::b55f2b9937c7 --reviewid 7 ssh://user@dummy/$TESTTMP/server

I'd like to see a test of the `-r <base> -r <tip>` format.

Also, can we please test `-r <tip> -r <base>`, I'm unfamiliar with how pushop.revs will be ordered and it appears that we assume `pushop.revs[0]` will always be the base - This is true if we get things in dag ordering not command line ordering.

::: hgext/reviewboard/client.py
(Diff revision 2)
> +            basenode = pushop.revs[0]

How is `pushop.revs` ordered? Our documentation doesn't indicate you *have* to specify the base first with `-r`, so unless this is DAG ordered and not command line ordered either the code needs to change or the docs.

Comment 8

[Gregory Szorc [:gps]]

/r/393 - reviewboard: test that specifying a revision range for reviews work; r=smacleod
/r/395 - reviewboard: change multiple head detection; r=smacleod
/r/397 - reviewboard: support specifying base revision for review (bug 1089685)

Pull down these commits:

hg pull review -r a1d1907b3f4b3b56b8fbb2a8f95332218e4b2ca7

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Gregory Szorc [:gps] from comment #3)
> Blair: You said this feature was important to you. I just want to make sure
> the proposed syntax of `hg push -r <base> -r <tip>` or `hg push -r
> base::tip` to select commits to review is acceptable.

That would work (thank you!).

Are both base and tip needed? Seems a bit verbose for the default case.

Comment 10

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/395/#review169

Ship It!

Comment 11

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/397/#review171

Looks great, Ship-it!

Comment 12

[Steven MacLeod [:smacleod]]

(In reply to Blair McBride [:Unfocused] from comment #9)
> That would work (thank you!).
> 
> Are both base and tip needed? Seems a bit verbose for the default case.

Both aren't needed in the default case.

Scenario 1:
* Public0 <- Public1 <- C0 <- C1 <- C2
* Working Dir: C2
* Desired changes to review: C0, C1, C2
* Command: hg push review

Scenario 2:
* Public0 <- Public1 <- C0 <- C1 <- C2
* Working Dir: C2
* Desired changes to review: C0, C1
* Command: hg push review -r C1

Scenario 3:
* Public0 <- Public1 <- C0 <- C1 <- C2
* Working Dir: C2
* Desired changes to review: C1, C2
* Command: hg push review -r C1 -r C2
* Alternate Command: hg push review -r C1::C2

So you really only need to specify both when you have parent changesets which are draft, but you don't want them as part of the intended review request.

Comment 13

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/2a2910d56e9d

Comment 14

[Gregory Szorc [:gps]]

I guess Steven forgot to "Ship It" the parent review request. Rest assured, all 3 commits in the series received r+.

Even MozReview authors can't get the workflow right :)

Comment 15

[Steven MacLeod [:smacleod]]

Comment on attachment 8520364 [details]
MozReview Request: bz://1089685/gps

(In reply to Gregory Szorc [:gps] from comment #14)
> Even MozReview authors can't get the workflow right :)

I'd prefer "make mistakes" rather than "can't get the workflow right", but yeah lets make the UX nicer :D

Comment 17

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: test that specifying a revision range for reviews work; r=smacleod

Comment 18

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: change multiple head detection; r=smacleod

Comment 19

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: support specifying base revision for review (bug 1089685)