Open Stack

On Tue, 2013-08-20 at 11:26 +0100, Mark McLoughlin wrote:
> On Thu, 2013-08-15 at 14:12 +1200, Robert Collins 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.
> 
> The full study is here:
> 
> http://support.smartbear.com/resources/cc/book/code-review-cisco-case-study.pdf
> 
> This is an important subject and I'm glad folks are studying it, but I'm
> sceptical about whether the "Defect density vs LOC" is going to help us
> come up with better guidelines than we have already.
> 
> Obviously, a metric like LOC hides some serious subtleties. Not all
> changes are of equal complexity. We see massive refactoring patches
> (like s/assertEquals/assertEqual/) that are actually safer than gnarly,
> single-line, head-scratcher bug-fixes. The only way the report addresses
> that issue with the underlying data is by eliding >10k LOC patches.
> 
> The "one logical change per commit" is a more effective guideline than
> any LOC based guideline:
> 
> https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes
> 
> IMHO, the number of distinct logical changes in a patch has a more
> predictable impact on review effectiveness than the LOC metric.

Wow, I didn't notice Joe had started to enforce that here:

  https://review.openstack.org/41695

and the exact example I mentioned above :)

We should not enforce rules like this blindly.

Mark.