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.

A couple weeks ago I upgraded my laptop to Fedora 9. This went mostly smoothly.

I used the new preupgrade tool. This downloads data needed for the upgrade while your system remains functional. Then, after the download is complete, you can reboot into anaconda for an upgrade. I gather the benefit of this is that upgrade-via-anaconda is actually supported, unlike yum upgrade. Also, it means you don’t have to burn a CD — nice.

There was some minor glitch during this process — but minor enough that I forgot what it was when the time came to report it. Whoops.

After upgrading, some programs would not start. For instance, gnome-terminal did, but Emacs didn’t. After a while I found bug #430416 — a bug causing an X server error when certain fonts are requested. Removing xfs is the quick workaround. This problem was a pain.

Next, I found that the cursor in gnome-terminal was blinking again. Argh! I dislike blinking cursors. I find them very distracting.

I had a surprisingly hard time figuring out how to disable this. It used to be available on the gnome-terminal options page, but this was removed. For some reason I did not think to look on the Gnome-wide “keyboard” properties page — what does this have to do with the keyboard? So, in the end, I resorted to searching and ran some obscure gconftool-2 command line. Yay. In the meantime I found this nice page on disabling cursor blinks.

I am disappointed with the Gnome upgrade experience. Almost every time I have upgraded my OS, I’ve had to spend a bit of time fixing my desktop to reassert customizations I’ve already made. This is no good! My changes matter a lot to me!

In this particular case, gnome-terminal could have noticed my earlier setting and offered to upgrade it. Or, Gnome could adopt a friendlier policy — simply accept that some mistakes were made in the past, and choose to value my experience over consistency. After all, we’re talking about the terminal here, a tool unlikely to be used by the inexperienced audience that is the apparent target of this sort of change.

I have become generally skeptical of arguments based on consistency of UI. People seem to have little trouble navigating the web, where no two sites are the same. Also, every desktop release seems to come with a new default theme and plenty of experience-breaking changes — that is, there is no consistency over time.

Someday we’ll write a Buddhist AI who will know that its self-awareness is an illusion.