Bugzilla:Simple Patches

The Short Version

Basically, the bottom line is that small, simple patches will get through the review process much faster.

If you attach a huge patch, you could literally be going through the review process for a year or more before your patch is accepted.

All patches should address only one thing. If you are implementing a huge feature, try to figure out how to split it up into many small patches that build on each other, and file separate bugs for each one of your small patches.

A Longer Explanation

It is a good idea to split your changes up into multiple simple patches. Simple patches are easier to review, because:

• there is more locality between similar changes, meaning less scrolling and hunting for code
• the whole patch can be dedicated to just one issue, making reviewing a simpler mental task by reducing the number of mental context switches and improving the likelihood of finding problems with the patches
• they can be reviewed and checked in separately, meaning only one part has to be remade if it gets a negative review, and meaning less code will get stale waiting around for a subsequent review
• simple patches can be easily reviewed in small blocks of time, if that's all the reviewer has at the time

Not splitting patches into separate simple patches is very tempting for all developers. They have completed our work, and they attach it. It is the easiest thing to do.

But it is not the easiest thing for the reviewer. And Bugzilla has a much bigger problem getting enough review than it does development. So moving work from the reviewer to the developer is probably a good idea. And in the end, getting the code in ASAP is what's usually important for the developer.

It is very easy as a developer to neglect the fact that reviewers come in with no context, and orienting yourself to someone else's code can be difficult. What seems like a simple patch to the code's developer, can be difficult to someone trying to do a thorough review.

Generally, simpler equates with smaller, but not always. A small patch that does ten different things is likely a lot more complex than a big one that does the same thing in 100 different places.

Not everything can be split up into simple patches. The first guideline of simple patches to follow is that no patch should break things. So often, this means patches cannot be simple.

The second guideline is that multiple independent changes should go in separate patches. Patches should usually be split up based on what you're doing rather than in what files you're doing it in.

It is not always possible to find multiple independent changes to make into independent patches. Often we must have a set of patches, where certain patches must be applied before others, and we should not neglect this possibility - it is still easier to review than one complex patch.

When you are creating separate patches, make each one a separate attachment on the bug report.

Ideas For How To Split Your Patch

Let's say that your patch modifies 100 different templates in Bugzilla:

1. Attach a patch that implements the most basic part of your enhancement. For example, if you were modifying every template to display a big penguin next to email addresses, first you'd attach a patch that had:

• The code that generates the big yellow penguin.
• Modification of one template to include the penguin next to the username fields, preferably a small template.

2. Attach an individual patch for each area of Bugzilla that's being modified. For example, you could have one patch for the administrative areas, one patch for bug editing and creation, and one patch for other places in Bugzilla.

Or, as another example, let's say that you wanted to make every .pm module in Bugzilla use a particular CPAN module:

1. Attach a patch that modifies one module, preferably a simple module that's not used in all of Bugzilla, like Bugzilla/Keyword.pm. This patch would also include the code for checksetup.pl to say that the module is required.

2. If it's going to take a lot of modification of each module, then file bugs and attach separate patches for each module that you want to modify after that.

What you'll notice that all of these examples have in common is:

• Every patch is functional--that is, there are no patches that do not actually make some change that affects a user. There's no patch that just adds a library and doesn't use it. (Sometimes we actually do add a library in one bug and then use it in another, but if that's the case, then we don't check in the library until the code that uses it is also ready for checkin.)
• The small pieces are focused--they work on one area of Bugzilla, or the smallest possible part that can be modified.