|
|
| (17 intermediate revisions by 3 users not shown) |
| Line 1: |
Line 1: |
| = Status =
| | MozReview has been decommissioned in favour of [https://phabricator.services.mozilla.com Phabricator] and [https://lando.services.mozilla.com Lando]. |
| | |
| Requirements for a next-generation code-review tool are settled and
| |
| the design complete. We're currently fixing up a few last rough edges
| |
| before general deployment; see blockers on {{bug|1021929}}.
| |
| | |
| We are aiming for deployment in mid-August 2014.
| |
| | |
| = Background =
| |
| | |
| == The old: patches ==
| |
| | |
| The main code-review tool in use at Mozilla is currently (July 2014)
| |
| Splinter, which is integrated into Bugzilla. Splinter uses a very old
| |
| approach to code reviews; it merely displays side-by-side, colourized
| |
| versions of the attached patch file and allows you to add comments to the diff. While tools such as [http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hgext/bzexport bzexport] have improved usability, Splinter is fundamentally limited. Adding useful features like increased context (outside of the patch itself), history, interdiffs, and so on are possible to varying degrees, all are made difficult or largely impossible by the fact that Splinter is based on patches.
| |
| | |
| == The new: branches ==
| |
| | |
| GitHub really popularized the idea of doing branch-based code
| |
| reviews. You have a forked repo of a project (which I am told is,
| |
| under the covers, actually branches, not a separate repo), you push
| |
| changes to your repo, and then you issue a Pull Request, which is
| |
| essentially a way for the author of the main fork to review your
| |
| changes and then easily merge them in.
| |
| | |
| Setting aside the last part--which we'll get to later--the advantages
| |
| of this code-review system are several, notably
| |
| | |
| * You can get full context of the changes.
| |
| * You can address review points by pushing a new commit, which inherently allows interdiffing.
| |
| * You can easily pull down the branch under review for testing.
| |
| | |
| Several other tools, such as [https://code.google.com/p/gerrit/ Gerrit],
| |
| implement this functionality as well, but sadly none of the good
| |
| options support multiple VCSes, and Mozilla still uses mercurial
| |
| heavily.
| |
| | |
| = The Solution: Review Board and friends =
| |
| | |
| This is just the basics of the model, with details (including a how-to) to come.
| |
| | |
| [http://www.reviewboard.org/ Review Board] is an open-source review
| |
| tool which supports just about every VCS out there. Unfortunately, it
| |
| is also fundamentally a patch-based system, although it does know
| |
| about repositories' canonical location so it can grab context
| |
| that isn't in the diff itself--at least, if the patch still applies.
| |
| This makes it still a fancy version of Splinter, fundamentally.
| |
| | |
| Luckily, we have all kinds of clever people here with intimate
| |
| knowledge of VCSes, including mercurial, so in late winter they
| |
| banged together a prototype showing that it is possible to convert
| |
| Review Board to a branch-based system after all!
| |
| | |
| As an aside, we we say "branches", we mean the git concept of
| |
| them--lightweight. Branches in mercurial are not the same at all;
| |
| however, there are two alternatives that provide similar
| |
| functionality: [https://developer.mozilla.org/en-US/docs/Mercurial_Queues Mercurial Queues], now considered obsolete by many, and [http://gregoryszorc.com/blog/2014/06/23/please-stop-using-mq/ hg bookmarks]. Both are supported by our system, though the latter is preferred
| |
| | |
| Since then, we've settled on a concrete design that makes submitting
| |
| patches as painless as possible, as well as providing integration to
| |
| Bugzilla. The main components are
| |
| | |
| * Review repos. Similar to the [https://wiki.mozilla.org/ReleaseEngineering/TryServer Try server], review repos are clones of existing repos, such as mozilla-central.
| |
| | |
| * Client and server [http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hgext/reviewboard extensions] for mercurial that allow bookmarks (and MQs for people who haven't made the switch) to be pushed to the review repos. This is how we preserve history.
| |
| | |
| * Extensions for Review Board:
| |
| ** [http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/rbmozui rbmozui], which has some UI customizations to make the branch-based workflow more obvious (and which will later provide a Mozilla look).
| |
| ** [http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/rbbz rbbz], which integrates Review Board with Bugzilla and does a few other housekeeping tasks with the extra data we're using in our branch-based model.
| |
| | |
| == Bugzilla integration ==
| |
| | |
| The rbbz extension ties Review Board to Bugzilla in several ways:
| |
| | |
| * All authentication, aside from the built-in admin user, is done with Bugzilla.
| |
| * All user searching and loading is referred to Bugzilla so that we have up-to-date user info.
| |
| * When a review request is published in Review Board, an attachment is created in the corresponding bug. This attachment contains the URL to the review request and automatically redirects when you click on it (as GitHub pull-requests URLs do already).
| |
| ** Users entered into the "Target People" field are flagged with r? on the Bugzilla attachment.
| |
| * Any comments ("reviews" in Review Board speak) are mirrored (one-way, for sanity) to Bugzilla. As with Splinter, some diff context is preserved along with the comment.
| |
| * When someone clicks "Ship it!" (the Review Board equivalent of r+), the associated Bugzilla attachment is r+ed by that user.
| |
| | |
| There are more plans for Bugzilla integration, but this was deemed the minimum
| |
| feature set to make the system usable.
| |
| | |
| == The Future: landing reviews ==
| |
| | |
| This model lends itself quite well to the work being done on AutoLand
| |
| and hg transplants. It should be possible to easily, or even
| |
| automatically, push the reviewed commits onto the target repository, a
| |
| long-sought-after feature that is extremely tricky, perhaps
| |
| impossible, with the current patch-based model.
| |