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_ :-)
