[Ns-developers] c0de review submission HOWTO

[Mathieu Lacage]

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 ?]

</recommendations>

With the above set of recommendations in mind, I have spent a good
amount of time to try to see how other projects do this but I could not
find a magic solution: the process of generating good patches, and
having people produce good reviews for them seems to be uniformly
painful. 

However, there are a couple of tools which have been developed by others
and which could be of use for some people: quilt is the oldest example
of a patch management system but mercurial has a similar functionality
builtin as 'mercurial queues', also known as 'mq'.

A couple of useful links:
http://hgbook.red-bean.com/read/managing-change-with-mercurial-queues.html
http://hgbook.red-bean.com/read/advanced-uses-of-mercurial-queues.html
http://hgbook.red-bean.com/read/adding-functionality-with-extensions.html
http://wiki.sagemath.org/combinat/Mercurial

'mq' looks cool but it also looks fairly complex to use so, I am not
going to recommend it very much for now.

Anyway, if you have experience dealing with similar problems in other
projects or using 'mq' or similar tools, it would be nice to hear about
it.


Mathieu


On Tue, 2009-04-14 at 09:03 +0200, Mathieu Lacage wrote:
> (strange title to go through the ns-dev filters)
> 
> hi,
> 
> Since I did not receive screaming emails saying that it was a horribly
> bad idea, I created http://groups.google.com/group/ns-3-reviews/, and I
> would like to suggest everyone who wants to submit a review to:
> 
> 0) download http://codereview.appspot.com/static/upload.py
> 1) record the changes you want to request a review for in a mercurial
> repository: "hg commit ..."
> 2) within the mercurial repository, run upload.py to submit a review
> with http://codereview.appspot.com/. Make sure you specify
> ns-3-reviews at googlegroups.com as a CC.
> 3) Paste your review request on
> http://www.nsnam.org/wiki/index.php/Reviews or send me an email and I
> will add it there
> 4) announce your review request on ns-developers
> 
> thank you,
> Mathieu
>