Proposal for pull request review process

[Joseph Rushton Wakeling]

On 07/30/2013 02:27 PM, Daniel Murphy wrote:
> We already do this for dmd by commenting with "LGTM, waiting for green". 
> Then if it goes green and the reviewer doesn't notice, the author can post 
> something like "It's green now!" on the pull.  This works quite well, as 
> another reviewer can pull without re-review if they notice first.

Well, my point is that once "LGTM" is given, the actual merge can be made
automatic conditional on the test suite passing.  It's an unnecessary bottleneck
to rely on reviewers (or code authors) taking time to check that an approved
pull request has gone green, and takes their time from other more productive
activities.

(In my experience "LGTM" doesn't always arrive, probably because the reviewer is
thinking, "I'll just merge it when it goes green", and then when the window of
opportunity is missed, it takes ages for green to come around again...:-)

> The priority is based on how recently the pull request was modified.  If you 
> find your pull request is somewhere at the bottom (or just not close enough 
> to the top) you can simply rebase it on the latest master and re-push it. 
> If you are not the author, a comment should be enough to ask the author to 
> do it.

Yes, I understand this, though personally I've always felt it rather rude to
push a rebased version like this just to speed up testing -- if lots of people
start doing this, the whole test priority queue will become a messy free-for-all.

That's why I propose a separate priority test queue for approved and
not-yet-approved pull requests -- get the approved stuff dealt with quickly and
out of the test system entirely.

But even without a separate queue, auto-merge-on-test-pass should help to avoid
the problem of pull requests getting tested over and over and over again simply
because of a lack of merge.

> This would be nice, and should be fairly straighforward to implement if 
> anybody feels like it.

Hint understood. :-)  I won't promise to follow up, as my time is constrained,
but I'm sure it would be interesting to look into the test suite and see if a
contribution is possible.

> I haven't found this to be a problem with dmd.  Waiting for the test suite 
> only seems to be a problem with really old pull requests, and they can be 
> bumped easily with a rebase/push.

I suspect there may be a difference between dmd, druntime and Phobos in how the
review process goes.  Apart from anything else the number of people who are
confident/expert enough to review compiler contributions is probably narrower
than the range of people who can work on the standard library.

But even with the number of reviewers being the bottleneck, automated merge
procedures should help those there are maximize their "useful" time, as well as
making testing more efficient by eliminating avoidable re-tests.

> I'm not saying these things wouldn't be good to have, but I suspect the 
> bottleneck is not enough people reviewing. 

More reviewers are always great and certainly necessary, but I can see there
being other scaling issues arising if we don't take advantage of automated
procedures to minimize the amount of time approved pull requests spend in the
test queue.

I'd see this as planning for the volume of pull requests _we'd like to have_ :-)