|
147
|
1 .. _phabricator-reviews:
|
|
|
2
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
3 =============================
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
4 Code Reviews with Phabricator
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
5 =============================
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
6
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
7 .. contents::
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
8 :local:
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
9
|
|
77
|
10 If you prefer to use a web user interface for code reviews, you can now submit
|
|
|
11 your patches for Clang and LLVM at `LLVM's Phabricator`_ instance.
|
|
|
12
|
|
|
13 While Phabricator is a useful tool for some, the relevant -commits mailing list
|
|
|
14 is the system of record for all LLVM code review. The mailing list should be
|
|
|
15 added as a subscriber on all reviews, and Phabricator users should be prepared
|
|
|
16 to respond to free-form comments in mail sent to the commits list.
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
17
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
18 Sign up
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
19 -------
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
20
|
|
147
|
21 To get started with Phabricator, navigate to `https://reviews.llvm.org`_ and
|
|
77
|
22 click the power icon in the top right. You can register with a GitHub account,
|
|
|
23 a Google account, or you can create your own profile.
|
|
|
24
|
|
|
25 Make *sure* that the email address registered with Phabricator is subscribed
|
|
95
|
26 to the relevant -commits mailing list. If you are not subscribed to the commit
|
|
77
|
27 list, all mail sent by Phabricator on your behalf will be held for moderation.
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
28
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
29 Note that if you use your Subversion user name as Phabricator user name,
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
30 Phabricator will automatically connect your submits to your Phabricator user in
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
31 the `Code Repository Browser`_.
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
32
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
33 Requesting a review via the command line
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
34 ----------------------------------------
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
35
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
36 Phabricator has a tool called *Arcanist* to upload patches from
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
37 the command line. To get you set up, follow the
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
38 `Arcanist Quick Start`_ instructions.
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
39
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
40 You can learn more about how to use arc to interact with
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
41 Phabricator in the `Arcanist User Guide`_.
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
42
|
|
134
|
43 .. _phabricator-request-review-web:
|
|
|
44
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
45 Requesting a review via the web interface
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
46 -----------------------------------------
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
47
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
48 The tool to create and review patches in Phabricator is called
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
49 *Differential*.
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
50
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
51 Note that you can upload patches created through various diff tools,
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
52 including git and svn. To make reviews easier, please always include
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
53 **as much context as possible** with your diff! Don't worry, Phabricator
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
54 will automatically send a diff with a smaller context in the review
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
55 email, but having the full file in the web interface will help the
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
56 reviewer understand your code.
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
57
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
58 To get a full diff, use one of the following commands (or just use Arcanist
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
59 to upload your patch):
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
60
|
|
121
|
61 * ``git show HEAD -U999999 > mypatch.patch``
|
|
|
62 * ``git format-patch -U999999 @{u}``
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
63 * ``svn diff --diff-cmd=diff -x -U999999``
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
64
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
65 To upload a new patch:
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
66
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
67 * Click *Differential*.
|
|
95
|
68 * Click *+ Create Diff*.
|
|
|
69 * Paste the text diff or browse to the patch file. Click *Create Diff*.
|
|
134
|
70 * Leave this first Repository field blank. (We'll fill in the Repository
|
|
|
71 later, when sending the review.)
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
72 * Leave the drop down on *Create a new Revision...* and click *Continue*.
|
|
95
|
73 * Enter a descriptive title and summary. The title and summary are usually
|
|
|
74 in the form of a :ref:`commit message <commit messages>`.
|
|
134
|
75 * Add reviewers (see below for advice). (If you set the Repository field
|
|
|
76 correctly, llvm-commits or cfe-commits will be subscribed automatically;
|
|
|
77 otherwise, you will have to manually subscribe them.)
|
|
|
78 * In the Repository field, enter the name of the project (LLVM, Clang,
|
|
|
79 etc.) to which the review should be sent.
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
80 * Click *Save*.
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
81
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
82 To submit an updated patch:
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
83
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
84 * Click *Differential*.
|
|
95
|
85 * Click *+ Create Diff*.
|
|
|
86 * Paste the updated diff or browse to the updated patch file. Click *Create Diff*.
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
87 * Select the review you want to from the *Attach To* dropdown and click
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
88 *Continue*.
|
|
134
|
89 * Leave the Repository field blank. (We previously filled out the Repository
|
|
|
90 for the review request.)
|
|
95
|
91 * Add comments about the changes in the new diff. Click *Save*.
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
92
|
|
100
|
93 Choosing reviewers: You typically pick one or two people as initial reviewers.
|
|
|
94 This choice is not crucial, because you are merely suggesting and not requiring
|
|
|
95 them to participate. Many people will see the email notification on cfe-commits
|
|
|
96 or llvm-commits, and if the subject line suggests the patch is something they
|
|
|
97 should look at, they will.
|
|
|
98
|
|
147
|
99
|
|
|
100 .. _finding-potential-reviewers:
|
|
|
101
|
|
|
102 Finding potential reviewers
|
|
|
103 ---------------------------
|
|
|
104
|
|
100
|
105 Here are a couple of ways to pick the initial reviewer(s):
|
|
|
106
|
|
|
107 * Use ``svn blame`` and the commit log to find names of people who have
|
|
|
108 recently modified the same area of code that you are modifying.
|
|
|
109 * Look in CODE_OWNERS.TXT to see who might be responsible for that area.
|
|
|
110 * If you've discussed the change on a dev list, the people who participated
|
|
|
111 might be appropriate reviewers.
|
|
|
112
|
|
|
113 Even if you think the code owner is the busiest person in the world, it's still
|
|
|
114 okay to put them as a reviewer. Being the code owner means they have accepted
|
|
|
115 responsibility for making sure the review happens.
|
|
|
116
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
117 Reviewing code with Phabricator
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
118 -------------------------------
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
119
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
120 Phabricator allows you to add inline comments as well as overall comments
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
121 to a revision. To add an inline comment, select the lines of code you want
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
122 to comment on by clicking and dragging the line numbers in the diff pane.
|
|
95
|
123 When you have added all your comments, scroll to the bottom of the page and
|
|
|
124 click the Submit button.
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
125
|
|
95
|
126 You can add overall comments in the text box at the bottom of the page.
|
|
|
127 When you're done, click the Submit button.
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
128
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
129 Phabricator has many useful features, for example allowing you to select
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
130 diffs between different versions of the patch as it was reviewed in the
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
131 *Revision Update History*. Most features are self descriptive - explore, and
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
132 if you have a question, drop by on #llvm in IRC to get help.
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
133
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
134 Note that as e-mail is the system of reference for code reviews, and some
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
135 people prefer it over a web interface, we do not generate automated mail
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
136 when a review changes state, for example by clicking "Accept Revision" in
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
137 the web interface. Thus, please type LGTM into the comment box to accept
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
138 a change from Phabricator.
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
139
|
|
77
|
140 Committing a change
|
|
|
141 -------------------
|
|
|
142
|
|
100
|
143 Once a patch has been reviewed and approved on Phabricator it can then be
|
|
121
|
144 committed to trunk. If you do not have commit access, someone has to
|
|
|
145 commit the change for you (with attribution). It is sufficient to add
|
|
|
146 a comment to the approved review indicating you cannot commit the patch
|
|
|
147 yourself. If you have commit access, there are multiple workflows to commit the
|
|
|
148 change. Whichever method you follow it is recommended that your commit message
|
|
|
149 ends with the line:
|
|
77
|
150
|
|
|
151 ::
|
|
|
152
|
|
|
153 Differential Revision: <URL>
|
|
|
154
|
|
|
155 where ``<URL>`` is the URL for the code review, starting with
|
|
147
|
156 ``https://reviews.llvm.org/``.
|
|
77
|
157
|
|
100
|
158 This allows people reading the version history to see the review for
|
|
|
159 context. This also allows Phabricator to detect the commit, close the
|
|
|
160 review, and add a link from the review to the commit.
|
|
|
161
|
|
|
162 Note that if you use the Arcanist tool the ``Differential Revision`` line will
|
|
|
163 be added automatically. If you don't want to use Arcanist, you can add the
|
|
|
164 ``Differential Revision`` line (as the last line) to the commit message
|
|
|
165 yourself.
|
|
|
166
|
|
147
|
167 Using the Arcanist tool can simplify the process of committing reviewed code as
|
|
|
168 it will retrieve reviewers, the ``Differential Revision``, etc from the review
|
|
|
169 and place it in the commit message. You may also commit an accepted change
|
|
|
170 directly using ``git llvm push``, per the section in the :ref:`getting started
|
|
|
171 guide <commit_from_git>`.
|
|
100
|
172
|
|
|
173 Note that if you commit the change without using Arcanist and forget to add the
|
|
|
174 ``Differential Revision`` line to your commit message then it is recommended
|
|
|
175 that you close the review manually. In the web UI, under "Leap Into Action" put
|
|
|
176 the SVN revision number in the Comment, set the Action to "Close Revision" and
|
|
|
177 click Submit. Note the review must have been Accepted first.
|
|
|
178
|
|
|
179
|
|
147
|
180 Committing someone's change from Phabricator
|
|
|
181 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
|
100
|
182
|
|
|
183 On a clean Git repository on an up to date ``master`` branch run the
|
|
|
184 following (where ``<Revision>`` is the Phabricator review number):
|
|
|
185
|
|
|
186 ::
|
|
|
187
|
|
|
188 arc patch D<Revision>
|
|
|
189
|
|
|
190
|
|
|
191 This will create a new branch called ``arcpatch-D<Revision>`` based on the
|
|
|
192 current ``master`` and will create a commit corresponding to ``D<Revision>`` with a
|
|
|
193 commit message derived from information in the Phabricator review.
|
|
|
194
|
|
147
|
195 Check you are happy with the commit message and amend it if necessary. Then,
|
|
|
196 make sure the commit is up-to-date, and commit it. This can be done by running
|
|
|
197 the following:
|
|
100
|
198
|
|
|
199 ::
|
|
|
200
|
|
147
|
201 git pull --rebase origin master
|
|
|
202 git show # Ensure the patch looks correct.
|
|
|
203 ninja check-$whatever # Rerun the appropriate tests if needed.
|
|
|
204 git llvm push
|
|
|
205
|
|
|
206 Subversion and Arcanist (deprecated)
|
|
|
207 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
|
100
|
208
|
|
147
|
209 To download a change from Phabricator and commit it with subversion, you should
|
|
|
210 first make sure you have a clean working directory. Then run the following
|
|
|
211 (where ``<Revision>`` is the Phabricator review number):
|
|
100
|
212
|
|
147
|
213 ::
|
|
|
214
|
|
|
215 arc patch D<Revision>
|
|
|
216 arc commit --revision D<Revision>
|
|
|
217
|
|
|
218 The first command will take the latest version of the reviewed patch and apply
|
|
|
219 it to the working copy. The second command will commit this revision to trunk.
|
|
77
|
220
|
|
95
|
221 Abandoning a change
|
|
|
222 -------------------
|
|
|
223
|
|
|
224 If you decide you should not commit the patch, you should explicitly abandon
|
|
|
225 the review so that reviewers don't think it is still open. In the web UI,
|
|
|
226 scroll to the bottom of the page where normally you would enter an overall
|
|
|
227 comment. In the drop-down Action list, which defaults to "Comment," you should
|
|
|
228 select "Abandon Revision" and then enter a comment explaining why. Click the
|
|
|
229 Submit button to finish closing the review.
|
|
|
230
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
231 Status
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
232 ------
|
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
233
|
|
95
|
234 Please let us know whether you like it and what could be improved! We're still
|
|
|
235 working on setting up a bug tracker, but you can email klimek-at-google-dot-com
|
|
|
236 and chandlerc-at-gmail-dot-com and CC the llvm-dev mailing list with questions
|
|
|
237 until then. We also could use help implementing improvements. This sadly is
|
|
|
238 really painful and hard because the Phabricator codebase is in PHP and not as
|
|
|
239 testable as you might like. However, we've put exactly what we're deploying up
|
|
|
240 on an `llvm-reviews GitHub project`_ where folks can hack on it and post pull
|
|
|
241 requests. We're looking into what the right long-term hosting for this is, but
|
|
|
242 note that it is a derivative of an existing open source project, and so not
|
|
|
243 trivially a good fit for an official LLVM project.
|
0
Kaito Tokumori <e105711@ie.u-ryukyu.ac.jp>
parents:
diff
changeset
|
244
|
|
147
|
245 .. _LLVM's Phabricator: https://reviews.llvm.org
|
|
|
246 .. _`https://reviews.llvm.org`: https://reviews.llvm.org
|
|
|
247 .. _Code Repository Browser: https://reviews.llvm.org/diffusion/
|
|
95
|
248 .. _Arcanist Quick Start: https://secure.phabricator.com/book/phabricator/article/arcanist_quick_start/
|
|
|
249 .. _Arcanist User Guide: https://secure.phabricator.com/book/phabricator/article/arcanist/
|
|
|
250 .. _llvm-reviews GitHub project: https://github.com/r4nt/llvm-reviews/
|
