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 | |
| 8 | If you prefer to use a web user interface for code reviews, |
| 9 | you can now submit your patches for Clang and LLVM at |
| 10 | `LLVM's Phabricator`_. |
| 11 | |
| 12 | Sign up |
| 13 | ------- |
| 14 | |
Chandler Carruth | 34e3477 | 2012-10-27 09:47:33 +0000 | [diff] [blame] | 15 | Note that if you use your Subversion user name as Phabricator user name, |
| 16 | Phabricator will automatically connect your submits to your Phabricator user in |
| 17 | the `Code Repository Browser`_. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 18 | |
| 19 | |
| 20 | Requesting a review via the command line |
| 21 | ---------------------------------------- |
| 22 | |
| 23 | Phabricator has a tool called *Arcanist* to upload patches from |
| 24 | the command line. To get you set up, follow the |
| 25 | `Arcanist Quick Start`_ instructions. |
| 26 | |
| 27 | You can learn more about how to use arc to interact with |
| 28 | Phabricator in the `Arcanist User Guide`_. |
| 29 | |
| 30 | Requesting a review via the web interface |
| 31 | ----------------------------------------- |
| 32 | |
| 33 | The tool to create and review patches in Phabricator is called |
| 34 | *Differential*. |
| 35 | |
| 36 | Note that you can upload patches created through various diff tools, |
| 37 | including git and svn. To make reviews easier, please always include |
| 38 | **as much context as possible** with your diff! Don't worry, Phabricator |
| 39 | will automatically send a diff with a smaller context in the review |
| 40 | email, but having the full file in the web interface will help the |
| 41 | reviewer understand your code. |
| 42 | |
| 43 | To get a full diff, use one of the following commands (or just use Arcanist |
| 44 | to upload your patch): |
| 45 | |
Alexey Samsonov | bdb2594 | 2012-11-06 15:04:37 +0000 | [diff] [blame] | 46 | * ``git diff -U999999 other-branch`` |
| 47 | * ``svn diff --diff-cmd=diff -x -U999999`` |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 48 | |
| 49 | To upload a new patch: |
| 50 | |
| 51 | * Click *Differential*. |
Hans Wennborg | ac1d6a0 | 2013-07-25 22:58:31 +0000 | [diff] [blame] | 52 | * Click *Create Diff*. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 53 | * Paste the text diff or upload the patch file. |
| 54 | Note that TODO |
| 55 | * Leave the drop down on *Create a new Revision...* and click *Continue*. |
| 56 | * Enter a descriptive title and summary; add reviewers and mailing |
| 57 | lists that you want to be included in the review. If your patch is |
| 58 | for LLVM, cc llvm-commits; if your patch is for Clang, cc cfe-commits. |
| 59 | * Click *Save*. |
| 60 | |
| 61 | To submit an updated patch: |
| 62 | |
| 63 | * Click *Differential*. |
Sylvestre Ledru | def97f4 | 2013-11-11 14:27:56 +0000 | [diff] [blame] | 64 | * Click *Create Diff*. |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 65 | * Paste the updated diff. |
| 66 | * Select the review you want to from the *Attach To* dropdown and click |
| 67 | *Continue*. |
| 68 | * Click *Save*. |
| 69 | |
| 70 | Reviewing code with Phabricator |
| 71 | ------------------------------- |
| 72 | |
| 73 | Phabricator allows you to add inline comments as well as overall comments |
| 74 | to a revision. To add an inline comment, select the lines of code you want |
| 75 | to comment on by clicking and dragging the line numbers in the diff pane. |
| 76 | |
| 77 | You can add overall comments or submit your comments at the bottom of the page. |
| 78 | |
| 79 | Phabricator has many useful features, for example allowing you to select |
| 80 | diffs between different versions of the patch as it was reviewed in the |
| 81 | *Revision Update History*. Most features are self descriptive - explore, and |
| 82 | if you have a question, drop by on #llvm in IRC to get help. |
| 83 | |
Manuel Klimek | 51e31d2 | 2013-02-13 09:07:18 +0000 | [diff] [blame] | 84 | Note that as e-mail is the system of reference for code reviews, and some |
| 85 | people prefer it over a web interface, we do not generate automated mail |
| 86 | when a review changes state, for example by clicking "Accept Revision" in |
| 87 | the web interface. Thus, please type LGTM into the comment box to accept |
| 88 | a change from Phabricator. |
| 89 | |
Mark Seaborn | 6e15ee8 | 2014-02-11 16:58:03 +0000 | [diff] [blame] | 90 | Committing a change |
| 91 | ------------------- |
| 92 | |
| 93 | When committing an LLVM change that has been reviewed using |
| 94 | Phabricator, the convention is for the commit message to end with the |
| 95 | line: |
| 96 | |
| 97 | :: |
| 98 | |
| 99 | Differential Revision: <URL> |
| 100 | |
| 101 | where ``<URL>`` is the URL for the code review, starting with |
Manuel Klimek | 234b86b | 2014-04-07 10:21:33 +0000 | [diff] [blame^] | 102 | ``http://reviews.llvm.org/``. |
Mark Seaborn | 6e15ee8 | 2014-02-11 16:58:03 +0000 | [diff] [blame] | 103 | |
| 104 | Note that Arcanist will add this automatically. |
| 105 | |
| 106 | This allows people reading the version history to see the review for |
| 107 | context. This also allows Phabricator to detect the commit, close the |
| 108 | review, and add a link from the review to the commit. |
| 109 | |
Manuel Klimek | 8398126 | 2012-10-11 19:40:46 +0000 | [diff] [blame] | 110 | Status |
| 111 | ------ |
| 112 | |
| 113 | Currently, we're testing Phabricator for use with Clang/LLVM. Please let us |
| 114 | know whether you like it and what could be improved! |
Sean Silva | b39f47b | 2012-10-12 01:21:24 +0000 | [diff] [blame] | 115 | |
Manuel Klimek | 234b86b | 2014-04-07 10:21:33 +0000 | [diff] [blame^] | 116 | .. _LLVM's Phabricator: http://reviews.llvm.org |
| 117 | .. _Code Repository Browser: http://reviews.llvm.org/diffusion/ |
Sean Silva | b39f47b | 2012-10-12 01:21:24 +0000 | [diff] [blame] | 118 | .. _Arcanist Quick Start: http://www.phabricator.com/docs/phabricator/article/Arcanist_Quick_Start.html |
| 119 | .. _Arcanist User Guide: http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide.html |