On Thu, Nov 19, 2015 at 6:49 PM, Greg Stein <gst...@gmail.com> wrote:
> Todd: as Ross notes, your three points about code reviews in a CTR project > are quite valid, and match accepted engineering practices. > > What I haven't seen is an explanation why a committer must be treated the > same as a drive-by. Both are subject to requiring "permission"[1] to make > even the simplest of changes under RTC. Even worse, from else-thread, it > sounds like under your control scheme, you don't even allow the committer > to apply their own change [after review]. They can apply their own change once someone else has +1ed it. On Hadoop, for example, the usual workflow when I review another committer's patch is that I give them a +1 and they commit it themselves. On gerrit or github, because the actual "commit" process is just clicking a button on a web UI, it's more normal for the reviewer to be the one to commit it after giving the +1 review, but both happen and either one's fine. > A committer can give a binding +1 > to somebody else's work. But they aren't trusted to give that to their own > work. Just like a drive-by contributor can't be trusted. > Right, they can't give it to their own work because it defeats the purpose of the code review (discussed earlier). Of course it's not hard and fast -- eg fixing a broken build due to a missing 'import' statement or something would be totally fine to commit without review, or fixing a grammar mistake in a comment, or anything else that's obviously trivial. But once actual code is changing, it's expected to get two pairs of eyes. -Todd
