Why Code Reviews?

“it’s only through collaboration that we produce our best work”

Code Review for Teams Too Busy to Review Code (start ~16min):

Effective Code Review

Stop More Bugs with our Code Review Checklist

“developers are often distracted by the tiny problems (style issues, typos) and ignore the larger problems (algorithmic issues, design flaws, bigger picture tasks) when reviewing.”

Source: http://www.commitstrip.com/en/2015/02/10/the-truth-about-code-reviews/

Extra

Using Pull Requests as Code Reviews

A better pull request

Architecture vs. Implementation Reviews

Can Static Analysis replace Code Reviews?

Tools

I was looking for a solution to manage pull requests and code reviews with Mercurial.

HgLab

HgLab is pretty cool. Basically, it’s a GitHub interface for a Mercurial repository. It is really look-alike and there are cool features: comment about a particular commit, on a specific line, stats with code languages, pushlog, markdow, wiki, users and groups rights, …

At the time (november 2014), there was no pull request, but it was in the roadmap and GUI was already showing a dedicated button.
However, there was a feature called Commit Approvals + comments which allowed enough to watch commits and practice code review.

Unfortunately, I had performance issue with a huge repository: interface was really slow.
I contacted HgLab creator and he told me he was working on it. He was fast and precise in his answers, which was appreciable.

In the end, it was a litlle light for what I was trying to achieve :

no interface dedicated to code review
post-push review only (you have to share code with others first)
commit approval tool is convenient but too simple
no way to reject commit
no way to know if a commit was reviewed
hard to identify commits to be checked
pull request not yet available, see roadmap

Review Board

A collegue of mine tried Review Board with his team. It worked but was a bit painful to set up.

If I remember correctly, he also said that review had to be manually “asked for”, rather than automatically triggered.

There was a Visual Studio plugin but I didn’t play enough to make my opinion.

Crucible

From Atlassian (awesome tutorials and tools) Crucible overview

Phabricator

Then I discovered Phabricator, which is coded by guys from Facebook.

Presentation

“Phabricator is a collection of open source web applications that help software companies build better software.”

I’m pretty impressed.
Also it’s free!

Details

Server has to be installed on a linux box. But it’s pretty straightforward and easy installation + configuration.

Phabricator can work with svn, git and mercurial repositories.

It can either poll an existing repository, or manage one itself. Either way, their is an initial phase where the repository is fully parsed to store infos in a database. It can take a few hours for huge repositories, and it will burn your CPU and I/O but that’s it. You can add several repositories.

The GUI is excellent and is composed of a lot of tools, but in an homogenous way. Amongst apps, the most interesting ones are:

Code review (pre-push[review] and post-push[audit])
Herald (automatic notifications with a lot of customization)
Bug tracker / Tasks
Conpherence (somewhat like a tchat with markdown)
Memes support!
Projects / Flags / Hashtag / Owners
Wiki

I invite everyone to read this page: User Guide: Review vs Audit
Apart from tools, code reviews concepts are well explained.

In the end, it a great sum of awesome and well crafted tools.
Also they have a great sens of humor: have a look at the links in the footer of their homepage! (+memes generator inside)

Interface is simple and really easy to discover.
Tracability levels and details are amazing!
Want to upload an image in a comment: just drag it from your filesystem, and voilà !

Arcanist

Since then, I’ve played with Arcanist to do pre-push reviews.

Arcanist User Guide
Arcanist User Guide: Windows

User Guide: Review vs Audit
Pre-merge Code Reviews
Mondrian Code Review On The Web - Youtube

It’s not as practical with a Mercurial repository than with a git one, especially arc land, but it’s ok.

Here’s how to get started on windows:

Install PHP

Download setup on http://php.iis.net/ and install it
Fix PATH environment variable after installation to add the missing “(x86)":

C:\Program Files (x86)\PHP\v5.3

Install Arcanist

Download the following repository, either with Download ZIP link (on the right) or git:

Extract them in a common folder, for instance:

E:\BuildSystem\Phabricator\
                          \arcanist
                          \libphutil

I recommend to rename folders “arcanist” and “libphutil” in case you’ve used the download ZIP link because GitHub add -master in the name.

Then add folder E:\BuildSystem\Phabricator\arcanist\bin to the PATH environment variable.

Configure Arcanist

Once and for all, in a terminal:

Set phabricator url:

arc set-config default http://phabricator-test.esker.corp

Edit: not required if you have a .arcconfig file in your project

Allow arcanist web service:

arc install-certificate

. Copy url displayed in terminal, ex: http://phabricator-test.esker.corp/conduit/token/
. Login & copy the displayed token from the web page into your terminal

Setup editor:

arc set-config editor "\"C:\Program Files (x86)\Notepad++\notepad++.exe\" -multiInst -nosession"

According to officiel documentation: “You can not use Notepad as your editor, because it does not have a blocking mode.”
I’m using Notepad2 which works great. But Notepad++ and GitPad work too.

Submit code for pre-push review

Do one or more commits, still in draft state (not pushed).
Open a terminal and type:

arc diff

Editor is going to open, you have to specify some infos:

test plan ; can be optionnal, cf config differential.require-test-plan-field
reviewers ; you can specify one or several (comma separation)

Arc patch

Reviewer can inject the commits into his repository with arc patch <revision>. It will first update the repository on the very ancestor your development started.

Finalize push

Once code review has been approved, you can push your commit(s). Phabricator is going to automatically close the code review when it will see commits in repository (can also be done manually).

Make sure to re-do arc diff each time you bring modifications. For instance in case you rebase before push. Otherwise Phabricator will ignore the new commits because changesets don’t match.

See Differential User Guide: FAQ

arc land allows to do merge + push automatically. Unfortunately, it doesn’t work to merge a branch on itself yet, you have to use bookmarks. See ticket: https://secure.phabricator.com/T3855

Known issues

In case you have encoding warnings, you can use –encoding. See https://secure.phabricator.com/book/phabricator/article/utf8/

In case you got: “Command failed with error #1!” with “STDERR” “not found!”, you’re likely missing the default repository in mercurial. See [paths] section in your .hg/hgrc.

Go further

.arcconfig documentation

Sample:

{
	"phabricator.uri" : "http://ly-phabricator-test.esker.corp/"
}

Linters:
Arcanist User Guide: Lint
Arcanist User Guide: Customizing Existing Linters

{
	"linters":
	{
		"jshint no_SAP":
		{
			"type": "jshint",
			"bin": "C:\\Users\\TiTi\\AppData\\Roaming\\npm\\jshint.cmd",
			"include": "(^DocMgr2/WebAccess/no_SAP/ap_reject.js$)",
			"severity.rules":
			{
				"(^W)": "error"
			}
		}
	}
}

Code Review & Phabricator