On Tue, May 31, 2011 at 12:09 PM, Russell N. Nelson - rnnelson <
rnnelson(a)clarkson.edu> wrote:
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.
For the same reason my position is that a "minimum time before revert" is
not desireable -- if anything is ever found to be problematic, the correct
thing is to remove it immediately.
Robla counters this with the observation that when there are multiple people
handling code review, simply being *uncertain* about whether something needs
a revert or not is better grounds for *passing it on* to someone else to
look at than to immediately revert.
This should usually mean a few minutes on IRC, however, not just a 72-hour
silence without feedback!
What really worries me though isn't the "somebody else built a patch on this
within 72 hours, now it's slightly awkward to revert" case but the "other
people built 100 patches on this in the last 10 months, now it's effing
impossible to revert".
Ultimately, we need to remember what reverting a commit means:
** Reverting means we're playing it safe: we are taking a machine in unknown
condition out of the line of fire and replacing it with a machine in
known-good condition, so the unknown one can be reexamined safely in
isolation while the whole assembly line keeps moving. **
It's not a moral judgment against you when someone marks code as fixme or
reverts it; it's simple workplace safety.
-- brion