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