<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 20, 2013 at 2:21 PM, Clint Byrum <span dir="ltr"><<a href="mailto:clint@fewbar.com" target="_blank">clint@fewbar.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Excerpts from Mark McLoughlin's message of 2013-08-20 03:26:01 -0700:<br>
<div class="im">> On Thu, 2013-08-15 at 14:12 +1200, Robert Collins wrote:<br>
> > This may interest data-driven types here.<br>
> ><br>
> > <a href="https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/" target="_blank">https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/</a><br>
> ><br>
> > Note specifically the citation of 200-400 lines as the knee of the review<br>
> > effectiveness curve: that's lower than I thought - I thought 200 was<br>
> > clearly fine - but no.<br>
><br>
> The full study is here:<br>
><br>
> <a href="http://support.smartbear.com/resources/cc/book/code-review-cisco-case-study.pdf" target="_blank">http://support.smartbear.com/resources/cc/book/code-review-cisco-case-study.pdf</a><br>
><br>
> This is an important subject and I'm glad folks are studying it, but I'm<br>
> sceptical about whether the "Defect density vs LOC" is going to help us<br>
> come up with better guidelines than we have already.<br>
><br>
> Obviously, a metric like LOC hides some serious subtleties. Not all<br>
> changes are of equal complexity. We see massive refactoring patches<br>
> (like s/assertEquals/assertEqual/) that are actually safer than gnarly,<br>
> single-line, head-scratcher bug-fixes. The only way the report addresses<br>
> that issue with the underlying data is by eliding >10k LOC patches.<br>
><br>
<br>
</div>I'm not so sure that it is obvious what these subtleties are, or they<br>
would not be subtleties, they would be glaring issues.<br>
<br>
I agree that LOC changed is an imperfect measure. However, so are the<br>
hacking rules. They, however, have allowed us to not spend time on these<br>
things. We whole-heartedly embrace an occasional imperfection by deferring<br>
to something that can be measured by automation and thus free up valuable<br>
time for other activities more suited to limited reviewer/developer time.<br>
<br>
I'd like to see automation enforce change size. And just like with<br>
hacking, it would not be possible without a switch like "#noqa" that<br>
one can put in the commit message that says "hey automation, this is<br>
a trivial change". That is something a reviewer can also see as a cue<br>
that this change, while big, should be trivial.<br></blockquote><div><br></div><div>++</div><div><br></div><div>I put this into a bug: <a href="https://bugs.launchpad.net/hacking/+bug/1216928">https://bugs.launchpad.net/hacking/+bug/1216928</a>, and added a related bug about making hacking git-checks run as post-commit hooks (<a href="https://bugs.launchpad.net/hacking/+bug/1216925">https://bugs.launchpad.net/hacking/+bug/1216925</a>).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im"><br>
> The "one logical change per commit" is a more effective guideline than<br>
> any LOC based guideline:<br>
><br>
> <a href="https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes" target="_blank">https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes</a><br>
><br>
> IMHO, the number of distinct logical changes in a patch has a more<br>
> predictable impact on review effectiveness than the LOC metric.<br>
<br>
</div>Indeed, however, automating a check for that may be very difficult. I<br>
have seen tools like PMD[1] that try very hard to measure the complexity<br>
of code and the risk of change, and those might be interesting to see,<br>
but I'm not sure they are reliable enough to put in the OpenStack gate.<br>
<br>
[1] <a href="http://pmd.sourceforge.net/" target="_blank">http://pmd.sourceforge.net/</a><br>
<div class=""><div class="h5"><br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div></div></blockquote></div><br></div></div>