Code Review Workflows

From MediaWiki.org
Jump to navigation Jump to search

Differential workflows[edit]

Submitting a differential diff[edit]

arc diff concepts[edit]

The arc diff command will create a diff from your local code.

By default, arc diff will push the patch shown in git format-patch --stdout $(git merge-base origin/master HEAD)..HEAD up for review.

Passing a commit to arc diff, e.g., arc diff [commit] will push the code represented by the patch shown in git format-patch --stdout $(git merge-base [commit] HEAD)..HEAD.

Submitting an example diff (D36)[edit]

$ git checkout master
$ git checkout -b sync_flag
$ vim scap/main.py
$ git add scap/main.py
$ git commit

# write commit message...

[sync_flag 94d1df5] abort sync if a file named sync.flag exists
 1 file changed, 16 insertions(+)

$ arc diff

Linting...
 LINT OKAY  No lint problems.
Running unit tests...
 UNIT OKAY  No unit test failures.
 SKIP STAGING  No staging area is configured for this repository.
Updating commit message...
Created a new Differential revision:
        Revision URI: https://phabricator.wikimedia.org/D36

Included changes:
  M       scap/main.py

Submitting a draft diff[edit]

This allows you to start a differential revision that is still a work in progress and not yet ready for review. The only difference from regular revisions is that you add the --plan-changes argument to arc diff, to signify that your revision is a work in-progress/draft. This also allows you to submit a diff without explicitly naming reviewers. Optionally, you may name interested parties via the --cc argument as I have done here:

 git checkout master
 # create a new branch for your work (recommended)
 git checkout -b wip
 # make some changes
 vim some_new_thing.py
 # commit your changes:
 git commit -a -m 'WIP: some cool new thing'
 # create a revision without requesting review, but do notify user bd808:
 arc diff --plan-changes --cc bd808 master

Landing a revision[edit]

The arc land command has been greatly improved by rARCa03c6079bb71d4f8d4cd4c8c661642f753349760. Now the workflow is absolutely straightforward and arc land will almost always do the right thing.

To land your own revision that exists in a local branch, use arc land [branch-name]. To land a revision that you don't have in a local branch, pass the differential revision as the argument to arc land [revision], e.g. arc land D123

Here is a complete real-world example, including the output from the arc land command:

$ cd /path/to/local-copy/
$ arc land T119200

 TARGET  Landing onto "master", the default target under git.
 REMOTE  Using remote "origin", the default remote under git.
 FETCH  Fetching origin/master...
These commits will be landed:

      - 763b3e6 search the environment specific path if specified
      - 4d264e9 Change search_path arg to a list
      - b6f3207 Add a search_path for finding target hosts files

Landing revision 'D59: Add a search_path for finding target hosts files'...
Harbormaster is still building the active diff for this revision:

     BUILDING  Build 279: test harbormaster with jenkins
     PASSED  Build 278: arc lint + arc unit

You can review build details here:

    Harbormaster URI: https://phabricator.wikimedia.org/B217

    Land revision anyway, despite ongoing build? [y/N] y

 PUSHING  Pushing changes to "origin/master".
Counting objects: 5, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (5/5), done.
Writing objects: 100% (5/5), 1.19 KiB | 0 bytes/s, done.
Total 5 (delta 4), reused 0 (delta 0)
To ssh://vcs@git-ssh.wikimedia.org/diffusion/MSCA/scap.git
   b62f945..578cce9  578cce980ac8f78ababaed18f34c65e31851ed29 -> master
 UPDATE  Local "master" tracks target remote "origin/master", checking out and pulling changes.
 PULL  Checking out and pulling "master".
Cleaning up branch "T119200"...
(Use `git checkout -b T119200 763b3e6026ebe032e25f0eaf50a938ca733a69bb` if you want it back.)
 DONE  Landed changes.

Amending a Diff owned by another developer[edit]

Sometimes you want to amend a Diff that you did not author but still leave the ownership with the original author. Here's how:

arc patch D12345 # this applies D12345's change to your checkout of the repo
# make minor change to fix typo or rebase and fix minor conflict
git commit --amend -a
arc diff --update D12345 HEAD^
# Optionally 'arc land' if you simply want to commit the change

Re-Run Tests for a Diff[edit]

When tests fail the fault may not be with the patch being tested, but within the testing infrastructure itself. As a result, it is sometimes necessary or desirable to re-run automated tests to verify their results. To re-run tests on a submitted diff, run arc diff --update [REVISION] in your local repository. You will be prompted to fill in a change description—this is not strictly necessary for automated CI to re-run, but a reply of "recheck" will clue-in watchers of the diff as to what is happening.