On Thu, Apr 22, 2021 at 5:12 PM Daniel Shahaf <d...@daniel.shahaf.name> wrote: > > Johan Corveleyn wrote on Thu, 22 Apr 2021 19:53 +00:00: > > Perhaps the lack of review-activity for us as a CTR project is more > > critical, I don't know. Good observation in any case. > > Note some of these issues are related: Approving backports amounts to > reviewing some specific commits carefully; work on security issues > involves some reviewing, some RMing, and some signing. > > I'd be hesitant to say "more critical" because I'm not sure the > criticalities of these five areas are orderable, but historically, > I think we rely on commit reviews to catch certain classes of bugs: for > instance, cross-version compatibility (in the ABI, over the wire, in > on-disk formats) and FSFS concurrency guarantees are both core promises > that have little test coverage. > > And, of course, there's any number of C gotchas and portability quirks > that our compilers don't warn on (in the default build configuration).
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? 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. 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. Nathan
