Code Review Guidelines

From OpenFlow Wiki

All code distributed with post-v0.9 OpenFlow reference implementations is expected to complete at least one review. This page includes guidelines for doing a code review, plus pointers to git tools to clean up commit sequences before review.

Code review is intended to find bugs early and maintain consistency. As a side benefit, reviews are often a way to learn how to code better.

As a review submitter, please try to make it as easy as possible for the reviewer to understand (1) what you changed and (2) why you changed it, if not obvious. This generally means submitting a patch series where changes are at the right granularity, and requesting review before your commit series becomes a monster.

Commit Guidelines

[1] Read the first section, "Commit Guidelines", roughly 2 screens worth of text.

Notes from Open vSwitch SubmittingPatches file

Thanks to Ben Pfaff for noting these.

Before you send patches at all, make sure that each patch makes sense. In particular:

• A given patch should not break anything, even if later patches fix the problems that it causes. The source tree should still build and work after each patch is applied. (This enables "git bisect" to work best.)

• A patch should make one logical change. Don't make multiple, logically unconnected changes to disparate subsystems in a single patch.

• A patch that adds or removes user-visible features should also update the appropriate user documentation or manpages.

Testing is also important:

• A patch that adds or deletes files should be tested with "make distcheck" before submission.

• A patch that modifies Linux kernel code should be at least build-tested on various Linux kernel versions before submission. I suggest versions 2.6.18, 2.6.27, and whatever the current latest release version is at the time.

• A patch that modifies the ofproto or vswitchd code should be tested in at least simple cases before submission.

...

The description should include:

• The rationale for the change.

• Design description and rationale (but this might be better added as code comments).

• Testing that you performed (or testing that should be done but you could not for whatever reason).

There is no need to describe what the patch actually changed, if the reader can see it for himself.

If the patch refers to a commit already in the Open vSwitch repository, please include both the commit number and the subject of the patch, e.g. 'commit 632d136c "vswitch: Remove restriction on datapath names."'.

Amending Git History

[2] With a little practice, you can edit, reorder, split, and merge commits in git. This is a nice description on how.

And here's another description of how: [3]