The Gentle Art of Patch Review

As the next round of the FOSS Outreach Program for Women (OPW) approaches, my mind turns to mentorship, and lessons learned when dealing with newcomers to open source projects.  Many open source contributors have been in the FOSS community for long enough to forget how painful their first experience contributing to a project was.  As the coordinator for the Linux kernel OPW internships, I get to help newcomers go through that experience every six months.  I’ve learned a lot about how we, as open source reviewers, maintainers, and mentors, can help newcomers during their first contributions, and I’d like to share some of the perspective I’ve gained from OPW.

The Newcomer’s Perspective

As a newcomer, you’ll come at the project with enthusiasm and determination to do your best to make a really good first contribution.  You’ll try to find all the documentation for the project you’re working on, and read through it, only to realize it’s completely outdated and incomplete.  You’ll ping mentors and ask questions, but you may not be able to reach the right person to answer your question.  So you do the best you can with the resources you find, cross your fingers, and submit your first contribution.

It’s common for newcomers to blame themselves when they make mistakes in their first contributions.  You’ll cringe, wring your hands, smack your forehead, or maybe even put your head in your hands.  Then you’ll sigh and try again.  No matter how good the documentation for contributing to the project is, how meticulous you are, you will slip up at some point.  And that’s fine, because you are going through a process of learning something new, and expanding your skills.  The most productive contributors see each mistake they make as a growth opportunity, instead of a personal failure.

The Maintainer’s Perspective

As a long-standing open source contributor, you may get contributions from newcomers all the time.  You’ll see several of them make the same mistakes over and over again, and if you have enough time, you’ll update your project documentation to help people avoid those mistakes.  Often you don’t have time, and the documentation doesn’t get updated.  Or you’ll think that something is so blindingly obvious that everyone should understand it, without realizing how much specialized experience you need to have that knowledge.

At some point in as a maintainer, you will be completely overloaded with contributions from both newcomers and familiar, trusted contributors.  It’s easy to review those contributions from long-standing contributors, because they know your expectations and the rules around contributing.  You trust them to write solid code containing very few bugs.  So you review the contributions from trusted contributors, and put off reviewing contributions from newcomers until you have a large block of time to thoroughly review the newcomer’s contribution.

It’s tempting to just go through the newcomer’s contribution from start to finish, commenting on every single thing they missed.  The maintainer’s mindset is, “Ok, I have time, I should share my knowledge with this person who is obviously missing some tribal knowledge they need to contribute to my project.”  From the newcomer’s perspective, what they experience is their contribution being ignored for days or even weeks, followed by a very long email full of nit-picky comments on coding style, criticism of their code structure, and even comments about their spelling and grammar.  Even if the review is fair and neutrally worded with a focus on their technical mistakes, it still feels very harsh.

We Can Do Better

How can we make this process better on both sides?  How can we make the first patch review less harsh, and still respect the maintainer’s valuable time?  Can we make turn around time on first patch review even shorter?  When I was the xHCI driver maintainer, I started experimenting with a different way of reviewing contributions from newcomers that I think might help address all three of these issues.

The Three-Phase Contribution Review

Instead of putting off reviewing first-time contributions and thoroughly reviewing everything in the contribution at once, I propose a three-phase review process for maintainers:

  1. Is the idea behind the contribution sound?
  2. Is the contribution architected correctly?
  3. Is the contribution polished?

You can compare these contribution review phases to the phases of building a new house or taking on a remodeling project.  The first phase is a simple yes or no on the architectural diagram, the big idea of the contribution.  The second phase is getting all the structural issues correct and making sure the plumbing and electrical all connect properly.  The third phase is making everything polished, sanding off the rough corners, and slapping on a coat of paint to match whatever color the bike shed is currently painted.

Phase One: Good or Bad Idea?

The first phase of the contribution review should only require a simple yes or no answer from the maintainer: “Is this contribution a good idea?”  If the contribution isn’t useful or it’s a bad idea, it isn’t worth reviewing further.  The best action in this case is to refocus the newcomer on a better idea or a completely different area they could work on.  Or open a discussion with the newcomer and other contributors as to what should be done to address the issue in a different way.

