"Russell N. Nelson - rnnelson" <rnnel...@clarkson.edu> wrote in message
news:777bc8a5a78cc048821dbfbf13a25d39208f1...@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
_______________________________________________
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
