Nathan Hartman wrote on Thu, 22 Apr 2021 21:41 +00:00: > Not knowing whether / how many people have reviewed a particular > commit is, as was said elsewhere, a silent failure mode of the CTR > (commit-then-review) convention. > > Do we want to try switching to a RTC (review-then-commit) convention?
If we do switch to RTC, we might want to also retroactively ensure all commits post 1.14.x's branching have been reviewed by at least two pairs of eyes each. However, I wonder whether there's a smaller change we can do first, rather than a full-blown s/CTR/RTC/ flag day. For instance, how about we agree, for the next N weeks, to make our commit reviews explicit? I.e., to explicitly say "I've reviewed this commit and found no issues"? (Call this Commit-Then-Explicit-Review.) At the end of the N weeks, we can look at the experience and data we'll have then, and decide how to continue: whether to revert to CTR, switch to RTC, keep the new system and implement supporting tooling, etc.. > Some committers are already posting patches for review before > committing them. Another project I work on adopted the convention that > no one commits their own patches. It seems to work well there. Yes, > mistakes still get through sometimes, but at least we know that each > patch has had at least two pairs of eyes on it: the author's and the > committer's. OpenBSD uses a two-man rule too (or at least did, when I last checked), so stsp may have useful input here. > In our case, I don't think that convention should be > applied everywhere. Branches, the site, documentation, areas outside > the core code, don't need this extra step -- though in the case of > r1888589 the code in question is part of tests. That doesn't mean it's > less important. We do rely on the test suite and buildbots to alert us > to problems. Cheers, Daniel