If the contribution is worthwhile, but you don’t have time to go onto the second phase of patch review, do NOT say nothing.  Instead, drop the contributor an email that says, “Thanks for this contribution!  I like the concept of this patch, but I don’t have time to thoroughly review it right now.  Ping me if I haven’t reviewed it in a week.”  This builds the newcomer up by expressing appreciation for the time and effort they put into creating this contribution, and lets them know they’re on the right path.  It also gives you incentive to actually move onto phase two, because the contributor will bug you again if you haven’t reviewed the contribution.

Phase Two: Is this Architecturally Sound?

In phase two, you review the contribution to see whether the code (and only the code) is architecturally correct.  Focus on whether the code is sound at an architectural level. Is the code behavior correct?  Are they modifying the right functions, or does the code need to be moved around?  Have they structured their build files correctly?  Do they need to refactor any code?  Do they need to get buy-in on the code structure from other maintainers?  Are there potential hazards or tricky parts of the code that the everyone needs to review carefully?

You will need to squash the nit-picky, perfectionist part of yourself that wants to comment on every single grammar mistake or code style issue.  Instead, only include a sentence or two with a pointer to coding style documentation, or any tools they will need to run their contribution through.  If their patch needs to be updated against a newer version of your project, or a different maintainer’s upstream repository, point that out.  Avoid nit-picking every instance where they violate your project’s contribution style rules. Your eyeballs may be bleeding from the number of camel case variable names or variables names that use variable type encoding, but take a deep breath and ignore that.  Let them explore the tools, documentation, and fix (most) of their mistakes on their own.

Double check and make sure the documentation and tools actually document the mistakes you see in the code, and if they don’t, update them.  Your documentation and tools should clearly spell out the format of a valid contribution, and if they don’t, you need to address that technical documentation debt.  If you don’t have time to address that technical documentation debt, tell the contributor what needs to be fixed, and see if they have the time to address it.  Don’t be silent just because you don’t have time to fix it.

Phase Three: Is the Contribution Polished?

From a newcomer’s perspective, after phase two is complete, they’re hooked on getting their contribution in.  You’ve worked with them on an architectural level, and they know you want to accept their contribution.  They’re emotionally invested in getting their contribution into your project, and they’ve learned a lot by going through a couple contribution revisions.  Thank the contributor for being patient this far and remind them that you’re willing to accept the contribution, but they need to clean up a few small things first.

Now is the time for phase three: the polishing phase.  In this phase, you finally get to comment on the meta (non-code) parts of the contribution.  Correct any spelling or grammar mistakes, suggest clearer wording for comments, and ask for any updated documentation for the code.  It doesn’t make sense to create documentation for the code until the code is structurally sound, which is why the documentation phase comes last.  You may also need to encourage them to write a better commit message, mark the patch to be back ported to stable versions of your software, or Cc the right maintainers.

As a newcomer, this third and final phase can be more painful than the architectural critiques in the second phase.  Many young programmers lean towards science, math, and technology because they feel like they don’t excel in writing or people skills.  Contributors may also be writing in a language that is not their native tongue.  That’s why this nit-picky phase comes last, so that the contributors get over their embarrassment after they’re emotionally invested in getting their patches into your project. Be gentle, patient, and compassionate.  As a maintainer, you may suggest comments or patch descriptions that you hope the contributor simply copy-pastes into their patch.  You may have to just edit the patch description yourself.

How Does This Benefit Maintainers?

I’ve found that this three-phase contribution review process saves me (as a maintainer) a lot of mental stress.  The first phase is a simple yes or no question (“Is this a good or bad idea?”), which means I don’t procrastinate on reviewing first time contributions.  Being up front with contributors about not having time to review their contribution can initially feel like shirking duties, but I feel a mental load lifting when I get over that and simply say something like, “Hey, this patch looks like a good idea, but I don’t have time to review it right now. I’m heading to a conference next week, and need to work on my slides.  Can you ping me in two weeks if I haven’t reviewed your code?”

If you’re honest with contributors about your time commitments, they know their contribution is wanted, and they can pass your time commitments onto their boss or program manager.  Also, if you find yourself delaying contribution review often, it may be a sign you need a co-maintainer or you need to ask other contributors to do more code review.

The absolute worst thing you can do during phase one is be completely silent.  The newcomer doesn’t know whether their contribution is a good or bad idea, and any discussion that needs to happen with other maintainers to modify the fundamental concept never happens.  That’s why phase one is a simple yes or no answer, in order to get the code review ball rolling.

