Diaries of a Core Maintainer #5: The 6-pass patch review

I would love to see a series of blog posts from Drupal developers on patch review strategies they employ, so we can share some tips and tricks and ramp up our collective review IQs. I'll start it off with mine. I call it "The 6-Pass Patch Review" (wittier names welcome! ;))

To begin, crack open your text editor. You'll use it to jot down notes and questions as you go through each of these passes. Other than that, no fancy tools are required to do most of this other than a keen set of eyes, an inquisitive mind, and the ability to empathize with people new to Drupal and put yourself in their shoes. (Of course, if you are relatively new to Drupal, then patch review can actually come easier for you than other people. :) Start today!)

Pass 1: The "Coder module" pass

In this pass, you basically just become a human version of Coder module. Quickly scan through of the code to see if you can spot anything amiss coding standards-wise. The more patches you read, the more obviously things stick out to you that deviate from the rest of the code. For instance, after 3+ years of doing this every day, my brain is now physically wired so that if I see something outdented two spaces more than it ought to, or a missing period on an inline comment, I break out into a cold sweat. ;)

Probably at some point most of this pass will be completely automated from testing.drupal.org. But for now, it's a really great way to scan over the length of the patch and quickly get the sense of what's there without busting your brain too badly.

Pass 2: The "Bottom up" pass

In this pass, you literally start reading the patch hunks from the bottom to the top and see if they make sense to you. Try to resist the urge to read any comments (that's for the next step).

This is a great way to catch things like some_function(NULL, TRUE, array(), $booger, 'aardvark'). Without benefit of having seen the PHPDoc for some_function() ahead of time, if the calling code makes no sense, we have some work to do. This work could involve renaming some_function() to something that makes it more obvious what it's for, or it could involve refactoring some_function() into several functions that each take parameters specific to their functionality, or it could just involve re-formatting the way options are passed in. At the very least, it means we need some inline comments to explain what it does. Which leads us to...

Pass 3: The "Comments-only" pass

Now that you've read the code from bottom to top, you should hopefully be starting to glean some idea of what this patch is actually doing. Now, your task is to make sure the comments reflect your understanding, and/or clarify places where you are still head-scratching.

Beyond obvious things like checking for grammar and punctuation, your goal should be to see if you only read the comments and not any of the code, can you still grasp what the patch is intended to do? (to the best of your current understanding)

Pass 4: The "Actually read the issue" pass

Yes, that's right. This is pass 4. Why is this not pass 0, you ask? Because future Drupal users won't have the luxury of knowing all the various background discussions that led up to a decision, without breaking into some CVS annotate action. Having this background information before you go into a patch review gives you an unfair advantage. Remember that you can never un-read an issue, so make sure that you take copious notes of all the things you observed before doing this step. Once you've read it, your future reviews become poisoned by "insider" knowledge, and you can no longer share the same empathy for new developers.

During this pass, your goal is to figure out if all of the various twists and turns the discussion made are reflected in the code and its comments. Was there a tweaky bit of code that was explained well in one of the issue follow-ups? That explanation should be an inline comment above the code. Was there a great use case someone thought up for why this functionality would be useful? That should be reflected in the automated tests that come with the patch.

Pass 5: The "Does it actually work?" pass

Now, and only now, apply the patch, and verify that it does what it says it's supposed to do. Also try and think of other, sneaky things that might be related to this functionality change, or different ways that the change here might ripple out to other things in the system. For example, if the patch changes something about the user registration process, you might also think to test the login process, since they are related. And, if you have true Testing-Fu, you might also think to test OpenID, since it touches both, and bugs within it are not likely to be encountered in most peoples' day-to-day use of Drupal.

At this stage, you should also run all the related tests and make sure that they pass. While doing so, check for holes in the testing suite that either this patch should include tests for, or that should be filed as separate issues. The tweakier the change, or the more things it touches, the more we definitely need tests to confirm that it stays working.

Pass 6: The "Put it all together" pass

Now, with all that knowledge in your head, go back through the patch one more time, this time in the proper order, and jot down any other notes that come up. By this time your text file is probably 60MB and you have plenty of follow-up questions and discovered flaws to mark that sucker code needs work. ;)

So that is mine. Interested to hear what other people do, too!