Re: Recommendations on source control and code review
Karl Tomlinson schrieb: Joshua Cranmer . writes: On 4/13/2014 4:42 PM, Robert O'Callahan wrote: Honestly, I think we're already pretty close to most of those recommendations, most of the time. Some experienced Mozillians are breaking up their large changes well, but some are not. And many less experienced contributors are learning. That's a good step. IMHO, often another good step is to mostly separate every patch to its own bug (I know there are cases where it might not makes sense, but often it does) so that individual chunks can be accounted for separately in reviews, checkins, work-done reports, maybe even verifications. KaiRo ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
On Wed, Apr 16, 2014 at 9:47 AM, Robert Kaiser ka...@kairo.at wrote: That's a good step. IMHO, often another good step is to mostly separate every patch to its own bug (I know there are cases where it might not makes sense, but often it does) so that individual chunks can be accounted for separately in reviews, checkins, work-done reports, maybe even verifications. I think this is too much overhead to do for every patch, and would generally encourage people to make their patches too big. I tend to optimize the size of my patches based on reviewability and bisectability, and file different bugs based on what I expect to land in a single push. bholley ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
On 4/16/14, 10:02 AM, Bobby Holley wrote: On Wed, Apr 16, 2014 at 9:47 AM, Robert Kaiser ka...@kairo.at wrote: That's a good step. IMHO, often another good step is to mostly separate every patch to its own bug (I know there are cases where it might not makes sense, but often it does) so that individual chunks can be accounted for separately in reviews, checkins, work-done reports, maybe even verifications. I think this is too much overhead to do for every patch, and would generally encourage people to make their patches too big. I tend to optimize the size of my patches based on reviewability and bisectability, and file different bugs based on what I expect to land in a single push. I agree. Except I would like to go a step further and argue for one bug per logical idea (as opposed to per push). Patches get bit rotted and you can and should land them as soon as you get review. (If your patch N of M doesn't leave the tree in a good state, then the patch is wrong. Every commit in m-c - not just commits that were heads when they were pushed - should be able to build. This helps ensure bisection works.) I would prefer to accomplish this by shifting code review out of Bugzilla. In my ideal world, code review is conducted in a non-Bugzilla code review tool (like ReviewBoard). That code review tool intelligently updates Bugzilla when appropriate (e.g. notifies Bugzilla when a review references a specific bug). I like Phabricator's model here. See https://secure.phabricator.com/T2230. The bug/task is nice and clean and all the low-level review activity occurs in review land. You don't have issues such as comments from multiple reviews/patches getting interleaved, making discerning history of a single patch difficult. You don't have reviews interfering with discussion of the issue or tracking progress. Separation of concerns. This may sound like a radical idea, but it's really not: GitHub reviews are already successfully employing this approach (Bugzilla is updated when there is activity on GitHub). As someone who wrote more patches in 2013 than all but ~10 other people [1], I think Bugzilla overhead is real. We impose a lot of process around landing patches. These are barriers to contribution (especially compared to say GitHub pull requests) and I challenge the necessity of a lot of these procedures and requirements. Once we have a better code review story (in ReviewBoard [2]), I will be arguing for a scope reduction in Bugzilla to eliminate some of the overhead to contribution. That scope reduction will feature fewer total bugs to manage (reducing Bugzilla overhead) and hopefully loosening the culture around every patch of significance requires a bug. But let's not discuss that until ReviewBoard is in place and such a future is more realistic. [1] https://gist.github.com/indygreg/9938270 [2] https://groups.google.com/forum/#!forum/mozilla-code-review ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
On 4/14/14, 10:31 AM, smaug wrote: As a reviewer I usually want to see _also_ a patch which contains all the changes. Otherwise it can be very difficult to see the big picture. But sure, having large patches split to smaller pieces may help. btw, if you have opinions about code review tools, see the mozilla-code-review list for discussions on the integration of Review Board, BMO, and hg: https://groups.google.com/forum/#!forum/mozilla-code-review chris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
On Mon, Apr 14, 2014 at 5:01 AM, Karl Tomlinson mozn...@karlt.net wrote: Very often I've found that the intended approach changes during the life of a bug, and there is no clear summary in the bug of what eventually was done. It is then necessary to go back through multiple revisions of the patch and associated comments and replies to find out the reasoning. Add in the complexity of multiple changes in one bug, and having a single summary in one place would be very helpful, and is, when it is there. I strongly agree with this, and was particular to write detailed commit messages in the other large project I ever worked on (MediaWiki). But doesn't Mercurial hide all but the first line by default in the places you'd normally look for it (e.g., log)? If so, given how few Mozillans currently use detailed commit messages, it would be a waste of effort to write them. It's unlikely that anyone will ever look. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
On Sun, Apr 13, 2014 at 10:01 PM, Karl Tomlinson mozn...@karlt.net wrote: Very often I've found that the intended approach changes during the life of a bug, and there is no clear summary in the bug of what eventually was done. It is then necessary to go back through multiple revisions of the patch and associated comments and replies to find out the reasoning. Add in the complexity of multiple changes in one bug, and having a single summary in one place would be very helpful, and is, when it is there. Indeed, putting the rationale in the commit message helps ensure what gets pushed is what gets explained, by removing one level of indirection. But if people aren't writing good Bugzilla comments now, they're probably not going to write good commit messages either. So we need to both make it easier and enforce it more consistently through the review process. Gavin ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
On 4/14/14 5:13 AM, Aryeh Gregor wrote: But doesn't Mercurial hide all but the first line by default in the places you'd normally look for it (e.g., log)? The normal place I'd look for the detailed message is something like https://hg.mozilla.org/mozilla-central/rev/, which shows the full commit message. It's unlikely that anyone will ever look. The only time someone will look is if your code isn't doing what they think it should for whatever reason. This is true for commit messages in general. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
On 04/14/2014 12:42 AM, Robert O'Callahan wrote: On Sat, Apr 12, 2014 at 8:29 AM, Gregory Szorc g...@mozilla.com wrote: I came across the following articles on source control and code review: * https://secure.phabricator.com/book/phabflavor/article/ recommendations_on_revision_control/ * https://secure.phabricator.com/book/phabflavor/article/ writing_reviewable_code/ * https://secure.phabricator.com/book/phabflavor/article/ recommendations_on_branching/ I think everyone working on Firefox should take the time to read them as they prescribe what I perceive to be a very rational set of best practices for working with large and complex code bases. The articles were written by a (now former) Facebooker and the recommendations are significantly influenced by Facebook's experiences. They have many of the same problems we do (size and scale of code base, hundreds of developers, etc). Some of the pieces on feature development don't translate easily, but most of the content is relevant. I would be thrilled if we started adopting some of the recommendations such as more descriptive commit messages and many, smaller commits over fewer, complex commits. As a reviewer one of the first things I do when reviewing a big patch is to see if I can suggest a reasonable way to split it into smaller patches. As a reviewer I usually want to see _also_ a patch which contains all the changes. Otherwise it can be very difficult to see the big picture. But sure, having large patches split to smaller pieces may help. Honestly, I think we're already pretty close to most of those recommendations, most of the time. More descriptive commit messages is the only recommendation there that is not commonly followed, as far as I can see. I always just use the link to the bug, so I haven't seen multiline comment useful at all. Mostly they are just annoying, making logs harder to read. -Olli Rob ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
On 4/14/14, 8:31 AM, Boris Zbarsky wrote:
On 4/14/14 5:13 AM, Aryeh Gregor wrote:
But doesn't Mercurial hide all but the first line by
default in the places you'd normally look for it (e.g., log)?
The normal place I'd look for the detailed message is something like
https://hg.mozilla.org/mozilla-central/rev/, which shows the
full commit message.
Simple solution:
$ hg log -v
More advanced solution:
If the default output of `hg log` doesn't meet your fancy, it is fully
customizable!
You can modify the [ui] style config option to define a custom style
file to use. The style file holds a bunch of named templates that are
used by various Mercurial commands. The default one is at [1] and should
be installed as a map-cmdline.default file in your Mercurial install.
If you want to display all lines of the commit message by default, copy
that file and modify the changeset entry so {desc|firstline} is
{desc} and update ui.style to reference the new file.
[1]
http://selenic.com/repo/hg/file/76f68595ff8e/mercurial/templates/map-cmdline.default
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
I didn't know this existed. I filed bug 995763 to get this link added to the 'review requested' email to hopefully increase visibility. On Sat, Apr 12, 2014 at 12:10 PM, Kartikaya Gupta kgu...@mozilla.comwrote: Just a reminder that this page exists: https://developer.mozilla.org/en-US/docs/Developer_Guide/ Reviewer_Checklist and you should feel free to add things to it, and use it when reviewing any code (your own or other people's). kats On 11/4/2014, 17:47, Mike Conley wrote: Whoa, didn't expect to see a blog post I wrote in grad school to get called out here. :) Interesting to see it show up on the radar. Re-reading it, I think the most interesting thing about the Cohen study that I absorbed was the value of reviewing my own code before requesting review from other people. I've found that when I look at my own code using Splinter or Review Board, my brain switches into critique mode, and I'm able to notice and flag the obvious things. This has the dual benefit of making the code better, and making it easier for my reviewer to not get distracted by minor things that I could have caught on my own. I almost made this a topic for my graduate study research paper[1], but then did this[2] instead. Always happy to talk about code review, -Mike [1]: http://mikeconley.ca/blog/2010/03/04/research-proposal- 1-the-effects-of-author-preparation-in-peer-code-review/ [2]: http://mikeconley.ca/blog/2010/12/23/the-wisdom-of- peers-a-motive-for-exploring-peer-code-review-in-the-classroom/ On 11/04/2014 5:32 PM, Chris Peterson wrote: Code review tool company SmartBear published an interesting study [1] of the effectiveness of code reviews at Cisco. (They used SmartBear's tools, of course.) Mozillian Mike Conley reviewed SmartBear's study on his blog [2]. The results are interesting and actionable. Some highlights: * Review fewer than 200-400 lines of code at a time. * Spend no more than 60-90 minutes per review session. * Authors should pre-review their own code before submitting a review request and add explanations and questions to guide reviewers. chris [1] http://smartbear.com/SmartBear/media/pdfs/WP-CC-11- Best-Practices-of-Peer-Code-Review.pdf [2] http://mikeconley.ca/blog/2009/09/14/smart-bear-cisco- and-the-largest-study-on-code-review-ever/ On 4/11/14, 1:29 PM, Gregory Szorc wrote: I came across the following articles on source control and code review: * https://secure.phabricator.com/book/phabflavor/article/ recommendations_on_revision_control/ * https://secure.phabricator.com/book/phabflavor/article/ writing_reviewable_code/ * https://secure.phabricator.com/book/phabflavor/article/ recommendations_on_branching/ I think everyone working on Firefox should take the time to read them as they prescribe what I perceive to be a very rational set of best practices for working with large and complex code bases. The articles were written by a (now former) Facebooker and the recommendations are significantly influenced by Facebook's experiences. They have many of the same problems we do (size and scale of code base, hundreds of developers, etc). Some of the pieces on feature development don't translate easily, but most of the content is relevant. I would be thrilled if we started adopting some of the recommendations such as more descriptive commit messages and many, smaller commits over fewer, complex commits. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
On Sat, Apr 12, 2014 at 8:29 AM, Gregory Szorc g...@mozilla.com wrote: I came across the following articles on source control and code review: * https://secure.phabricator.com/book/phabflavor/article/ recommendations_on_revision_control/ * https://secure.phabricator.com/book/phabflavor/article/ writing_reviewable_code/ * https://secure.phabricator.com/book/phabflavor/article/ recommendations_on_branching/ I think everyone working on Firefox should take the time to read them as they prescribe what I perceive to be a very rational set of best practices for working with large and complex code bases. The articles were written by a (now former) Facebooker and the recommendations are significantly influenced by Facebook's experiences. They have many of the same problems we do (size and scale of code base, hundreds of developers, etc). Some of the pieces on feature development don't translate easily, but most of the content is relevant. I would be thrilled if we started adopting some of the recommendations such as more descriptive commit messages and many, smaller commits over fewer, complex commits. As a reviewer one of the first things I do when reviewing a big patch is to see if I can suggest a reasonable way to split it into smaller patches. Honestly, I think we're already pretty close to most of those recommendations, most of the time. More descriptive commit messages is the only recommendation there that is not commonly followed, as far as I can see. Rob -- Jtehsauts tshaei dS,o n Wohfy Mdaon yhoaus eanuttehrotraiitny eovni le atrhtohu gthot sf oirng iyvoeu rs ihnesa.rt sS?o Whhei csha iids teoa stiheer :p atroa lsyazye,d 'mYaonu,r sGients uapr,e tfaokreg iyvoeunr, 'm aotr atnod sgaoy ,h o'mGee.t uTph eann dt hwea lmka'n? gBoutt uIp waanndt wyeonut thoo mken.o w ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
On 4/13/2014 4:42 PM, Robert O'Callahan wrote: Honestly, I think we're already pretty close to most of those recommendations, most of the time. More descriptive commit messages is the only recommendation there that is not commonly followed, as far as I can see. I was thinking about it, and I'm not sure how useful this prescription is. Almost all of our commits require a bug number, and often the comments in the bug serve a better motivating factor for why changes need to be made rather than trying to distill discussion into a length commit comment. (This is coming from somebody who has often done deep archaeology to figure out why the hell does this broken interface exist?) -- Joshua Cranmer Thunderbird and DXR developer Source code archæologist ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
On Fri, 11 Apr 2014 13:29:01 -0700, Gregory Szorc wrote: https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code I would be thrilled if we started adopting some of the recommendations such as more descriptive commit messages and many, smaller commits over fewer, complex commits. Thank you very much, Gregory. The Many Small Commits explains things well, and the nearest thing I found in our developer docs was merely a suggestion, so I've made our recommendations stronger [1] and linked to this article. [1] https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch$compare?to=549939from=468795 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
I also added a link at https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Creating_a_patch Benoit Girard writes: I didn't know this existed. I filed bug 995763 to get this link added to the 'review requested' email to hopefully increase visibility. On Sat, Apr 12, 2014 at 12:10 PM, Kartikaya Gupta kgu...@mozilla.comwrote: Just a reminder that this page exists: https://developer.mozilla.org/en-US/docs/Developer_Guide/ Reviewer_Checklist and you should feel free to add things to it, and use it when reviewing any code (your own or other people's). kats ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
Joshua Cranmer . writes: On 4/13/2014 4:42 PM, Robert O'Callahan wrote: Honestly, I think we're already pretty close to most of those recommendations, most of the time. Some experienced Mozillians are breaking up their large changes well, but some are not. And many less experienced contributors are learning. More descriptive commit messages is the only recommendation there that is not commonly followed, as far as I can see. I was thinking about it, and I'm not sure how useful this prescription is. Almost all of our commits require a bug number, and often the comments in the bug serve a better motivating factor for why changes need to be made rather than trying to distill discussion into a length commit comment. (This is coming from somebody who has often done deep archaeology to figure out why the hell does this broken interface exist?) Very often I've found that the intended approach changes during the life of a bug, and there is no clear summary in the bug of what eventually was done. It is then necessary to go back through multiple revisions of the patch and associated comments and replies to find out the reasoning. Add in the complexity of multiple changes in one bug, and having a single summary in one place would be very helpful, and is, when it is there. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
Just a reminder that this page exists: https://developer.mozilla.org/en-US/docs/Developer_Guide/Reviewer_Checklist and you should feel free to add things to it, and use it when reviewing any code (your own or other people's). kats On 11/4/2014, 17:47, Mike Conley wrote: Whoa, didn't expect to see a blog post I wrote in grad school to get called out here. :) Interesting to see it show up on the radar. Re-reading it, I think the most interesting thing about the Cohen study that I absorbed was the value of reviewing my own code before requesting review from other people. I've found that when I look at my own code using Splinter or Review Board, my brain switches into critique mode, and I'm able to notice and flag the obvious things. This has the dual benefit of making the code better, and making it easier for my reviewer to not get distracted by minor things that I could have caught on my own. I almost made this a topic for my graduate study research paper[1], but then did this[2] instead. Always happy to talk about code review, -Mike [1]: http://mikeconley.ca/blog/2010/03/04/research-proposal-1-the-effects-of-author-preparation-in-peer-code-review/ [2]: http://mikeconley.ca/blog/2010/12/23/the-wisdom-of-peers-a-motive-for-exploring-peer-code-review-in-the-classroom/ On 11/04/2014 5:32 PM, Chris Peterson wrote: Code review tool company SmartBear published an interesting study [1] of the effectiveness of code reviews at Cisco. (They used SmartBear's tools, of course.) Mozillian Mike Conley reviewed SmartBear's study on his blog [2]. The results are interesting and actionable. Some highlights: * Review fewer than 200-400 lines of code at a time. * Spend no more than 60-90 minutes per review session. * Authors should pre-review their own code before submitting a review request and add explanations and questions to guide reviewers. chris [1] http://smartbear.com/SmartBear/media/pdfs/WP-CC-11-Best-Practices-of-Peer-Code-Review.pdf [2] http://mikeconley.ca/blog/2009/09/14/smart-bear-cisco-and-the-largest-study-on-code-review-ever/ On 4/11/14, 1:29 PM, Gregory Szorc wrote: I came across the following articles on source control and code review: * https://secure.phabricator.com/book/phabflavor/article/recommendations_on_revision_control/ * https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/ * https://secure.phabricator.com/book/phabflavor/article/recommendations_on_branching/ I think everyone working on Firefox should take the time to read them as they prescribe what I perceive to be a very rational set of best practices for working with large and complex code bases. The articles were written by a (now former) Facebooker and the recommendations are significantly influenced by Facebook's experiences. They have many of the same problems we do (size and scale of code base, hundreds of developers, etc). Some of the pieces on feature development don't translate easily, but most of the content is relevant. I would be thrilled if we started adopting some of the recommendations such as more descriptive commit messages and many, smaller commits over fewer, complex commits. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Recommendations on source control and code review
I came across the following articles on source control and code review: * https://secure.phabricator.com/book/phabflavor/article/recommendations_on_revision_control/ * https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/ * https://secure.phabricator.com/book/phabflavor/article/recommendations_on_branching/ I think everyone working on Firefox should take the time to read them as they prescribe what I perceive to be a very rational set of best practices for working with large and complex code bases. The articles were written by a (now former) Facebooker and the recommendations are significantly influenced by Facebook's experiences. They have many of the same problems we do (size and scale of code base, hundreds of developers, etc). Some of the pieces on feature development don't translate easily, but most of the content is relevant. I would be thrilled if we started adopting some of the recommendations such as more descriptive commit messages and many, smaller commits over fewer, complex commits. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
Code review tool company SmartBear published an interesting study [1] of the effectiveness of code reviews at Cisco. (They used SmartBear's tools, of course.) Mozillian Mike Conley reviewed SmartBear's study on his blog [2]. The results are interesting and actionable. Some highlights: * Review fewer than 200-400 lines of code at a time. * Spend no more than 60-90 minutes per review session. * Authors should pre-review their own code before submitting a review request and add explanations and questions to guide reviewers. chris [1] http://smartbear.com/SmartBear/media/pdfs/WP-CC-11-Best-Practices-of-Peer-Code-Review.pdf [2] http://mikeconley.ca/blog/2009/09/14/smart-bear-cisco-and-the-largest-study-on-code-review-ever/ On 4/11/14, 1:29 PM, Gregory Szorc wrote: I came across the following articles on source control and code review: * https://secure.phabricator.com/book/phabflavor/article/recommendations_on_revision_control/ * https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/ * https://secure.phabricator.com/book/phabflavor/article/recommendations_on_branching/ I think everyone working on Firefox should take the time to read them as they prescribe what I perceive to be a very rational set of best practices for working with large and complex code bases. The articles were written by a (now former) Facebooker and the recommendations are significantly influenced by Facebook's experiences. They have many of the same problems we do (size and scale of code base, hundreds of developers, etc). Some of the pieces on feature development don't translate easily, but most of the content is relevant. I would be thrilled if we started adopting some of the recommendations such as more descriptive commit messages and many, smaller commits over fewer, complex commits. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Recommendations on source control and code review
Whoa, didn't expect to see a blog post I wrote in grad school to get called out here. :) Interesting to see it show up on the radar. Re-reading it, I think the most interesting thing about the Cohen study that I absorbed was the value of reviewing my own code before requesting review from other people. I've found that when I look at my own code using Splinter or Review Board, my brain switches into critique mode, and I'm able to notice and flag the obvious things. This has the dual benefit of making the code better, and making it easier for my reviewer to not get distracted by minor things that I could have caught on my own. I almost made this a topic for my graduate study research paper[1], but then did this[2] instead. Always happy to talk about code review, -Mike [1]: http://mikeconley.ca/blog/2010/03/04/research-proposal-1-the-effects-of-author-preparation-in-peer-code-review/ [2]: http://mikeconley.ca/blog/2010/12/23/the-wisdom-of-peers-a-motive-for-exploring-peer-code-review-in-the-classroom/ On 11/04/2014 5:32 PM, Chris Peterson wrote: Code review tool company SmartBear published an interesting study [1] of the effectiveness of code reviews at Cisco. (They used SmartBear's tools, of course.) Mozillian Mike Conley reviewed SmartBear's study on his blog [2]. The results are interesting and actionable. Some highlights: * Review fewer than 200-400 lines of code at a time. * Spend no more than 60-90 minutes per review session. * Authors should pre-review their own code before submitting a review request and add explanations and questions to guide reviewers. chris [1] http://smartbear.com/SmartBear/media/pdfs/WP-CC-11-Best-Practices-of-Peer-Code-Review.pdf [2] http://mikeconley.ca/blog/2009/09/14/smart-bear-cisco-and-the-largest-study-on-code-review-ever/ On 4/11/14, 1:29 PM, Gregory Szorc wrote: I came across the following articles on source control and code review: * https://secure.phabricator.com/book/phabflavor/article/recommendations_on_revision_control/ * https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/ * https://secure.phabricator.com/book/phabflavor/article/recommendations_on_branching/ I think everyone working on Firefox should take the time to read them as they prescribe what I perceive to be a very rational set of best practices for working with large and complex code bases. The articles were written by a (now former) Facebooker and the recommendations are significantly influenced by Facebook's experiences. They have many of the same problems we do (size and scale of code base, hundreds of developers, etc). Some of the pieces on feature development don't translate easily, but most of the content is relevant. I would be thrilled if we started adopting some of the recommendations such as more descriptive commit messages and many, smaller commits over fewer, complex commits. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
