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*. |
| 67 | * Enter a descriptive title and summary; add reviewers and mailing |
| 68 | lists that you want to be included in the review. If your patch is |
| 69 | for LLVM, cc llvm-commits; if your patch is for Clang, cc cfe-commits. |
| 70 | * Click *Save*. |
| 71 | |
| 72 | To submit an updated patch: |
| 73 | |
| 74 | * Click *Differential*. |
Sylvestre Ledru | def97f4 | 2013-11-11 14:27:56 +0000 | [diff] [blame] | 75 | * Click *Create Diff*. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 76 | * Paste the updated diff. |
| 77 | * Select the review you want to from the *Attach To* dropdown and click |
| 78 | *Continue*. |
| 79 | * Click *Save*. |
| 80 | |
| 81 | Reviewing code with Phabricator |
| 82 | ------------------------------- |
| 83 | |
| 84 | Phabricator allows you to add inline comments as well as overall comments |
| 85 | to a revision. To add an inline comment, select the lines of code you want |
| 86 | to comment on by clicking and dragging the line numbers in the diff pane. |
| 87 | |
| 88 | You can add overall comments or submit your comments at the bottom of the page. |
| 89 | |
| 90 | Phabricator has many useful features, for example allowing you to select |
| 91 | diffs between different versions of the patch as it was reviewed in the |
| 92 | *Revision Update History*. Most features are self descriptive - explore, and |
| 93 | if you have a question, drop by on #llvm in IRC to get help. |
| 94 | |
Manuel Klimek | 51e31d2 | 2013-02-13 09:07:18 +0000 | [diff] [blame] | 95 | Note that as e-mail is the system of reference for code reviews, and some |
| 96 | people prefer it over a web interface, we do not generate automated mail |
| 97 | when a review changes state, for example by clicking "Accept Revision" in |
| 98 | the web interface. Thus, please type LGTM into the comment box to accept |
| 99 | a change from Phabricator. |
| 100 | |
Mark Seaborn | 6e15ee8 | 2014-02-11 16:58:03 +0000 | [diff] [blame] | 101 | Committing a change |
| 102 | ------------------- |
| 103 | |
Sylvestre Ledru | 4c16e61 | 2014-07-02 15:25:25 +0000 | [diff] [blame^] | 104 | Arcanist can manage the commit transparently. It will retrieve the description, |
| 105 | reviewers, the ``Differential Revision``, etc from the review and commit it to the repository. |
| 106 | |
| 107 | :: |
| 108 | |
| 109 | arc commit --revision D<Revision> |
| 110 | |
| 111 | |
Mark Seaborn | 6e15ee8 | 2014-02-11 16:58:03 +0000 | [diff] [blame] | 112 | When committing an LLVM change that has been reviewed using |
| 113 | Phabricator, the convention is for the commit message to end with the |
| 114 | line: |
| 115 | |
| 116 | :: |
| 117 | |
| 118 | Differential Revision: <URL> |
| 119 | |
| 120 | where ``<URL>`` is the URL for the code review, starting with |
Manuel Klimek | 234b86b | 2014-04-07 10:21:33 +0000 | [diff] [blame] | 121 | ``http://reviews.llvm.org/``. |
Mark Seaborn | 6e15ee8 | 2014-02-11 16:58:03 +0000 | [diff] [blame] | 122 | |
| 123 | Note that Arcanist will add this automatically. |
| 124 | |
| 125 | This allows people reading the version history to see the review for |
| 126 | context. This also allows Phabricator to detect the commit, close the |
| 127 | review, and add a link from the review to the commit. |
| 128 | |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 129 | Status |
| 130 | ------ |
| 131 | |
Sylvestre Ledru | 75a58f4 | 2014-05-14 09:22:15 +0000 | [diff] [blame] | 132 | 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] | 133 | |
Manuel Klimek | 234b86b | 2014-04-07 10:21:33 +0000 | [diff] [blame] | 134 | .. _LLVM's Phabricator: http://reviews.llvm.org |
Reid Kleckner | 8b91d36 | 2014-06-25 20:25:21 +0000 | [diff] [blame] | 135 | .. _`http://reviews.llvm.org`: http://reviews.llvm.org |
Manuel Klimek | 234b86b | 2014-04-07 10:21:33 +0000 | [diff] [blame] | 136 | .. _Code Repository Browser: http://reviews.llvm.org/diffusion/ |
Sean Silva | b39f47b | 2012-10-12 01:21:24 +0000 | [diff] [blame] | 137 | .. _Arcanist Quick Start: http://www.phabricator.com/docs/phabricator/article/Arcanist_Quick_Start.html |
| 138 | .. _Arcanist User Guide: http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide.html |