[Ns-developers] c0de review submission HOWTO

[Gustavo Carneiro]

2009/4/15 Mathieu Lacage <mathieu.lacage at sophia.inria.fr>

> hi all,
>
> After 2 iterations with mirko, I would like to point out a couple of
> important issues:
>
> 1) When there are multiple iterations over a review (patch submited,
> review done, new patch submitted, new review done), it might be easier
> to submit a patch _relative_ to the previous patch to help the reviewer
> assess what changes you really did. But, it might also be easier to
> generate a complete patch: it really depends on the context. Generally,
> if you produce a complete patch which is not relative to the previous
> review, you _need_ to produce also some explanation text to explain what
> you changed such that the reviewer can review these parts without having
> to look again at everything in the patch. If you produce a relative
> patch, it's ok to not include that explanation because the patch already
> contains that information.
>
> 2) It helps a lot to _split_ the changes in independent small simple
> patches: simple method renames can be done either as a separate
> preparation patch or as a followup patch. These decrease the size of
> each patch to review, and thus make it easier to merge these patches
> quickly in ns-3-dev.
>
> I know that this is a lot of pain for submitters but it's also a lot of
> pain for reviewers so, we really need to figure out a decent way to
> handle this process.


This is why I think this review tool is not enough.  For trivial patches, I
think it's not worth the trouble to use a review tool.  For extensive
changes, puiblishing a mercurial branch is probably a better solution,
because with a branch the reviewer already has access to both absolute and
incremental changes, taking care of your first point, and small separate
commits can take care of your second point.

This is not your fault, and I do not have any better solution.  Just
pointing out some conclusions I reached.  Probably the ideal tool would
allow online annotations, like rietveld, but would work on top of a
mercurial branch instead of a plain patch.  Unfortunately, probably such a
tool does not exist.  And so this email is probably not helping, I know...
:-)


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


-- 
Gustavo J. A. M. Carneiro
INESC Porto, Telecommunications and Multimedia Unit
"The universe is always one step beyond logic." -- Frank Herbert