Allow client to specify base commit to review

RESOLVED FIXED

Status

MozReview
General
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8520364 [details]
MozReview Request: bz://1089685/gps
Attachment #8520364 - Flags: review?(smacleod)
(Assignee)

Comment 2

3 years ago
/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
(Assignee)

Comment 3

3 years ago
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.
Assignee: nobody → gps
Flags: needinfo?(bmcbride)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

3 years ago
/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
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
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.
(Assignee)

Comment 8

3 years ago
/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
(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.
Flags: needinfo?(bmcbride)
(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.
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

3 years ago
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 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
Attachment #8520364 - Flags: review?(smacleod) → review+
(Assignee)

Comment 16

3 years ago
Comment on attachment 8520364 [details]
MozReview Request: bz://1089685/gps
Attachment #8520364 - Attachment is obsolete: true
Attachment #8618475 - Flags: review+
Attachment #8618476 - Flags: review+
Attachment #8618477 - Flags: review+
(Assignee)

Comment 17

3 years ago
Created attachment 8618475 [details]
MozReview Request: reviewboard: test that specifying a revision range for reviews work; r=smacleod
(Assignee)

Comment 18

3 years ago
Created attachment 8618476 [details]
MozReview Request: reviewboard: change multiple head detection; r=smacleod
(Assignee)

Comment 19

3 years ago
Created attachment 8618477 [details]
MozReview Request: reviewboard: support specifying base revision for review (bug 1089685)
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.