I’ve also heard some maintainers state that they want to dump all their review into phase two.  They have precious little time, and they fear they will forget specific feedback if they break code review into several phases.  I will often notice nit-picky coding style issues during my architectural review, and I will make a note to myself to nip that pattern in the bud in phase three.  Keeping a dated text file per patchset or even replying to the patch but only adding your own email address in the To field will help you keep track of the issues that need to get addressed in phase three.

Often by the time you get past the architectural discussion in phase two, you’ll find many of your initial nit-picky criticisms were addressed.  A conscientious contributor will look at the documentation and tools you point out in phase two, and will address most of them in their next revisions.  What will be left for the third (polishing) phase is mistakes made because of undocumented tribal knowledge, or rules that are undocumented because they differ from maintainer to maintainer within the project.

Try It Out!

The following three-phase contribution review process should help both maintainers and newcomers:

  1. Is the idea behind the contribution sound?
  2. Is the contribution architected correctly?
  3. Is the contribution polished?

Maintainers will be able to respond more quickly to contribution review if they focus on just answering one question during the first phase of review: “Is this a good or bad idea?”  Newcomers will be encouraged by a timely email that states whether the basic concept of their patch is sound.  Both the maintainer and the contributor benefit from splitting the actual code review into an architectural discussion, followed by a polishing phase.  Maintainers will save themselves time if they simply point out documentation and tools contributors should use to ensure their contribution is up to community standards, and the nit-picky polishing phase is saved for after the newcomer is emotionally invested in getting their contribution into your project.

I think this process should both save maintainers time, and decrease the bounce rate for newcomers, so I encourage you to try it out!

