[Ns-developers] c0de review submission HOWTO

[Mathieu Lacage]

Based on your comments, here is an extended version which still lacks a
section on testing (my hope is that we can fill that section quickly).


Mathieu

On Fri, 2009-04-24 at 13:58 -0700, Tom Henderson wrote:
> 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
> 
-------------- next part --------------

                    Submitting code to ns-3
                    -----------------------


0) Introduction
---------------

So, you have downloaded the latest version of ns-3 (or an older version),
you have started to play with it, found a bug or two, fixed them, added
a new feature. Now, because you don't want to have to port your work
to newer versions of ns-3, you would like to integrate your work in ns-3
directly. This is awesome !!

Before you start trying to sumit your code/changes for merging in ns-3, 
you should keep in mind that there is already a large burden on the ns-3 
maintainers to manage the flow of incoming contributions and maintain 
new and existing code (the backlog of unmerged code is already fairly 
big). The goal of this document is thus to outline how you can help to 
minimize this burden and thus minimize the average-time-to-merge of 
your code. Making sure that each code submission fulfils as many items
as possible in the following checklist is the best way to ensure quick
merging of your code. 

Also, keep in mind that the contribution process is not a memory-free 
stachastic process: if you keep doing good submissions, reviewers will 
remember about it and will eventually make it easier for you to contribute 
code. Conversely, if you keep doing bad submissions and keep ignoring some 
of the steps decribed below, reviewers will become increasingly annoyed 
at you, sometimes reluctant to merge perfectly valid changes, sometimes 
even grumpy. Reviewers are just human beings after all.

1) Testing
----------

Every new code should include automated tests. 

XXX: add content here.

2) Example code, documentation
------------------------------

If you add a new features, or make changes to existing features, you
need to update existing or write new documentation and example code.
Consider the following checklist:
  - incompatible API changes must be documented in CHANGES.html
  - new features should be described in RELEASE_NOTES
  - new features should include a new example program in the examples/
    directory
  - new API or changes to existing API must update the inline doxygen
    documentation in header files
  - consider updating or adding a new section to the tutorial in doc/tutorial/
  - consider updating or adding a new section to the manual in doc/manual/

3) Coding style
---------------

Whatever code you submit, make sure that you follow the ns-3 coding 
style. It is described in http://www.nsnam.org/codingstyle.html.

Some of the rules it describes might seem arbitrary to you and you
might disagree with these rules because the resulting code layout
looks ugly to you. If you are one of these person, we ask you to 
consider carefully the purpose of a coding style: the purpose is
to ensure that the overal codebase is consistently indented and 
layed out to make it easier for a trained eye to deal with large
amounts of code. The sole purpose of a coding style is not to generate
a beautifully-layed codebase: it is to ensure consistency.

So, whether you disagree with our layout rules, or you are unsure
about whether a specific piece of code matches the rules, focus
on making sure that the coding style of your code matches the coding 
style of any surrounding ns-3 code.

Code submissions which ignore the coding style will just annoy your
reviewers and it is not a good idea to annoy your reviewers if you
really care about minimizing your time-to-merge delay.

4) Whitespace changes
---------------------

Do not ever submit code which contains whitespace/tab changes mixed
with other non-whitespace changes. Generally, it's a good idea to avoid
submitting whitespace changes at all, but, if you really must, submit
a single change consisting only of whitespace changes.

5) Description
--------------

For each code submission, include a description of what your code is
doing, and why. Ideally, you should be able to provide a summary
description in a 5-line paragraph with a 1-line (15 word) subject. If
you fix a bug filed in our bugzilla database (http://www.nsnam.org/bugzilla), 
the subject should include first the bug number, and then, the bug title.
For example: "bug 558: qos-tag.h is gone missing from wscript"

Bonus points go to submitters who provide a description of their 
testing strategy.

6) Minimize the size of each submission
---------------------------------------

Each submission should be as small as possible. Ideally, each submission
should deal with one issue only. If your description of your submission 
includes words such as "and", it is a big red warning sign that you
should split your submission in two separate smaller submissions.

7) Coherent changes
-------------------

Each submission should be a coherent whole: if you need to edit ten
files to get a feature to work, then, the submission should contain all
the changes for these ten files. Of course, if you can split the feature 
in sub-features, then, you should do it to decrease the size of the 
submission as per the previous item.

For example, if you have made changes to optimized a module and to fix
a bug in another module, make sure you separate these two sets of changes
in two separate submissions.

8) Multi-part submissions
-------------------------

If you are working on a large new feature or a large refactoring, and 
because you will attempt to minimize the size of your submissions, you
will have to split your large work in multiple separate submissions.

Ideally, these submissions will be started by a detailed explanation
of the overall plan such that code reviewers can review each submission
separately but within a large context. This kind of work typically is
split in multiple dependent steps where each step depends on the previous
one. If this is the case, make it very clear in your initial explanation.
If you can, minimize dependencies between each step such that reviewers
can merge each step separately without having to consider the impact of
merging one submission on other submissions.

9) Before sending a submission
------------------------------

