On Wed, Jun 1, 2011 at 11:23 AM, Roan Kattouw <roan.kattouw(a)gmail.com> wrote:
On Wed, Jun 1, 2011 at 1:49 AM, Happy-melon
<happy-melon(a)live.com> wrote:
By far the most likely outcome of this is that in
the two months following
its introduction, 95% of all commits are reverted, because the people who
are supposed to be reviewing them don't spend any more time than usual
reviewing them. If I make a change to the preprocessor, HipHop, Sanitizer,
SecurePoll, passwords, tokens, or any of a number of other things, I'm going
to need Tim to review them. I don't begrudge Tim the thousand-and-one other
things he has to do besides review my code within 72 hours. Does that mean
that no one should make *any* changes to *any* of those areas? More
dangerously still, does that mean that **only people who can persuade Tim to
review** be allowed to make changes to those areas? I know what the answers
to those questions are *supposed* to be, that's not the point. The point is
**what actually happens**.
This, for me, is the remaining problem with the 72-hour rule. If I
happen to commit a SecurePoll change during a hackathon in Europe just
as Tim leaves for the airport to go home, it's pretty much guaranteed
that he won't be able to look at my commit within 72 hours. Or maybe
I'm committing an UploadWizard change just after 5pm PDT on Friday,
and the next Monday is a federal holiday like the 4th of July. Or
maybe the one reviewer for a piece of code has just gone on vacation
for a week and we're all screwed.
So yes, before we implement this we need to 1) ensure that reviewers
spend enough time reviewing and 2) figure out what to do with
one-reviewer situations.
I don't think "revert in 72 hours if its unreviewed" is a good idea. It
just discourages people from contributing to areas in which we only
have one reviewer looking at code.
I *do* think we should enforce a 48hr "revert if broken" rule. If you
can't be bothered to clean up your breakages in within 48 hours of
putting your original patch in, it must not have been very important.
-Chad