This page is just a brain dump, for now...

This project is not active anymore. All information on this page is likely very outdated and invalid.

Getting Started

The goal for members of the GNOME PatchSquad is to find potential problems in submitted patches. This page aims to show you how to begin doing this. This Getting Started section covers some things to keep in mind, the necessary background knowledge you'll need, and how to get your system setup so that you can begin doing reviews. The next section, Reviewing Patches, goes over the steps of patch review -- from finding a patch, to ways of testing the patch, to things to check for when reading it.

Things to keep in mind

There are various rules you should keep in mind throughout the patch review process:

• You are not accepting or rejecting patches.

  • Your job is just to look for potential problems with the patches, and point them out if you find any.

• You are working to try to decrease the workload of the maintainers.

  • This means, among other things, that you should be brief in your reviews.

• You may be unable to find any problems with a particular patch.

  • Even after thorough checking and testing, you may not find any problems with a given patch. That's fine, it just means it is more likely to be ready to be included. Just add a 'the patch works for me in my testing' comment to the bug.

• Maintainers rule.

  • This page tries to show rules common to all projects, but each project will almost certainly have rules of its own as well (though likely undocumented, unfortunately). In fact, a project may have rules in conflict with information you find here, in which case the project's rules take precedence.

Necessary preliminary steps

FIXME: We need to write or link to the relevant pages for these

• Learn how diff and patch work (and cvs diff)
• Build a development version of Gnome
• Join relevant mailing lists

Reviewing Patches

(Maybe I should mention something about expected time requirements? This isn't anything like the Bugsquad; it will require far more time to review patches than to triage bugs)

Step by Step

1. Get a bugzilla account

   • Just go to: https://bugzilla.gnome.org/createaccount.cgi

2. Find a patch to review

   • You can use the following pages to find them:

     • https://bugzilla.gnome.org/browse.cgi

     • https://bugzilla.gnome.org/reports/patch-diligence-report.cgi

3. Read it thoroughly

   • Read up on all the functions used in the patch to understand what they do (devhelp or google for developer.gnome.org documentation).

4. Read the surrounding code

   • Do the same, check for problems such as those in the list(s) below.

5. Test the patch

   • Maybe run it under some special tools (lint, valgrind, others?).

     You can test patches quickly and easily with conary and the Gnome Developer Kit as documented on the Building Packages page.

6. Add yourself to the CC list

   • It's important to keep track of further changes (comments, new patches, status) so that you can react accordingly.

7. Report anything wrong with the patch

   • Set the preliminary-review patch status

8. Watch the module in your bugzilla userprefs

FIXME: What else is missing?

Examples of Problems to check for

Finding problems can cover a huge range of checks, depending on the reviewers expertise, covering things like

• Does the patch still apply?

  • Since code can sometimes morph quickly as other changes are committed, it is often the case that remaining patches do not apply anymore -- the 'patch' command will warn that hunks (portions of the patch) fail to apply. Many of the times when patches do fail, it is relatively easy to fix up (the patch command is lacking a decent artificial intelligence algorithm and thus fails when even the most minor of things change, such as the number of spaces). This is one of the few cases where you may want to consider updating the patch yourself, which means that after the patch fails you manually edit the source code to account for the failed hunks and then generate a new patch.

• Was the patch created with both the -u and -p flags?

  • Personally, I'll ignore or reject patches that aren't created with these two flags. The patch might be perfectly fine and fix an important problem, but it's too hard to review. Besides, it's easy to make the patch submitter just generate a new patch.

    • Adding a [patch generated without -up flags] stock comment in bugzilla would help --ClaudioSaavedra

• Does the patch come with a meaningful explanation of the change?

  • For the maintainer to review the patch it usually helps when there's a brief summary of the change written up front. And when the explanation is written in ChangeLog style the maintainer can then simply copypaste it rather than write up the description himself.

• In your testing, does the patch work?

  • Try to test thoroughly and uncover bugs. This is a step that doesn't take any programming experience (though reading the code may suggest extra things to test) and yet it's often the step that uncovers the most problems. This is usually a worthwhile step regardless of who else has already tested the patch, because you may do things slightly differently and discover something new.

• Does it match the coding style of the project?

  • New contributors often get part of the coding style right but miss other parts of it without realizing it. This is at least partially because we have different coding styles throughout the Gnome project, though it'd probably still be a problem even if that weren't true. You can usually figure out the style by just searching for several examples of similar code elsewhere. Anyway, different modules have different requirements about how strictly patches much conform, but, for example, enlightened modules require all tabs be nuked from patches before they can be accepted.

• Does it violate any guidelines documented by the project?

  • Many projects will have a HACKING file in CVS or other documentation pointing out common pitfalls in programming specific to their project or other specialized guidelines on patches. Contributors are often unaware of these.

• Are there any memory leaks caused by the patch?

  • Memory leaks are a fairly common error, at least for modules written in C. Check for these, especially for functions that might allocate data that the caller is expected to free. valgrind may be helpful here as well (you'd need to run it on the module both before applying the patch and after applying to to determine which memory issues are new from the patch; the old issues you may want to consider filing as a bug report if one isn't already)

• Are there off-by-one errors, refcount mistakes or other similar common errors?

  • FIXME: We should get a page describing common errors like these. It'd be especially nice to have e.g. common errors in gtk+ programs, common errors in pygtk programs and other such guides available.

• Does it cause potential race conditions?

  • This is probably something more likely to be checked by a maintainer, but if you know how to catch these, all the better.

• If you have followed the project for a while, does the patch cause any other problems you are aware of that might be specific to that project or how the code is structured?

  • The best way to improve your patch review skills for a project is to follow it for a while and read the patch reviews done by the maintainer (including their follow-ups to your preliminary reviews in addition to their reviews of other patches). As you do so, you'll learn extra details about the structure of the particular project, invariants that the code tries to maintain, the big ideas of how the code works, and be better able to provide more thorough reviews. You'll also learn of common mistakes made by new patch submitters and may want to offer to document these for the maintainers.