Important Changes to MozReview

May 29, 2015 at 04:20 PM | categories: MozReview, Mozilla, code review | View Comments

This was a busy week for MozReview! There are a number of changes people need to be aware of.

Support for Specifying Reviewers via Commit Messages

MozReview will now parse r?gps syntax out of commit messages to set reviewers for pushed commits.

Read the docs for more information, including why we are preferring r? to r=.

Since it landed, a number of workflow concerns have been reported. See the bug tree for bug 1142251 before filing a bug to help avoid duplicates.

Thank Dan Minor for the feature!

Review Attachment/Flag Per Commit

Since the beginning of MozReview, there was one Bugzilla attachment / review flag per commit series. This has changed to one attachment / review flag per commit.

Before, you needed to grant Ship It on the parent/root review request in order to r+ the MozReview review request. Now, you grant Ship It on individual commits and these turn into individual r+ on Bugzilla. To reinforce that reviewing the parent/root review request doesn't do anything meaningful any more, the Ship It button and checkbox have been removed from the parent/root review request.

The new model more closely maps to how code review in Bugzilla has worked at Mozilla for ages. And, it is a superior workflow for future workflows we're trying to enable.

We tried to run a one-time migration script to convert existing parent/root attachments/review flags to per-commit attachments/flags. However, there were issues. We will attempt again sometime next week. In the interim, in-flight review requests may enter an inconsistent state if they are updated. If a new push is performed, the old parent/root attachment/review flag may linger and per-commit attachments/flags will be created. This could be confusing. The workaround is to manually clear the r? flag from the parent/root attachment or wait for the migration script to run in a few days.

Mark Côté put in a lot of hard work to make this change happen.

r? Flags Cleared After Review

Before, submitting a review without granting Ship It wouldn't do anything to the r? flag: the r? flag would linger.

Now, submitting review without granting Ship It will clear the r? flag. We think the new default is better for the majority of users. However, we recognize it isn't always wanted. There is a bug open to provide a checkbox to keep the r? flag present.

Metadata Added to Changesets

If you update to the tip of the version-control-tools repository (you should do this every week or so to stay fresh - use mach mercurial-setup to do this automatically), metadata will automatically be added to commits when working with MozReview-enabled repositories.

Specifically, we insert a hidden, unique, random ID into every changeset. This ID can be used to map commits to each other. We don't use this ID yet. But we have plans.

A side-effect of this change is that you can no longer push to MozReview if you have uncommitted local changes. If this is annoying, use hg shelve and hg unshelve to create and undo temporary commits. If this is too annoying, complain by filing a bug and we can look into doing this automatically as part of pushing.

What's Next?

We're actively working on more workflow enhancements to make MozReview an even more compelling experience.

I'm building a web service to query file metadata from moz.build files. This will allow MozReview to automatically file bugs in proper components based on what files changed. Once code reviewer metadata is added to moz.build files, it will enable us to assign reviewers automatically as well. The end goal here is to lower the number of steps needed to turn changed code into a landing. This will be useful when we turn GitHub pull requests into MozReview review requests (random GitHub contributors shouldn't need to know who to flag for review, nor should they be required to file a bug if they write some code). Oh year, we're working on integrating GitHub pull requests.

Another area of focus is better commit tracking and partially landed series. I have preliminary patches for automatically adding review URL annotations to commit messages. This will enable people to easily go from commit (message) to MozReview, without having to jump through Bugzilla. This also enables better commit tracking. If you e.g. perform complicated history rewriting, the review URL annotation will enable the MozReview server to better map previously-submitted commits to existing review requests. Partially landed series will enable commits to land as soon as they are reviewed, without having to wait on the entire series. It's my strong belief that if a commit is granted review, it should land as soon as possible. This helps prevent bit rot of ready-to-land commits. Landings also make people feel better because you feel like you've accomplished something. Positive feedback loops are good.

Major work is also being done to overhaul the web UI. The commit series view (which is currently populated via XHR) will soon be generated on the server and served as part of the page. This should make pages load a bit faster. And, things should be prettier as well.

And, of course, work is being invested into Autoland. Support for submitting pushes to Try landed a few weeks ago. Having Autoland actually land reviewed commits is on the radar.

Exciting times are ahead. Please continue to provide feedback. If you see something, say something.

blog comments powered by Disqus