"Russell N. Nelson - rnnelson" <rnnelson(a)clarkson.edu> wrote in message
news:777BC8A5A78CC048821DBFBF13A25D39208F1141@EXCH01.ad.clarkson.edu...
Robla writes:
1. We say that a commit has some fixed window (e.g. 72 hours) to get
reviewed, or else it is subject to automatic reversion. This will
motivate committers to make sure they have a reviewer lined up, and
make it clear that, if their code gets reverted, it's nothing
personal...it's just our process.
Worse than pre-commit review. What if you make a change, I see it, and
make changes to my code using your changes. 72 hours later, they get
reverted, which screws my code. Okay, so then nobody's going to compile
anything, or use anything, if it has 72-hour code in it. The alternative,
if they want the code badly enough, is to review it so it will stick.
Well, that devolves to the same thing as pre-commit review.
And ... this only makes sense for core, or maybe for stable extensions. It
doesn't make sense for experimental extensions where only one person is
likely to understand or care what the code says.
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**.
Every way of phrasing or describing the problem with MW CR can be boiled
down to one simple equation: "not enough qualified people are not spending
enough time doing Code Review (until a mad rush before release) to match the
amount of code being committed". Any attempt at a solution which does not
change that fundamental equation is doomed to failure. There are at least
six things that could be changed ("enough people", "qualified people",
"enough time", "Code Review", "match[ing]", "being
committed"); we almost
certainly need to change more than one. The most likely outcome of this
particular suggestion is simply to radically reduce the amount of code being
committed. I'm not sure that that's the best way to deal with the problem.
--HM