Proposal for pull request review process

[Joseph Rushton Wakeling]

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
can't merge code until it's passed the test suite, and as there are no alerts
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