Publishing code reviews
Joseph D. Darcy
Joe.Darcy at Sun.COM
Thu Oct 11 18:51:25 UTC 2007
Phil Race wrote:
> Dmitri beat me to it ..
> 'robot' also archives the webrevs and the email exchange .. so
> even the rest of the world who really didn't want to see every
> set of comments on every bug can always go look ..
> It also generates approval emails with nicely formatted putback comments.
> I was chatting yesterday with an external developer who saw
> something like this as the next step after mercurial.
Yes, we've also used the robot on the javac team. It is easy to use and
helps manage a large flow of review requests. The archival aspect is
> BTW Mark wrote :
> > In mainline
> > JDK development at least one review is required for every code change,
> > and two or more are needed as release milestones approach.
> In the client groups (2D, AWT, Swing etc) we always require two reviews,
> except for the most trivial and low-risk changes (updating a test and
> the like,
> which optionally may only require one review). I expect we will continue
> to maintain that standard.
> Dmitri Trembovetski wrote:
>> Hi Mark,
>> what about moving one of our our internal code review tools
>> to an outside server?
>> Like the one the client team uses - the 'code review robot'.
>> You submit the code review either via email or web page.
>> The webrev is then hosted on the "robot system" so the reviewers
>> can see it.
>> The communication between the reviewers happens via email, the
>> robot is CC-ed. The tool supports multiple fix revisions,
>> approvals, etc.
>> It will probably need some polishing up - since currently
>> there's no authentication, for example, but it's been used
>> by many people for a long time and is found to be very
>> convenient if not as flashy as Mondrian and some ajaxy tools..
>> Mark Reinhold wrote:
>>> One of the engineering practices that's served the JDK development team
>>> well for many years now is that of peer-based code reviews. In
>>> JDK development at least one review is required for every code change,
>>> and two or more are needed as release milestones approach.
>>> Reviews are most often performed with the help of a tool called
>>> which basically diffs two source trees and generates a pile of HTML
>>> you can view in a browser (sample: ). webrev was originally written
>>> by the Solaris team; lately it's been revised and extended  to
>>> Mercurial and Subversion in addition to the old internal TeamWare SCM.
>>> A lot of the e-mail that JDK developers read and write is, naturally,
>>> about code reviews. It's not exactly convenient to attach webrevs to
>>> e-mail messages, so people typically either copy them to a directory
>>> published by a private web server and send the URL around, or they copy
>>> them over to an NFS-exported filesystem and send a long pathname (and
>>> cross their fingers if a potential reviewer is far away network-wise).
>>> The bug here, of course, is that these publication methods don't make
>>> code reviews visible on the open web, and this is one reason that most
>>> JDK developers have thus far been reluctant to move more of their
>>> traffic onto the public openjdk.java.net mailing lists.
>>> So: How should we solve this problem in a way that works for all JDK
>>> contributors, whether or not they work for Sun?
>>> We could build something slick and fancy, like Google's Mondrian ,
>>> but I think it's relatively more important to get something up quickly
>>> and work on improving it later.
>>> A more expedient solution would be to construct something like the
>>> OpenSolaris code-review site [4,5], where contributors can use rsync,
>>> scp, and sftp (all via SSH) to upload webrevs to temporary storage on
>>> a public server. SSH keys will ultimately be required in order to push
>>> changesets into the public Mercurial repositories, so contributors will
>>> need to create and register SSH keys anyway, and the code-review site
>>> can easily leverage the same underlying infrastructure.
>>> Comments? Alternatives?
>>> - Mark
>>>  http://cr.opensolaris.org/~dp/i2o-del/
>>>  http://blogs.sun.com/dp/entry/webrev_revised
>>>  http://cr.opensolaris.org/
>>>  http://blogs.sun.com/dp/entry/cr_opensolaris_org_a_work
More information about the discuss