I feel that the code review process is going too fast. Sometimes I see patches being submitted and merged within the very same weekend. Now I just reviewed a patch for which I received an invitation yesterday evening, but I’m too late already: It has already been merged.
Do you expect reviewers to be available 24/7?
Otherwise, wouldn’t it be reasonable to give reviewers a little bit more time, for example by:
avoiding merges to the master during the weekend
allowing 24 hours between the final patch set and the merge to the master
?
I would just like to add that this is especially the case for feature patches. Patches that correct bugs and broken dashboards, in certain cases, need to be merged fast.
@Niels_Dekker did you want to submit a pull request to the ITK Software Guide repository? A note could be added to the Software Process chapter stating that proposed patched should not be merged until one business day has passed to allow for code reviews. An exception to this rule would be patches that clean up the dashboard or address critical bugs.
I understand well that some critical bug fixes cannot wait for 24 hours. But do you think that a patch that would only fix a rather harmless compiler warning (thereby cleaning up the dashboard) should also get a shorter review time?
I would prefer to only have a shorter review time for critical bug fixes and compilation error fixes.
Of course, critical bug fixes do often need a careful review anyway.
Most people work on ITK on the workdays. But some (@hjmjohnson?) seem to be working on the weekends.
I think that asking for a whole workday to pass before non-critical patches are merged is reasonable. Perhaps this condition could be waived if multiple people review the patch quickly?
@dzenanz It’s fine if people like to work in the weekend, of course. But others may have a party on Saturday and a hangover on Sunday Those ones should still be allowed to review during workdays, right?
Even if multiple people have reviewed a patch quickly, someone who is explicitly invited to review the patch should get a fair chance to do a more thorough review, in my opinion.
It would be nice if Gerrit would have a “Slow Down” button, to be pressed by (potential) reviewers. Or if Gerrit would provide an indication how much time is left for reviewing. (If you assume, for example, 24 hours review time by default, it would not be too hard to implement.)
Of course, there should always be room for exceptions to the rules, but I think some guidelines would be helpful.
Having some policy that we agree upon may be sensible.
Some suggestions, such as relying on the type of topic/patch, are interesting to accommodate the process.
Having a deadline or “slow down” button, requiring the approval of some maintainers, or a number of people, etc. seem desirable also.
But implementing these in GitHub (remember that ITK is transitioning to GitHub) or requiring the GitHub people do it may not be easy/out of reach.
Having said that, the truth is that:
Current maintainers are mostly based in the US, and accommodating different time zones may not be that easy, and workdays may also differ across countries, across companies, etc.
So this will inevitably happen.
We should acknowledge that maintainers’ institutions may also have other constraints (funding agencies, etc.) that one, including me, as a community member, may be unaware of.
People having push access are trustworthy and work to the best of their possibilities, including outside work hours, to make ITK better for the community, and the funding agencies, institutions, scientific community. Even if this means merging within short delays.
A good feature, module, class or awesome contribution in terms of bug fixes can beat missing a review. So there is always a chance to improve ITK !!
Just a short explanation @Niels_Dekker: the gerrit review system automatically adds people as reviewers when it detects that a given user made some commit on the file at issue at some point in history.
Those ones should still be allowed to review during workdays, right?
, and the funding agencies, institutions, scientific community. Even if this means merging within short delays.