[openstack-dev] Code review study

Daniel P. Berrange berrange at redhat.com
Thu Aug 15 12:24:50 UTC 2013

On Thu, Aug 15, 2013 at 09:42:09PM +0930, Christopher Yeoh wrote:
> On Thu, Aug 15, 2013 at 11:42 AM, Robert Collins
> <robertc at robertcollins.net>wrote:
> > This may interest data-driven types here.
> >
> >
> > https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/
> >
> > Note specifically the citation of 200-400 lines as the knee of the review
> > effectiveness curve: that's lower than I thought - I thought 200 was
> > clearly fine - but no.
> >
> >
> Very interesting article. One other point which I think is pretty relevant
> is point 4 about getting authors to annotate the code better (and for those
> who haven't read it, they don't mean comments in the code but separately)
> because it results in the authors picking up more bugs before they even
> submit the code.
> So I wonder if its worth asking people to write more detailed commit logs
> which include some reasoning about why some of the more complex changes
> were done in a certain way and not just what is implemented or fixed. As it
> is many of the commit messages are often very succinct so I think it would
> help on the review efficiency side too.

We in fact already ask people to do that 


Commit message quality has improved somewhat since I first wrote & published
that page, but there's definitely still scope to improve things further. What
it really needs is for more reviewers to push back against badly written
commit messages, to nudge authors into the habit of being more verbose in
their commits.

That said, commit messages are fine for describing what a specific commit
does, but for many non-trivial features spanning multiple commits, you need
an external reference document. In projects based around a git send-mail
mailing list review process, there would be cover letter describing the
broad design / goals of the work. In openstack's dev model, this role is
really served by the blueprints.

Unfortunately we are often badly lacking quality control over blueprints.
Far too many blueprints have just a handful of lines of text in the blueprint
description and no link to any wiki page with design info or background on
why certain decisions where made. At some point I think it would be pretty
beneficial to start getting stricter around quality of blueprints info, so
that the creation of the blueprint isn't just a "tick box" for process
compliance, but an actual useful source of information.

|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

More information about the OpenStack-dev mailing list