Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 1 | ============================= |
| 2 | Code Reviews with Phabricator |
| 3 | ============================= |
| 4 | |
| 5 | .. contents:: |
| 6 | :local: |
| 7 | |
Reid Kleckner | 8b91d36 | 2014-06-25 20:25:21 +0000 | [diff] [blame] | 8 | If you prefer to use a web user interface for code reviews, you can now submit |
| 9 | your patches for Clang and LLVM at `LLVM's Phabricator`_ instance. |
| 10 | |
| 11 | While Phabricator is a useful tool for some, the relevant -commits mailing list |
| 12 | is the system of record for all LLVM code review. The mailing list should be |
Sanjay Patel | ec0c53d | 2014-06-26 18:12:42 +0000 | [diff] [blame] | 13 | added as a subscriber on all reviews, and Phabricator users should be prepared |
| 14 | to respond to free-form comments in mail sent to the commits list. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 15 | |
| 16 | Sign up |
| 17 | ------- |
| 18 | |
Reid Kleckner | 8b91d36 | 2014-06-25 20:25:21 +0000 | [diff] [blame] | 19 | To get started with Phabricator, navigate to `http://reviews.llvm.org`_ and |
| 20 | click the power icon in the top right. You can register with a GitHub account, |
| 21 | a Google account, or you can create your own profile. |
| 22 | |
Sanjay Patel | ec0c53d | 2014-06-26 18:12:42 +0000 | [diff] [blame] | 23 | Make *sure* that the email address registered with Phabricator is subscribed |
Reid Kleckner | 8b91d36 | 2014-06-25 20:25:21 +0000 | [diff] [blame] | 24 | to the relevant -commits mailing list. If your are not subscribed to the commit |
| 25 | list, all mail sent by Phabricator on your behalf will be held for moderation. |
| 26 | |
Chandler Carruth | 34e3477 | 2012-10-27 09:47:33 +0000 | [diff] [blame] | 27 | Note that if you use your Subversion user name as Phabricator user name, |
| 28 | Phabricator will automatically connect your submits to your Phabricator user in |
| 29 | the `Code Repository Browser`_. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 30 | |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 31 | Requesting a review via the command line |
| 32 | ---------------------------------------- |
| 33 | |
| 34 | Phabricator has a tool called *Arcanist* to upload patches from |
| 35 | the command line. To get you set up, follow the |
| 36 | `Arcanist Quick Start`_ instructions. |
| 37 | |
| 38 | You can learn more about how to use arc to interact with |
| 39 | Phabricator in the `Arcanist User Guide`_. |
| 40 | |
| 41 | Requesting a review via the web interface |
| 42 | ----------------------------------------- |
| 43 | |
| 44 | The tool to create and review patches in Phabricator is called |
| 45 | *Differential*. |
| 46 | |
| 47 | Note that you can upload patches created through various diff tools, |
| 48 | including git and svn. To make reviews easier, please always include |
| 49 | **as much context as possible** with your diff! Don't worry, Phabricator |
| 50 | will automatically send a diff with a smaller context in the review |
| 51 | email, but having the full file in the web interface will help the |
| 52 | reviewer understand your code. |
| 53 | |
| 54 | To get a full diff, use one of the following commands (or just use Arcanist |
| 55 | to upload your patch): |
| 56 | |
Alexey Samsonov | bdb2594 | 2012-11-06 15:04:37 +0000 | [diff] [blame] | 57 | * ``git diff -U999999 other-branch`` |
| 58 | * ``svn diff --diff-cmd=diff -x -U999999`` |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 59 | |
| 60 | To upload a new patch: |
| 61 | |
| 62 | * Click *Differential*. |
Scott Douglass | a0cda9b | 2015-07-01 13:41:18 +0000 | [diff] [blame^] | 63 | * Click *+ Create Diff*. |
| 64 | * Paste the text diff or browse to the patch file. Click *Create Diff*. |
| 65 | * Leave the Repository field blank. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 66 | * Leave the drop down on *Create a new Revision...* and click *Continue*. |
Scott Douglass | db32277 | 2015-03-31 15:07:53 +0000 | [diff] [blame] | 67 | * Enter a descriptive title and summary. The title and summary are usually |
| 68 | in the form of a :ref:`commit message <commit messages>`. |
| 69 | * Add reviewers and mailing |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 70 | lists that you want to be included in the review. If your patch is |
Scott Douglass | a0cda9b | 2015-07-01 13:41:18 +0000 | [diff] [blame^] | 71 | for LLVM, add llvm-commits as a Subscriber; if your patch is for Clang, |
Paul Robinson | 6f4a19f | 2015-01-13 00:50:31 +0000 | [diff] [blame] | 72 | add cfe-commits. |
Scott Douglass | a0cda9b | 2015-07-01 13:41:18 +0000 | [diff] [blame^] | 73 | * Leave the Repository and Project fields blank. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 74 | * Click *Save*. |
| 75 | |
| 76 | To submit an updated patch: |
| 77 | |
| 78 | * Click *Differential*. |
Scott Douglass | a0cda9b | 2015-07-01 13:41:18 +0000 | [diff] [blame^] | 79 | * Click *+ Create Diff*. |
| 80 | * Paste the updated diff or browse to the updated patch file. Click *Create Diff*. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 81 | * Select the review you want to from the *Attach To* dropdown and click |
| 82 | *Continue*. |
Scott Douglass | a0cda9b | 2015-07-01 13:41:18 +0000 | [diff] [blame^] | 83 | * Leave the Repository and Project fields blank. |
| 84 | * Add comments about the changes in the new diff. Click *Save*. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 85 | |
| 86 | Reviewing code with Phabricator |
| 87 | ------------------------------- |
| 88 | |
| 89 | Phabricator allows you to add inline comments as well as overall comments |
| 90 | to a revision. To add an inline comment, select the lines of code you want |
| 91 | to comment on by clicking and dragging the line numbers in the diff pane. |
Paul Robinson | c02e858 | 2015-03-30 21:27:28 +0000 | [diff] [blame] | 92 | When you have added all your comments, scroll to the bottom of the page and |
| 93 | click the Submit button. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 94 | |
Paul Robinson | c02e858 | 2015-03-30 21:27:28 +0000 | [diff] [blame] | 95 | You can add overall comments in the text box at the bottom of the page. |
| 96 | When you're done, click the Submit button. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 97 | |
| 98 | Phabricator has many useful features, for example allowing you to select |
| 99 | diffs between different versions of the patch as it was reviewed in the |
| 100 | *Revision Update History*. Most features are self descriptive - explore, and |
| 101 | if you have a question, drop by on #llvm in IRC to get help. |
| 102 | |
Manuel Klimek | 51e31d2 | 2013-02-13 09:07:18 +0000 | [diff] [blame] | 103 | Note that as e-mail is the system of reference for code reviews, and some |
| 104 | people prefer it over a web interface, we do not generate automated mail |
| 105 | when a review changes state, for example by clicking "Accept Revision" in |
| 106 | the web interface. Thus, please type LGTM into the comment box to accept |
| 107 | a change from Phabricator. |
| 108 | |
Mark Seaborn | 6e15ee8 | 2014-02-11 16:58:03 +0000 | [diff] [blame] | 109 | Committing a change |
| 110 | ------------------- |
| 111 | |
Sylvestre Ledru | 4c16e61 | 2014-07-02 15:25:25 +0000 | [diff] [blame] | 112 | Arcanist can manage the commit transparently. It will retrieve the description, |
| 113 | reviewers, the ``Differential Revision``, etc from the review and commit it to the repository. |
| 114 | |
| 115 | :: |
| 116 | |
Sylvestre Ledru | ae0fc23 | 2014-07-04 09:00:35 +0000 | [diff] [blame] | 117 | arc patch D<Revision> |
Sylvestre Ledru | 4c16e61 | 2014-07-02 15:25:25 +0000 | [diff] [blame] | 118 | arc commit --revision D<Revision> |
| 119 | |
| 120 | |
Mark Seaborn | 6e15ee8 | 2014-02-11 16:58:03 +0000 | [diff] [blame] | 121 | When committing an LLVM change that has been reviewed using |
| 122 | Phabricator, the convention is for the commit message to end with the |
| 123 | line: |
| 124 | |
| 125 | :: |
| 126 | |
| 127 | Differential Revision: <URL> |
| 128 | |
| 129 | where ``<URL>`` is the URL for the code review, starting with |
Manuel Klimek | 234b86b | 2014-04-07 10:21:33 +0000 | [diff] [blame] | 130 | ``http://reviews.llvm.org/``. |
Mark Seaborn | 6e15ee8 | 2014-02-11 16:58:03 +0000 | [diff] [blame] | 131 | |
| 132 | Note that Arcanist will add this automatically. |
| 133 | |
| 134 | This allows people reading the version history to see the review for |
| 135 | context. This also allows Phabricator to detect the commit, close the |
| 136 | review, and add a link from the review to the commit. |
| 137 | |
Paul Robinson | c02e858 | 2015-03-30 21:27:28 +0000 | [diff] [blame] | 138 | Abandoning a change |
| 139 | ------------------- |
| 140 | |
| 141 | If you decide you should not commit the patch, you should explicitly abandon |
| 142 | the review so that reviewers don't think it is still open. In the web UI, |
| 143 | scroll to the bottom of the page where normally you would enter an overall |
| 144 | comment. In the drop-down Action list, which defaults to "Comment," you should |
| 145 | select "Abandon Revision" and then enter a comment explaining why. Click the |
| 146 | Submit button to finish closing the review. |
| 147 | |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 148 | Status |
| 149 | ------ |
| 150 | |
Chandler Carruth | 792b94c | 2015-05-27 07:20:46 +0000 | [diff] [blame] | 151 | Please let us know whether you like it and what could be improved! We're still |
| 152 | working on setting up a bug tracker, but you can email klimek-at-google-dot-com |
| 153 | and chandlerc-at-gmail-dot-com and CC the llvmdev mailing list with questions |
| 154 | until then. We also could use help implementing improvements. This sadly is |
| 155 | really painful and hard because the Phabricator codebase is in PHP and not as |
| 156 | testable as you might like. However, we've put exactly what we're deploying up |
| 157 | on an `llvm-reviews GitHub project`_ where folks can hack on it and post pull |
| 158 | requests. We're looking into what the right long-term hosting for this is, but |
| 159 | note that it is a derivative of an existing open source project, and so not |
| 160 | trivially a good fit for an official LLVM project. |
Sean Silva | b39f47b | 2012-10-12 01:21:24 +0000 | [diff] [blame] | 161 | |
Manuel Klimek | 234b86b | 2014-04-07 10:21:33 +0000 | [diff] [blame] | 162 | .. _LLVM's Phabricator: http://reviews.llvm.org |
Reid Kleckner | 8b91d36 | 2014-06-25 20:25:21 +0000 | [diff] [blame] | 163 | .. _`http://reviews.llvm.org`: http://reviews.llvm.org |
Manuel Klimek | 234b86b | 2014-04-07 10:21:33 +0000 | [diff] [blame] | 164 | .. _Code Repository Browser: http://reviews.llvm.org/diffusion/ |
Sean Silva | b39f47b | 2012-10-12 01:21:24 +0000 | [diff] [blame] | 165 | .. _Arcanist Quick Start: http://www.phabricator.com/docs/phabricator/article/Arcanist_Quick_Start.html |
| 166 | .. _Arcanist User Guide: http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide.html |
Chandler Carruth | 792b94c | 2015-05-27 07:20:46 +0000 | [diff] [blame] | 167 | .. _llvm-reviews GitHub project: https://github.com/r4nt/llvm-reviews/ |