Code Reviews

Author(s): The Ciao Development Team.

This describes the Ciao code review workflow using Phabricator and Arcanist.

How to Submit Code for Review

Step 1: Prepare your working directory

By default Arcanist assumes the One Idea, One Commit approach for each task. In practice this means that your working directory should not contain any local changes with respect to the HEAD commit when you start working on a new task. Should you have any changes you would like to keep, use 'git stash:'

[git stash] # optional step, only if you have changes to tracked files
git checkout master
git pull origin master

Step 2: Create a local branch you will be working on

To begin, prepare your working directory by creating a branch with the name of the task that you will be working on. Assuming that your default branch is 'master' and the task is T42, do:

arc branch T42 # alternatively: arc feature T42

The 'arc branch' command creates a local git task branch and switches to it. You should name your task branch the same as the Task you are working on. If you do so, Arcanist will automatically link your Diff and the Task together.

Working with multiple branches: the task branches are set up to track the local branch you are currently working on. If you accidentally create a task branch from a wrong local branch, simply delete the task branch with 'git branch -d ...', check out the correct local branch and repeat the 'arc branch' command. It also seems to be possible in some cases to change the local branch manually or 'arc diff' against a particular local branch.

Step 3: Create a code revision (AKA a Diff)

Revisions associated with a specific task

This is the default workflow.

After you finish working on the task on the Task branch, you commit the changes and start a code review:

git commit -a "commit message"
# or add the files individually and commit them as:
#   git add FILE(s)
#   git commit -m "commit message"
arc diff

The commit message should be a one-liner (preferable no more than 80 characters) summarizing the changes. It must begin with a tag that specifies which part of the system was changed (e.g. (tests), (core), (rtchecks), fix(lpdoc), etc.).

The 'arc diff' command will open a text editor (provided in the EDITOR environment variable) with an extended commit message template. The first line(s) should correspond to the one-liner summary entered earlier. Fill the rest of the lines with the detailed information about the changes.

The Summary and Test fields are mandatory for filling (in some cases N/A can be put into Test field iff the changes do not affect any executable code). The Summary should include a more detailed explanation in several sentences or bullet points about the new changes/fixes in the Diff. The rest of the fields (Reviewers, Subscribers, Projects) can be filled later through the web interface.

When you save the commit message in the editor and exit it, Arcanist submits the changes to the Phabricator server for review and updates your local commit to include the URL of the revision on the server.

Note: the detailed message entered at this point will be used also in the web interface of Phabricator. You can manually introduce changes to it there (e.g., add/remove reviewers, associate projects, edit/update the text, etc.). This version will also be used when you commit your final changes to the master branch, so do not worry if you entered something wrong. Just make sure that the commit message in the web interface is the one you want before the final commit.

Revisions with no task

In some exceptional cases (e.g., for isolated changes related to a very specific task when there has been some previous agreement on the changes) it is acceptable to create a code revision without associating it to a specific task. However, to avoid complications during landing changes ('arc land') all the new changes should be implemented on a local branch, NOT on master. The workflow does not differ significantly for the developer and is the same for the reviewer.

To create such revision it is enough to execute the 'git commit' and 'arc diff' commands.

To close such revision it is enough to execute the 'arc amend' (or 'arc close-revision') and 'git push' commands.

Step 4: Respond to Reviews or Update a Revision

If a reviewer requests changes to the revision and you agree with those, implement them and submit a new code revision. To do this you introduce the changes as a new commit:

git commit -m "new changes description"
arc diff

Arcanist will send new changes back to the server for another round of review.

At the point of committing the final changes (see 'arc land' below), all commits in the differential revision will be flattened to just one commit with all the changes. The messages entered at this point are mostly for self-orientation, and the developer can go back easily to previous states of the local branch with the traditional 'git checkout' commands. See also the documentation for the 'arc diff --plan-changes' option below.

Note: make sure you never merge anything from the master branch (or other branches) onto your local branch while you are working on it. The SHA of the last commit on your local branch should not change until you 'arc land' changes.

If you had to roll back your commit before submitting an updated Diff and arc lost the track of it, you can still associate it with the specific revision with:

arc diff --update <revision>

Alternatively, a brand new revision can be created with:

arc diff --create

Step 5: Landing Changes

Once a reviewer requests no more modifications, the changes should be pushed to the repository with:

arc land

If no conflicts occur Arcanist merges the changes into master, deletes the local working branch, and pushes the changes to origin. Note that arc land can be executed for a Diff that was not accepted by any reviewer, but it is not advised to do so.

If conflicts occur, in most cases they are caused by unsuccessful merging of the commits that happened while you were working on the task. In most cases it should suffice to do the following:

#  fix conflicts
git commit
arc land

If this does not work, a rebase can be performed before landing:

git merge --abort
git fetch
git rebase -i origin/master
# ... fix merge conflicts
git add
git rebase --continue
# ... fix more conflicts should they occur, until the code is synced
arc land

How to Review Code

The goal of the code reviewer is to identify:

Weird coding styles.
Code in intermediate state (unless absolutely necessary).
Non-English comments, identifiers, strings (unless absolutely necessary).
Missing NODISTRIBUTE, NOCOMPILE in directories that are not ready for distribution.
Failure in automated tests.

The following subsections describe the code reviewing actions with the Phabricator web interface and the Archanist commands.

Pre-commit reviews with Differential

Checking out the Phabricator branch (e.g., D24) with the changes

To try the new changes:

Do 'git checkout' to the Diff point (or 'git checkout master' if you have most of the recent commits), and
create a local branch with 'arc patch D24,'
compile, test, ...

(This branch can be deleted later with 'git branch -D arcpatch-D24'.)

Suggesting modifications

To annotate the changes with inline comments got to Phabricator and perform click and drag on the respective line numbers.
To request changes fill the text area at the bottom of the page in Phabricator with instructions or extended comments, select Request Changes action from the drop-down list and hit the Submit button.

Updating the branch after changes

One cannot really update the branch with the patch: instead for each new update of a Diff the old patch branch should be deleted, and a fresh one created from the master branch, i.e.:

Switch to master with 'git checkout master'.
Delete the (now outdated) branch with 'git branch -D arcpatch-D24'.
Check out the branch again (which now will contain the updates): 'arc patch D24'.

Repeat the actions above until the changes look good.

Accepting Changes

To accept changes select in Phabricator the Accept Revision action from the drop-down list and hit the Submit button.

Post-commit reviews with Audit

For the commits that were pushed/landed without prior review it is still possible to make a code review through Phabricator's web interface:

from the main page navigate to the Audit section in the tools list;
select the commit of interest, review it, annotate it with comments if needed;
in the Add Action... menu at the bottom of the web page either Accept Commit or Raise Concern.

Some Useful `arcanist` Commands

Note: invocations of the arc utility require a working Internet connection.

'arc feature': Lists the available branches and their revision status (like git branch but adding extra information).
'arc diff --plan-changes': Updated the current Diff but does not request a review from its reviewers, useful for incomplete temporal saves (e.g., daily saves).

Also, arc understands some keywords in commit messages (see this web page for details):

attach a revision or a commit to a task with Ref T42 in the summary/commit message;
close a task when pushing a commit with Fixes T42 in the summary/commit message;

These commands also work for regular commits (e.g., pushed via git push, not only during arc land).