on code review and commit policies

I’ve been doing what feels like a lot of reviews this year.  Groveling imperfectly through mozilla-inbound’s commit log:

[froydnj@cerebro mi.hg]$ hg log -d '>2014-01-01' -k r=froydnj -k r=nfroyd|grep -c summary:
189

tells me I’ve been doing slightly over 1 review/day, which seems like a healthy clip.  Most of these reviews are hardly rocket science: some minor changes to make a compiler happy(ier), some sweeping style changes, and tweaks to make things more robust against previously unforeseen problems.  I call out reviews that take me “significant” amounts of time in my weekly status updates to my team; last week was the first week in a while where I could say that a review would lead to significantly better code being committed into mozilla-central.

You could take this as commentary on the generally high quality of patches that I get to review (the “our contributors are incredible” argument). You could take this as people asking me to review fairly trivial patches and saving the “real” patches for “real” reviewers (the “inferiority complex” argument). You could also take this as evidence that not all patches need an explicitly-requested and mandatory code review prior to commit (the “you’re doing code review wrong” argument).

All the projects I’ve worked on in my professional life have what I’ll call “hard” code review policies: everything that gets committed must undergo review, save for “obvious” patches. Definitions of “obvious” vary, but spelling fixes, backing out build/test bustages, and no-questions-asked correctness fixes generally qualify. (GDB has a “will the person who hates my work the most be able to find fault with the change” test for obviousness that seems both whimsical and helpful.) Such a policy applies to everyone, from the person who just submitted their first patch to the veteran working for 10+ years on the same project.

Other, equally successful projects that I’ve had contact with have what I’ll call “soft” code review policies: patches from people without commit bits undergo review, but being given commit privileges is an expression of trust. (This trust is qualitatively different from the trust expressed in granting commit privileges in the “hard” review model.) Your prior work has demonstrated sufficient taste and competence that you may commit patches freely, but you are also expected to exhibit appropriate humility in knowing when to ask for a second opinion. Of course, your code may undergo post-commit review by interested parties, where changes ranging from minor edits to complete backouts may be requested.

What percentage of commits benefit from a “hard” review policy? Can we classify patches so as to apply the “correct” review policy? (Can the people writing the patches even identify the “correct” review policy themselves?)

6 comments

  1. FWIW my anecdotal experience is that you get better, more useful, code reviews when you have better code review tools. The tools at Mozilla are pretty terrible, so that’s another potential explanation for the fact that you don’t think your reviews are all that useful. We are supposed to be getting some sort of modified Review Board install — which may or may not be “good”, but will hopefully at least be better — so it will be interesting to see how that changes your experience.

    • Nathan Froyd

      Well, my poorly communicated point isn’t that the tools are somehow limiting the quality of the review, but that the majority of reviews have a usefulness that’s very small. Better tooling isn’t going to change that any. Having to wait for review on such patches limits what you can do, or at least discourages people from doing certain things. Maybe it’s not worth requiring review for everything, but only for “architecturally significant patches” or somesuch.

  2. Sure, I understand what you were trying to claim. But I don’t really know how to assess the veracity of your claim, particularly in the presence of so many environmental factors. Certainly I would make the counter-claim that I have both found significant problems, and had my own errors pointed out, in reviews of small, “architecturally insignificant” patches. I also find like the code review process at Mozilla feels like more effort for less value than other review systems I have used. So it doesn’t seem unreasonable to suggest that changing that might change your overall perspective here.

    • Trevor Saunders

      We can be fairly sure that just about any patch can contain a bug, and so given enough of them one will whoever is writing them. However if very few bugs are caught by reviews even if we don’t consider the time of the reviewer it still impacts the author to have to wait for the review which will never be instant. Also not requiring review before checkin doesn’t imply no review at all.

      I’d probably be infavor of say allowing peers and module owners to make changes to that module without review, which is more or less another part of the gdb policy.

  3. I’ve been involved in two open source communities that have code review. At Eclipse, once you gained commit rights, you could commit all you like without code review. (Contributors without commit rights must have all patches reviewed and landed by a committer). A soft review policy in your words. The only exception was as we approached a release date. As we started release candidates, you had to have your patches reviewed by progressive numbers of committers until release day.

    At Mozilla, all the patches I write must be reviewed. Not only is this better from a code quality perspective, it helps me learn how to implement things better from people who have more experience with the code base. Aside from my own personal learning, code review also diffuses the knowledge of the code base within a larger group of people. One of the problems we had at Eclipse was that one person would often be the sole contributor to a particular component. So when they left the company/community nobody else would have any experience with that component. Since there was no code review, nobody else had any experience with the code base or understanding of the history of changes to it. Not good.

  4. $ hg log -d ‘>2014-01-01′ -k r=khuey -k ,khuey |grep -c summary:
    213

    That’s probably 2x what I would have estimated … I guess I know where all my time goes now.