17 thoughts on “The Gentle Art of Patch Review

  1. This is a very sound approach to reviewing contributions. The good or bad idea phase would help newcomers from devoting too much to a patch that may never get accepted! Saving time and redirecting efforts toward more worthwhile patches that will get accepted is definitely more helpful to the project and the newcomer.

  2. Thank you for posting this taxonomy of review phases!

    I’ve noticed that there are a surprisingly large number of people who are not capable of giving review of the type mentioned in phase 1 or most of phase 2, and are only capable of cursory phase 2 review and lots of phase 3 review.

    Pointing to documentation and tools in phase 2 and letting prospective contributors handle most of the style issues themselves seems like a massive improvement. Whatever’s left in the resulting phase 3 review (if one is needed at all) will likely be either things the contributor missed (in which case we should evaluate if they’re sufficiently well explained), tribal knowledge (in which case we should document that knowledge), and questions of subtle taste that don’t fit into a particular pattern (hopefully uncommon).

  3. Pingback: The Gentle Art of Patch Review | The WordPress C(h)ronicle

  4. Incredibly useful Sarah. Thanks so much. Will be sharing widely with the Drupal community.

    Reviewing is a critical part of the open source software development process. The act itself is also, often, not given the credit and acknowledgement it deserves.

    D.

  5. This is really great! Thanks so much for writing it. I definitely have a tendency to give all three phases of feedback at once, and I will keep this approach in mind.

    I don’t agree, though, that “it doesn’t make sense to write the documentation before the code.” I often write docs before code, and I feel that in many cases attempting to write readable docs for a feature can help clarify step one (is this a good idea?) and step two (do we have the public API / interface right?).

    That said, I agree that asking the contributor to write docs first (or at all) may not be a good choice in many cases, depending whether documentation-writing is an area they feel confident in.

  6. Thanks for this writing. In fact, we love to see this ideology can be pushed further and gained acceptance among maintainers & contributors. Maybe some kind of “ALS Ice Bucket Challenge … to stir the viral acceptance into such practice ?” If you don’t mind, I would share this article with some of new comers in open-source within my group.

  7. This is a great post, Sarah, thanks! This post is pretty clearly targeted at Linux kernel maintainers, but I think it is also just as relevant for random people doing peer code reviews, whether in open source or not.

  8. Any thoughts on good habits for relatively new reviewers?

    New reviewers tend to focus on #3 because that’s what are they best able to cover, especially if they hop around the kernel a bit working on small things here and there. In the past I’ve assumed that getting smaller things tidied up means the contributor is more likely to see #1 and #2 from the mainainter when the patch comes round again.

    I would hate to think I was crushing their spirits (or wasting their time) instead…

    • I think excessively focusing on coding style does discourage first-time patch submitters. It’s better to point them to the tool, and give them agency to learn what’s wrong. If they submit a second patch with coding style mistakes after checkpatch.pl has been pointed out to them, then I will go into detail about what’s wrong, and suggest they add a git hook to call it before every commit.

      For new patch reviewers, I would focus on having a good mix of coding style comments, as well as either questions about the patch (“Why did you do it that way?”) or comments about race conditions or other issues. I refer to the ratio of “useful” comments to coding style/grammar/nitpick comments as a signal to noise ratio. If you start going below 1:3 (1 useful comment for every 3 coding style comments), it’s very disheartening to a newcomer.

      It may also be useful to run the patched driver through more advanced tools, like sparse. If the patch added an extra gcc warning, or a new sparse warning, that’s a very valuable thing to point out.

    • Might help to at least be clear what you’re doing: “I can’t comment on the bigger issues yet, hopefully X will reply soon, while we’re waiting here’s a few style nits that I noticed…”.

      I still have trouble with this. Review is just painfully difficult sometimes, and the nitpicking is a way to trick myself into starting the hard work of reading a patch. So sometimes I start with the nitpicks and then keep the message in my “postponed” folder until I start to get the bigger picture, at which point I go back and write a preamble with the more important stuff. I should probably do that more often.

      The worst is when somebody’s tackling a really hard problem that I don’t know how to solve either. Hard not to keep up any enthusiasm on either side after a few rounds of “sorry, that’s wrong too, I don’t know what you should do instead”.

  9. Pingback: Sharp’s 3 Phases of a Code Review | found drama

  10. Well, there is an issue. From a certain perspective I shouldn’t worry how an idea was implemented if the idea itself is bad. Yet, that would mean that I take my own evaluation of the idea as definite. Thus I tend to review things which I think shouldn’t be done at all, as even if I prefer they weren’t done at all, I wanted them done correctly if they are done at all.

  11. Hi there, interesting post. A couple of notes from the trenches of another bigger open source project (LibreOffice). In part, because of the quality of the code we inherited, we started with a different mindset for patch review: In broad strokes, the guiding principle for LibreOffice patch review is simply: “Is it better than before?”. In practice, that usually means one _should_[1] only look for your 1/ and 2/ and either fix 3/ on the fly if its trivial[2] or ask to fix style issues in a follow-up.
    A further note: Your post implicitly assumes a “1/ newcomer 2/ trusted contributor 3/ maintainer” heirachy, presumably because that is the reality in the Linux Kernel world. It was pretty much the same in the old OpenOffice.org days, and has a chilling effect there too. At LibreOffice we _try_ to keep the separation between “2/ trusted contributor and 3/ maintainer” in flux as much as possible[3]. It has its costs, but the benefit outweight them by far: Reducing the bus-factor in the “maintainer” class results in changes beyond the obvious ones by rewiring the social dynamics.

    [1] This needs constant reemphasizing though. I should look at it again.
    [2] That sounds like a horrible thing to do, tweaking someone elses work. But in reality it mostly works out as contributors actually dont feel too strongly about style _unless_ one starts an explicit discussion/bikeshed about it. Within bounds, this is “Easier to beg for forgiveness than ask for permission” (Thanks, Grace Hopper).
    [3] Same for the separation between “1/ newcomer and 2/ trusted developer” btw.: It should be rather permeable.

  12. Pingback: La question des bonnes pratiques au sein d’une communauté | Framablog

  13. Pingback: What makes a good community? | The Geekess

  14. Hi Sarah.
    Thanks for the amazing post.
    I myself is an Open Source contributor. I am contributing to a specific project from past 4 months. And the project is active enough. Even it is one of the most *everyday/everyhour used* project for the organization.
    I am feeling that the pull requests of the project are not getting reviewed by the maintainer.
    I have been asking the maintainer to review the pull requests for more than a month.
    I am not sure what I should do now. I really love to work in that project :(. The maintainer must be busy enough. But it has been over one and half month.
    I really feeling discouraged now.

    • Would it help to resend the patches again? Can you find other community members to review them other than the maintainer? It may help to get the patches acked/signed-off-by other community members. Possibly the patchset is too large and makes it hard to review? Maybe you could break it down or send in sections? Also, it’s entirely possible that the maintainer is getting ready for conference season, and may be overloaded. It’s fine to ping them after two weeks of no review. Good luck with your patchset!

Leave a Reply

Your email address will not be published. Required fields are marked *