Due to my recent hacking on gdb, I’ve been thinking recently about patch review. Mostly, I seem to be interested the social aspects of review; though there’s also a lot to discuss concerning the mechanics, and that would probably be a useful area for a tutorial of some kind.

Here are a few guidelines I came up with, based on what I think I do when I review a patch:

• Don’t hold a patch to a higher standard than the code it modifies. I think it is pretty tempting to try to get patch submitters to do cleanups that you, the maintainer, have been putting off. My view is that it is more important to encourage part-time patch contributors than it is to try to achieve perfection in the code. Yes, the existing code may have some problem that the new patch perpetuates. But, one more instance of that is not going to severely disable a cleanup effort.
• Keep your standards up. While you can’t expect a patch to solve unrelated existing problems, it is also important not to let your standards drop. This is easy to do — for example, a big, useful feature might be written by someone who is unwilling to write documentation. One thing to remember is that every time you give in to something like this, it is harder to resist the next time. And, it gets harder to enforce the rule for the rest of the community. One solution is to try to work with the submitter to find someone willing to help him. With new contributors it is sometimes handy to temporarily break this rule — fix up their first patch, send it back so they can see it, and then tell them that this is how it ought to be done in the future. This can avoid the slippery slope problem, as long as you are clear that it is a one-time-only event.
• Review the patch, not the context. This is related to the first point. Sometimes the context of a diff will have an obvious problem; it can be tempting to ask the patch submitter to fix this as well. In the worst cases, this will snowball into a generic cleanup. Resist this temptation! Instead, fix the context yourself; or at worst ask if the submitter would be willing to submit a separate cleanup patch. I think that, ordinarily, this temptation arises when patch-writing barriers are too high — rather than try to push things off on volunteers, look at lowering these barriers so you don’t shy away from quickly cleaning up small messes.
• Keep independent disputes independent. When I am triggered by someone, it is easy to want to hold a grudge and to review their other patches more harshly. This is a huge mistake. A dispute about one change need imply nothing about another; mixing them together will just cause confusion and distrust. This does not mean you ought to ignore bad behavior — it is very important to set boundaries and raise communication standards as early as possible. Instead, you can reply to a “bad” note twice, once to start a sub-thread discussing the content of the patch, and a second response to address the tone of the message.
• Enforce your standards on yourself. Or, if you can do it, have higher standards for yourself than for others. Either way, this is hard to do — I find it easy to cut myself some slack, knowing I will be around to clean things up later. Likewise it is easy for me to go with the first fix I think of, or to give a new function the first name that comes to mind. Similarly, hold your logic and standards up to the same scrutiny. It is community-destroying when a maintainer says one thing, but then does another himself.
• Ignore patches you don’t care about. A large project will get many patches. Think about what areas you are interested in, or know about, or want to influence; and then ignore the rest. I sometimes see attacks on patches from otherwise disinterested parties. I’m not sure what is going on here — maybe just the occasionally irresistible urge to appear smart — but it is counterproductive. If someone’s patch fixes their problem, and doesn’t affect you, get out of the way.
• Be generous. Remember what your goals are when reviewing. Mine are to get quality patches into the tree, and to gain new contributors. So, I try to be polite when responding and I try to give only constructive criticism. If I make pedantic comments on a patch, I usually explain that they are just nits to fix; but more importantly, I just try to avoid being overly pedantic.