When you send a submission for review and merging in ns-3, before
you hit the 'Send' button of your email client, ask yourself one 
last time: "If I were a reviewer, and I had to review that submission,
what would I do ?". Specifically, make sure that you have provided 
enough context to allow someone else to understand what you did and why.
Keep in mind that your reviewer does not have access to a readable
dump of your brain: he has access only to your code, and your emails.


10) Submission tools
-------------------

There are many ways to submit a piece of code. Some of them are not
appropriate:
  - do not send us a modified version of each file you have changed
  - do not send us a .zip or .tar.gz file by email with a copy of
every modified file
  - do not send us a patch against an unspecified version of ns-3

Others are more appropriate:
  - a simple patch
  - a binary bundle
  - a hosted clone
  - patch emails

  i. A simple patch
  -----------------

Patches can be attached to a bug report or sent to our developer mailing-lists.

The UNIX "diff" tool is the most common way of producing a patch: a 
patch is a text-based representation of the difference between
two text files or two directories with text files in them. If you have
two files, "original.cc", and, "modified.cc", you can generate a patch
with the command "diff -u original.cc modified.cc". If you wish to
produce a patch between two directories, use the command 
"diff -uprN original modified".

Make sure you specify to the reviewer where the original files came 
from and make sure that the resulting patch file does not contain 
unexpected content by performing a final inspection of the patch 
file yourself.

Patches such as this are sufficient for simple bug fixes or very simple
small features.

  ii. Binary bundles
  ------------------

ns-3 uses "mercurial" as its version control tool: a quick introduction
to using it can be found on our homepage: http://www.nsnam.org/mercurial.html.
Further extensive documentation and manuals are available from the mercurial
homepage: http://www.selenic.com/mercurial/

If you get used to use mercurial to record your daily/hourly work using
mercurial in a local clone of the ns-3 main repository, and if you want to 
share that work with other people to get initial feedback about your work,
your implementation approach, and start a high-level discussion about it,
you can easily package all your local work history as a single binary bundle
file with the "hg bundle" command and send this file over email. 

  iii. Hosted clones
  ------------------

If your bundles grow large in size, it can become tricky to send them over
email: hosting a clone of your mercurial repository is a simple way to work
around this. If you have a host with a static ip address and/or a hostname,
you can run "hg serve" within your repository in this host and send instead
to your co-workers a url to that repository: if I run "hg serve -d -p 8080" 
on my local ns-3-dev clone, I could send the url "http://diese.inria.fr:8080/"
to my co-workers to allow them to pull all my development history from there.

If you don't have a static ip address or hostname, or are located behind
a NAT or a firewall, you have to use a third-party hosting provider for your
code. A small list of hosting providers is available there:
http://www.selenic.com/mercurial/wiki/index.cgi/MercurialHosting

We can also provide hosting to ns-3 developers on http://code.nsnam.org/
should none of the above alternative work for you.

 iv. Patchbombs
 --------------

Finally, patchbombs are a nice way to perform a request for a detailed
review. Make sure you enable the patchbomb extension in your ~/.hgrc:
[extensions]
hgext.patchbomb = 
Make sure you configure your patchbomb extension correctly: 
"man 5 hgrc" and look in the [email] and [smtp] sections

Finally:
hg email -i -t ns-developers at isi.edu http://code.nsnam.org/ns-3-dev

Beware that generating too many emails with this tool without doing
proper "history rewriting" to produce clean easy-to-review patches will
be considered rude.

11) History rewriting
---------------------

The idea behind history rewriting is to request a code submitter to 
re-arrange their repository history prior to merging in the main
ns-3 repository. 

For example, let's say that during development of a new feature, you
decide to use your mercurial repository as a fancy version-enabled
backup system: you do a lot of work, and regularly commit it to save it.
When you are done implementing and testing that new feature, the resulting
repository history as shown with "hg log" will look very verbose, and
will most likely contain a lot of commit messages such as "fix bug".
Clearly, none of these commits are very helpful and there is little point
in keeping them around: they make it painful to use the annotate, and bisect
commands, and will make review of the final mercurial tree harder than it
need to be.

A simple way to work around these problems is to ask each contributor to
re-structure their commit history before submitting their tree for review:
  - pointless backup commits are deleted from the history
  - commits are re-organized in sub-feature commits
  - each commit is made buildable
  - etc.

  i. Rewrite a new history from scratch
  -------------------------------------

Once you are done with a new feature, you can generate one final patch
with the mercurial "diff" command and apply it to a clean repository
as one single commit with a nice new commit message.

A variant on the above is to split the final commit in multiple smaller
commits, each of which addresses one aspect of the final feature and
make sure that each commit is still buildable.

  ii. Build clean history from the start
  --------------------------------------

Instead of using mercurial as a powerful backup system and trying to go back
later, some users prefer to use tools such as 'mq' or 'pbranch' to split
their work from the start in a set of separate entities and still record 
somewhere their day-to-day development history.

More information on these tools:
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://arrenbrecht.ch/mercurial/pbranch/