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*. |
Hans Wennborg | ac1d6a0 | 2013-07-25 22:58:31 +0000 | [diff] [blame] | 63 | * Click *Create Diff*. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 64 | * Paste the text diff or upload the patch file. |
| 65 | Note that TODO |
| 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 |
Paul Robinson | 6f4a19f | 2015-01-13 00:50:31 +0000 | [diff] [blame] | 71 | for LLVM, add llvm-commits as a subscriber; if your patch is for Clang, |
| 72 | add cfe-commits. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 73 | * Click *Save*. |
| 74 | |
| 75 | To submit an updated patch: |
| 76 | |
| 77 | * Click *Differential*. |
Sylvestre Ledru | def97f4 | 2013-11-11 14:27:56 +0000 | [diff] [blame] | 78 | * Click *Create Diff*. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 79 | * Paste the updated diff. |
| 80 | * Select the review you want to from the *Attach To* dropdown and click |
| 81 | *Continue*. |
| 82 | * Click *Save*. |
| 83 | |
| 84 | Reviewing code with Phabricator |
| 85 | ------------------------------- |
| 86 | |
| 87 | Phabricator allows you to add inline comments as well as overall comments |
| 88 | to a revision. To add an inline comment, select the lines of code you want |
| 89 | 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] | 90 | When you have added all your comments, scroll to the bottom of the page and |
| 91 | click the Submit button. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 92 | |
Paul Robinson | c02e858 | 2015-03-30 21:27:28 +0000 | [diff] [blame] | 93 | You can add overall comments in the text box at the bottom of the page. |
| 94 | When you're done, click the Submit button. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 95 | |
| 96 | Phabricator has many useful features, for example allowing you to select |
| 97 | diffs between different versions of the patch as it was reviewed in the |
| 98 | *Revision Update History*. Most features are self descriptive - explore, and |
| 99 | if you have a question, drop by on #llvm in IRC to get help. |
| 100 | |
Manuel Klimek | 51e31d2 | 2013-02-13 09:07:18 +0000 | [diff] [blame] | 101 | Note that as e-mail is the system of reference for code reviews, and some |
| 102 | people prefer it over a web interface, we do not generate automated mail |
| 103 | when a review changes state, for example by clicking "Accept Revision" in |
| 104 | the web interface. Thus, please type LGTM into the comment box to accept |
| 105 | a change from Phabricator. |
| 106 | |
Mark Seaborn | 6e15ee8 | 2014-02-11 16:58:03 +0000 | [diff] [blame] | 107 | Committing a change |
| 108 | ------------------- |
| 109 | |
Sylvestre Ledru | 4c16e61 | 2014-07-02 15:25:25 +0000 | [diff] [blame] | 110 | Arcanist can manage the commit transparently. It will retrieve the description, |
| 111 | reviewers, the ``Differential Revision``, etc from the review and commit it to the repository. |
| 112 | |
| 113 | :: |
| 114 | |
Sylvestre Ledru | ae0fc23 | 2014-07-04 09:00:35 +0000 | [diff] [blame] | 115 | arc patch D<Revision> |
Sylvestre Ledru | 4c16e61 | 2014-07-02 15:25:25 +0000 | [diff] [blame] | 116 | arc commit --revision D<Revision> |
| 117 | |
| 118 | |
Mark Seaborn | 6e15ee8 | 2014-02-11 16:58:03 +0000 | [diff] [blame] | 119 | When committing an LLVM change that has been reviewed using |
| 120 | Phabricator, the convention is for the commit message to end with the |
| 121 | line: |
| 122 | |
| 123 | :: |
| 124 | |
| 125 | Differential Revision: <URL> |
| 126 | |
| 127 | where ``<URL>`` is the URL for the code review, starting with |
Manuel Klimek | 234b86b | 2014-04-07 10:21:33 +0000 | [diff] [blame] | 128 | ``http://reviews.llvm.org/``. |
Mark Seaborn | 6e15ee8 | 2014-02-11 16:58:03 +0000 | [diff] [blame] | 129 | |
| 130 | Note that Arcanist will add this automatically. |
| 131 | |
| 132 | This allows people reading the version history to see the review for |
| 133 | context. This also allows Phabricator to detect the commit, close the |
| 134 | review, and add a link from the review to the commit. |
| 135 | |
Paul Robinson | c02e858 | 2015-03-30 21:27:28 +0000 | [diff] [blame] | 136 | Abandoning a change |
| 137 | ------------------- |
| 138 | |
| 139 | If you decide you should not commit the patch, you should explicitly abandon |
| 140 | the review so that reviewers don't think it is still open. In the web UI, |
| 141 | scroll to the bottom of the page where normally you would enter an overall |
| 142 | comment. In the drop-down Action list, which defaults to "Comment," you should |
| 143 | select "Abandon Revision" and then enter a comment explaining why. Click the |
| 144 | Submit button to finish closing the review. |
| 145 | |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 146 | Status |
| 147 | ------ |
| 148 | |
Sylvestre Ledru | 75a58f4 | 2014-05-14 09:22:15 +0000 | [diff] [blame] | 149 | Please let us know whether you like it and what could be improved! |
Sean Silva | b39f47b | 2012-10-12 01:21:24 +0000 | [diff] [blame] | 150 | |
Manuel Klimek | 234b86b | 2014-04-07 10:21:33 +0000 | [diff] [blame] | 151 | .. _LLVM's Phabricator: http://reviews.llvm.org |
Reid Kleckner | 8b91d36 | 2014-06-25 20:25:21 +0000 | [diff] [blame] | 152 | .. _`http://reviews.llvm.org`: http://reviews.llvm.org |
Manuel Klimek | 234b86b | 2014-04-07 10:21:33 +0000 | [diff] [blame] | 153 | .. _Code Repository Browser: http://reviews.llvm.org/diffusion/ |
Sean Silva | b39f47b | 2012-10-12 01:21:24 +0000 | [diff] [blame] | 154 | .. _Arcanist Quick Start: http://www.phabricator.com/docs/phabricator/article/Arcanist_Quick_Start.html |
| 155 | .. _Arcanist User Guide: http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide.html |