www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Proposal for pull request review process

reply Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
Hi all,

I originally posted this idea in the course of a thread in D.announce but
thought I'd put it here so that more people can pitch in.

It's generally recognized that the reviewing and merging pull requests is one of
the major bottlenecks in D's development process.  My own experience submitting
patches to Phobos (I can't speak for DMD or druntime) has been that often the
actual _review_ part is quite quick, but that the delay is in going from a patch
being approved to being merged.

Confusion can be added because reviewers don't always indicate explicit approval
of the code, so the submitter can be sitting in limbo not knowing whether the
problem is that their code is inadequate or the reviewer just not getting round
to merging it yet.

From the reviewer's point of view, on the other hand, the problem is that they

for test-suite passes, it's easy to miss an opportunity to merge and instead have to sit through another round of testing that happens due to another patch being merged first. This only needs to happen a few times before the code under review gets shunted to the bottom of the priority queue for testing, so increasing the wait between opportunities to merge (and, without alerts, the likelihood to miss them). So, I'd propose that the review process include a way for the reviewers to indicate that a pull request is approved subject to passing the test suite, and that it be obligatory for reviewers to provide such an indicator. Then, approved pull requests would go on a separate priority test queue with "first in, first out" priority. If the test suite passes, the pull request is auto-merged, if it fails then it is removed from the queue and must be re-approved. Ideally it should be possible to distinguish actual test failures from something going wrong with the test procedure itself (e.g. a test process not spawning correctly), and in the latter case keeping the pull request in the approved queue. The result should be that reviewers can focus on _reviewing_, and don't need to worry about waiting around for the test suite -- they can just approve in the safe knowledge that the merge will happen (or not) depending on the test results. Does this sound like something good to have, that would be feasible to implement? Thanks & best wishes, -- Joe
Jul 28 2013
next sibling parent "Daniel Murphy" <yebblies nospamgmail.com> writes:
"Joseph Rushton Wakeling" <joseph.wakeling webdrake.net> wrote in message 
news:mailman.289.1375044093.22075.digitalmars-d puremagic.com...

 So, I'd propose that the review process include a way for the reviewers to
 indicate that a pull request is approved subject to passing the test 
 suite, and
 that it be obligatory for reviewers to provide such an indicator.

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.
 Then, approved pull requests would go on a separate priority test queue 
 with
 "first in, first out" priority.  If the test suite passes, the pull 
 request is
 auto-merged, if it fails then it is removed from the queue and must be 
 re-approved.

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.
 Ideally it should be possible to distinguish actual test failures from 
 something
 going wrong with the test procedure itself (e.g. a test process not 
 spawning
 correctly), and in the latter case keeping the pull request in the 
 approved queue.

This would be nice, and should be fairly straighforward to implement if anybody feels like it.
 The result should be that reviewers can focus on _reviewing_, and don't 
 need to
 worry about waiting around for the test suite -- they can just approve in 
 the
 safe knowledge that the merge will happen (or not) depending on the test 
 results.

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.
 Does this sound like something good to have, that would be feasible to 
 implement?

I'm not saying these things wouldn't be good to have, but I suspect the bottleneck is not enough people reviewing.
Jul 30 2013
prev sibling parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
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_ :-)
Jul 30 2013