[Ns-developers] c0de review submission HOWTO

[Tom Henderson]

Mathieu Lacage wrote:
> hi all,
> 
> After the first round of reviews using rietveld, it became clear that,
> while the tool is a nice graphical frontend to the process on commenting
> on a patch, it does not really improve the current code review situation
> and most reviewees seem to dislike it quite a bit. So, here is a summary
> of what problems we face.
> 
> It's pretty clear that the big issue with code reviews as of today is
> that of preparing patches for review. We have no clear guidelines on
> what is a good patch for review so, I think that a first step would be
> to try to come up with such guidelines. Here is an attempt: please,
> comment, edit, scream, etc.
> 
> <recommendations>
> 1) No whitespace changes, ever, unless the patch is _only_ about
> whitespace changes, but even then, don't do it if you can avoid it.
> 
> 2) Each patch should be as small as possible, and deal with _only_ one
> issue. i.e., if the commit message says: "X and Y", it's a big red
> warning sign that you should split the commit in two separate commits.
> 
> 3) Each patch should contain a coherent change: if you need to edit 10
> files to get a feature to work, then, the patch should contain changes
> for 10 files. Of course, if you can split the feature is sub-features,
> then, you should do it to decrease the size of the patch as per 2)
> above.
> 
> 4) Ideally, after each patch is applied, the tree ns-3-dev should still
> build and all tests should still pass (this helps bisection
> considerably).
> 
> 5) Big features will require multiple patches for implementation.
> Ideally, these would be managed as a set of dependent patches: the first
> patches would touch existing code to prepare the ground for the big
> feature while the latter patches would add the new code for the big
> feature. When this is the case, make sure you point out the rationale
> for the initial patches to outline why they are needed later.
> 
> 6) Read the patch before sending it for review. 
> 
> 7) Submit patches using one of the following tools:
>   - mercurial patchbomb extension to ns-3-reviews at googlegroups.com:
> http://hgbook.red-bean.com/read/adding-functionality-with-extensions.html#sec:hgext:patchbomb it's fine to send either a pure ascii fully-inline patchset (one patch per email) or a complete bundle.
>   - publish a repository of your changes on a website using hgweb: one
> commit per patch
> 
> 8) Always include a description of what the patch is doing, and why it
> is doing it
> 
> [more recommendations ?]

We should ask people to conform to the coding style; this has been a 
problem in the past.

We could suggest people either prepare a single patch or prepare a repo 
with a set of patches.

A common convention for preparing patches (e.g. arguments to diff to 
match mercurial diffs) would be nice.

I think it is important to ask people to contribute these things, on an 
as appropriate basis:
   - example script(s) that exercises the new code
   - regression script(s) to check the new code
   - patch to CHANGES.html or RELEASE_NOTES
   - patch or new chapters for documentation (manual or tutorial)
   - Doxygen

Now, these may not be necessary to do an initial review, but we risk not 
getting these pieces of the system updated if there isn't some phase of 
the review that checks for these items.  I would like to avoid having 
undocumented modules in the system.  Of course, we would need a HOWTO 
for contributors to generate the above, as well.

Tom