Re: [openstack-dev] Code review study
On 20/08/13 11:24 -0400, Russell Bryant wrote: On 08/20/2013 11:08 AM, Daniel P. Berrange wrote: On Tue, Aug 20, 2013 at 04:02:12PM +0100, Mark McLoughlin wrote: 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. Agreed, lines of code is a particularly poor metric for evaluating commits on. Agreed, I would _strongly_ prefer no enforcement around LOC. It's just not the right metric to be looking at for a hard rule. Agreed. I think we should focus on other things like per feature patches, which make more sense. Huge patches can be split in several ones - most of the time - which will implicitly enforce a LOC limit but will let patches like s/assertEquals/assertEqual/ land, which make sense to me. This should be evaluated in a case-by-case basis. -- @flaper87 Flavio Percoco ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
-Original Message- From: Flavio Percoco [mailto:fla...@redhat.com] Sent: Monday, August 26, 2013 11:41 AM To: OpenStack Development Mailing List Subject: Re: [openstack-dev] Code review study On 20/08/13 11:24 -0400, Russell Bryant wrote: On 08/20/2013 11:08 AM, Daniel P. Berrange wrote: On Tue, Aug 20, 2013 at 04:02:12PM +0100, Mark McLoughlin wrote: 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- prac tices-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- ca se-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. Agreed, lines of code is a particularly poor metric for evaluating commits on. Agreed, I would _strongly_ prefer no enforcement around LOC. It's just not the right metric to be looking at for a hard rule. Agreed. I think we should focus on other things like per feature patches, which make more sense. Huge patches can be split in several ones - most of the time - which will implicitly enforce a LOC limit but will let patches like s/assertEquals/assertEqual/ land, which make sense to me. This should be evaluated in a case-by-case basis. [Gary Kotton] Agreed. The reviewers should use their discretion when it comes to the patch size. The flip side is that there are times when small patches based on on top of another fail to provide a complete picture of the change. -- @flaper87 Flavio Percoco ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Tue, Aug 20, 2013 at 2:21 PM, Clint Byrum cl...@fewbar.com wrote: Excerpts from Mark McLoughlin's message of 2013-08-20 03:26:01 -0700: 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. I'm not so sure that it is obvious what these subtleties are, or they would not be subtleties, they would be glaring issues. I agree that LOC changed is an imperfect measure. However, so are the hacking rules. They, however, have allowed us to not spend time on these things. We whole-heartedly embrace an occasional imperfection by deferring to something that can be measured by automation and thus free up valuable time for other activities more suited to limited reviewer/developer time. I'd like to see automation enforce change size. And just like with hacking, it would not be possible without a switch like #noqa that one can put in the commit message that says hey automation, this is a trivial change. That is something a reviewer can also see as a cue that this change, while big, should be trivial. ++ I put this into a bug: https://bugs.launchpad.net/hacking/+bug/1216928, and added a related bug about making hacking git-checks run as post-commit hooks (https://bugs.launchpad.net/hacking/+bug/1216925). 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. Indeed, however, automating a check for that may be very difficult. I have seen tools like PMD[1] that try very hard to measure the complexity of code and the risk of change, and those might be interesting to see, but I'm not sure they are reliable enough to put in the OpenStack gate. [1] http://pmd.sourceforge.net/ ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
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. Cheers, Mark. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
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. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Tue, Aug 20, 2013 at 04:02:12PM +0100, Mark McLoughlin wrote: 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. Agreed, lines of code is a particularly poor metric for evaluating commits on. Daniel -- |: 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 :| ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Tue, Aug 20, 2013 at 8:02 AM, Mark McLoughlin mar...@redhat.com wrote: On Tue, 2013-08-20 at 11:26 +0100, Mark McLoughlin wrote: The full study is here: http://support.smartbear.com/resources/cc/book/code-review-cisco-case-study.pdf I can't find the data they based their numbers on, nor their definition for a line of code, so I feel like I have to take that study with a grain of salt. We should not enforce rules like this blindly. I agree with the sentiment: less complex patches are easier to review. Reviewers should use their judgement and push back on complex patches to be split up. Jay ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
Excerpts from Mark McLoughlin's message of 2013-08-20 03:26:01 -0700: 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. I'm not so sure that it is obvious what these subtleties are, or they would not be subtleties, they would be glaring issues. I agree that LOC changed is an imperfect measure. However, so are the hacking rules. They, however, have allowed us to not spend time on these things. We whole-heartedly embrace an occasional imperfection by deferring to something that can be measured by automation and thus free up valuable time for other activities more suited to limited reviewer/developer time. I'd like to see automation enforce change size. And just like with hacking, it would not be possible without a switch like #noqa that one can put in the commit message that says hey automation, this is a trivial change. That is something a reviewer can also see as a cue that this change, while big, should be trivial. 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. Indeed, however, automating a check for that may be very difficult. I have seen tools like PMD[1] that try very hard to measure the complexity of code and the risk of change, and those might be interesting to see, but I'm not sure they are reliable enough to put in the OpenStack gate. [1] http://pmd.sourceforge.net/ ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Wed, Aug 14, 2013 at 7:12 PM, Robert Collins robe...@robertcollins.netwrote: 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. This is really interesting. I wish they would have explicitly defined lines of code. Is that git show |wc -l? Just the new lines which were added? The sum of the lines changed, removed and added? You can get vastly different numbers depending on how you count it. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Tue, Aug 20, 2013 at 5:14 AM, Jay Buffington m...@jaybuff.com wrote: This is really interesting. I wish they would have explicitly defined lines of code. Is that git show |wc -l? Just the new lines which were added? The sum of the lines changed, removed and added? You can get vastly different numbers depending on how you count it. Normally in the literature LOC is defined as non-comment, non-blank code line deltas, with a few exceptions. The exceptions normally refer to not counting braces in C-style languages and other syntactic sugar elements. Of course in Python we don't really have these issues to content with :) I'd normally include comments and docstrings too, since we review these as well. Michael... -- Michael Davies mich...@the-davies.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Aug 15, 2013, at 12:50 PM, Joe Gordon joe.gord...@gmail.com wrote: • On Thu, Aug 15, 2013 at 12:22 PM, Sam Harwell sam.harw...@rackspace.com wrote: I like to take a different approach. If my commit message is going to take more than a couple lines for people to understand the decisions I made, I go and make an issue in the issue tracker before committing locally and then reference that issue in the commit message. This helps in a few ways: 1. If I find a technical or grammatical error in the commit message, it can be corrected. 2. Developers can provide feedback on the subject matter independently of the implementation, as well as feedback on the implementation itself. 3. I like the ability to include formatting and hyperlinks in my documentation of the commit. This pattern has one slight issue, which is: • Do not assume the reviewer has access to external web services/site. In 6 months time when someone is on a train/plane/coach/beach/pub troubleshooting a problem browsing GIT history, there is no guarantee they will have access to the online bug tracker, or online blueprint documents. The great step forward with distributed SCM is that you no longer need to be online to have access to all information about the code repository. The commit message should be totally self-contained, to maintain that benefit. I'm not sure I agree with this. It can't be true in all cases, so it can hardly be considered a rule. A guideline, maybe - something to strive for. But not all artifacts of the development process are amenable to being stuffed into code or the commits associated with them. A dvcs is great and all, but unless one is working in a silo, online resources are all but mandatory. m. https://wiki.openstack.org/wiki/GitCommitMessages#Information_in_commit_messages Sam From: Christopher Yeoh [mailto:cbky...@gmail.com] Sent: Thursday, August 15, 2013 7:12 AM To: OpenStack Development Mailing List Subject: Re: [openstack-dev] Code review study On Thu, Aug 15, 2013 at 11:42 AM, Robert Collins robe...@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. Chris ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On 16 August 2013 20:15, Maru Newby ma...@redhat.com wrote: This pattern has one slight issue, which is: • Do not assume the reviewer has access to external web services/site. In 6 months time when someone is on a train/plane/coach/beach/pub troubleshooting a problem browsing GIT history, there is no guarantee they will have access to the online bug tracker, or online blueprint documents. The great step forward with distributed SCM is that you no longer need to be online to have access to all information about the code repository. The commit message should be totally self-contained, to maintain that benefit. I'm not sure I agree with this. It can't be true in all cases, so it can hardly be considered a rule. A guideline, maybe - something to strive for. But not all artifacts of the development process are amenable to being stuffed into code or the commits associated with them. A dvcs is great and all, but unless one is working in a silo, online resources are all but mandatory. In a very strict sense you're right, but consider that for anyone doing fast iterative development the need to go hit a website is a huge slowdown : at least in most of the world :). So - while I agree that it's something to strive for, I think we should invert it and say 'not having everything in the repo is something we should permit occasional exceptions to'. -Rob -- Robert Collins rbtcoll...@hp.com Distinguished Technologist HP Converged Cloud ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Aug 16, 2013, at 2:12 AM, Robert Collins robe...@robertcollins.net wrote: On 16 August 2013 20:15, Maru Newby ma...@redhat.com wrote: This pattern has one slight issue, which is: • Do not assume the reviewer has access to external web services/site. In 6 months time when someone is on a train/plane/coach/beach/pub troubleshooting a problem browsing GIT history, there is no guarantee they will have access to the online bug tracker, or online blueprint documents. The great step forward with distributed SCM is that you no longer need to be online to have access to all information about the code repository. The commit message should be totally self-contained, to maintain that benefit. I'm not sure I agree with this. It can't be true in all cases, so it can hardly be considered a rule. A guideline, maybe - something to strive for. But not all artifacts of the development process are amenable to being stuffed into code or the commits associated with them. A dvcs is great and all, but unless one is working in a silo, online resources are all but mandatory. In a very strict sense you're right, but consider that for anyone doing fast iterative development the need to go hit a website is a huge slowdown : at least in most of the world :). You're suggesting that it's possible to do _fast_ iterative development on a distributed system of immense and largely undocumented complexity (like openstack)? I'd like to be working on the code you're working on! ;) m. So - while I agree that it's something to strive for, I think we should invert it and say 'not having everything in the repo is something we should permit occasional exceptions to'. -Rob -- Robert Collins rbtcoll...@hp.com Distinguished Technologist HP Converged Cloud ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
That's an interesting article and also meaningful for coders. If I have a patch more than 200 or 300 lines, to split this may be a good idea. Some time, an easy patch with a little more lines would prevent more reviewers to think about it. On Thu, Aug 15, 2013 at 10:12 AM, Robert Collins robe...@robertcollins.netwrote: 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. -Rob -- Robert Collins rbtcoll...@hp.com Distinguished Technologist HP Converged Cloud ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Gareth *Cloud Computing, OpenStack, Fitness, Basketball* *OpenStack contributor* *Company: UnitedStack http://www.ustack.com* *My promise: if you find any spelling or grammar mistakes in my email from Mar 1 2013, notify me * *and I'll donate $1 or ￥1 to an open organization you specify.* ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Thu, Aug 15, 2013 at 7:12 AM, Christopher Yeoh cbky...@gmail.com wrote: On Thu, Aug 15, 2013 at 11:42 AM, Robert Collins robe...@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. +one million 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. Chris ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Anne Gentle annegen...@justwriteclick.com ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Thu, Aug 15, 2013 at 9:54 PM, Daniel P. Berrange berra...@redhat.comwrote: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. Agreed. There is often what and sometimes why, but not very often how in commit messages. Chris ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Thu, Aug 15, 2013 at 7:57 AM, Christopher Yeoh cbky...@gmail.com wrote: On Thu, Aug 15, 2013 at 9:54 PM, Daniel P. Berrange berra...@redhat.comwrote: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. Agreed. There is often what and sometimes why, but not very often how in commit messages. ++ Beyond the one line summary (which *should* describe what changed), describing what changed in the commit message is entirely redundant with the commit itself. Chris ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- -Dolph ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Thu, Aug 15, 2013 at 09:46:07AM -0500, Dolph Mathews wrote: On Thu, Aug 15, 2013 at 7:57 AM, Christopher Yeoh cbky...@gmail.com wrote: On Thu, Aug 15, 2013 at 9:54 PM, Daniel P. Berrange berra...@redhat.comwrote: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. Agreed. There is often what and sometimes why, but not very often how in commit messages. ++ Beyond the one line summary (which *should* describe what changed), describing what changed in the commit message is entirely redundant with the commit itself. It isn't that clearcut actually. It is quite often helpful to summarize what changed in the commit message, particularly for changes touching large areas of code, or many files. The diff's can't always be assumed to be easily readable - for example if you re-indented a large area of code, the actual what can be clear as mud. Or if there are related changes spread across many files functions, a description of what is being done will aid reviewers. Just be pragmatic about deciding when a change is complex enough that it merits summarizing the 'what', as well as the 'why'. Daniel -- |: 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 :| ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Thu, Aug 15, 2013 at 9:56 AM, Daniel P. Berrange berra...@redhat.comwrote: On Thu, Aug 15, 2013 at 09:46:07AM -0500, Dolph Mathews wrote: On Thu, Aug 15, 2013 at 7:57 AM, Christopher Yeoh cbky...@gmail.com wrote: On Thu, Aug 15, 2013 at 9:54 PM, Daniel P. Berrange berra...@redhat.comwrote: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. Agreed. There is often what and sometimes why, but not very often how in commit messages. ++ Beyond the one line summary (which *should* describe what changed), describing what changed in the commit message is entirely redundant with the commit itself. It isn't that clearcut actually. It is quite often helpful to summarize what changed in the commit message, particularly for changes touching large areas of code, or many files. Acknowledged, but keyword: *summarize*! If it really won't fit in a one line summary, that's a giant red flag that you should be producing multiple commits. The diff's can't always be assumed to be easily readable - for example if you re-indented a large area of code, the actual what can be clear as mud. Multiple commits! Or if there are related changes spread across many files functions, a description of what is being done will aid reviewers. This doesn't necessarily belong in the commit message... this can fall under item #4 in the article -- annotate your code review before it begins. Just be pragmatic about deciding when a change is complex enough that it merits summarizing the 'what', as well as the 'why'. Daniel -- |: 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:| -- -Dolph ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
On Thu, Aug 15, 2013 at 12:00 PM, Mark Washenberger mark.washenber...@markwash.net wrote: On Thu, Aug 15, 2013 at 5:12 AM, Christopher Yeoh cbky...@gmail.comwrote: On Thu, Aug 15, 2013 at 11:42 AM, Robert Collins robe...@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. Good commit messages are important, but I wonder if a more direct approach is for submitters to put review notes for their patches directly in gerrit. That allows annotation to be directly in place, without the burden of over-commenting. ++ I've done this myself when I can anticipate the questions that reviewers are going to ask anyway. The best way to get your code merged quickly is to make it as easy to review as possible! ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- -Dolph ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Code review study
I like to take a different approach. If my commit message is going to take more than a couple lines for people to understand the decisions I made, I go and make an issue in the issue tracker before committing locally and then reference that issue in the commit message. This helps in a few ways: 1. If I find a technical or grammatical error in the commit message, it can be corrected. 2. Developers can provide feedback on the subject matter independently of the implementation, as well as feedback on the implementation itself. 3. I like the ability to include formatting and hyperlinks in my documentation of the commit. Sam From: Christopher Yeoh [mailto:cbky...@gmail.com] Sent: Thursday, August 15, 2013 7:12 AM To: OpenStack Development Mailing List Subject: Re: [openstack-dev] Code review study On Thu, Aug 15, 2013 at 11:42 AM, Robert Collins robe...@robertcollins.netmailto:robe...@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. Chris ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
