Changeset View
Standalone View
llvm/docs/Phabricator.rst
Show All 13 Lines | |||||
is the system of record for all LLVM code review. The mailing list should be | is the system of record for all LLVM code review. The mailing list should be | ||||
added as a subscriber on all reviews, and Phabricator users should be prepared | added as a subscriber on all reviews, and Phabricator users should be prepared | ||||
to respond to free-form comments in mail sent to the commits list. | to respond to free-form comments in mail sent to the commits list. | ||||
Sign up | Sign up | ||||
------- | ------- | ||||
To get started with Phabricator, navigate to `https://reviews.llvm.org`_ and | To get started with Phabricator, navigate to `https://reviews.llvm.org`_ and | ||||
click the power icon in the top right. You can register with a GitHub account, | click the Log In button in the top right side: | ||||
a Google account, or you can create your own profile. | |||||
.. image:: Phabricator-login.png | |||||
Then Register New Account: | |||||
.. image:: Phabricator-register.png | |||||
Using GitHub, Google or create a Phabricator profile. | |||||
Make *sure* that the email address registered with Phabricator is subscribed | Make *sure* that the email address registered with Phabricator is subscribed | ||||
to the relevant -commits mailing list. If you are not subscribed to the commit | to the relevant -commits mailing list. If you are not subscribed to the commit | ||||
list, all mail sent by Phabricator on your behalf will be held for moderation. | list, all mail sent by Phabricator on your behalf will be held for moderation. | ||||
Note that if you use your Subversion user name as Phabricator user name, | Note that if you use your Subversion user name as Phabricator user name, | ||||
Phabricator will automatically connect your submits to your Phabricator user in | Phabricator will automatically connect your submits to your Phabricator user in | ||||
the `Code Repository Browser`_. | the `Code Repository Browser`_. | ||||
.. _phabricator-request-review-command-line: | |||||
Requesting a review via the command line | Requesting a review via the command line | ||||
---------------------------------------- | ---------------------------------------- | ||||
Phabricator has a tool called *Arcanist* to upload patches from | Phabricator has a tool called *Arcanist* to upload patches from | ||||
the command line. To get you set up, follow the | the command line. To get you set up, follow the | ||||
`Arcanist Quick Start`_ instructions. | `Arcanist Quick Start`_ instructions. | ||||
There are many possible workflows for creating changes, what follows is | |||||
one possibility. | |||||
First create a branch off commit, tag or another branch like master. For a | |||||
new feature this would typically be off the **master**. Then edit/create files, | |||||
compile, test and commit the changes: | |||||
:: | |||||
git checkout -b update-phabricator.rst master | |||||
vi llvm/docs/phabricator.rst | |||||
... | |||||
git commit -a -m "Update Phabricator.rst" | |||||
| | |||||
Before uploading for review you need to find your initial reviewers, | |||||
finding-potential-reviewers_. Then use ``arc diff`` to upload commits to | |||||
Differential, the web based Phabricator code review tool. When ``arc diff`` | |||||
runs it will use your editor, set-local-editor_, so you can supply ``Reviewers`` | |||||
and ``Subscribers`` in your commit message. It requires **one or more** Reviewers | |||||
and allows **zero or more** Subscribers, finding-reviewer-names_. | |||||
lebedev.ri: It really should be emphasized that either the appropriate repo field should be set,
or… | |||||
winksavilleAuthorUnsubmitted Not Done ReplyInline ActionsI'm don't know what you mean by 'appropriate repo field'. I've emphasied there must be one or more Reviewers. Or do you mean something else? winksaville: I'm don't know what you mean by 'appropriate repo field'. I've emphasied there must be **one or… | |||||
winksavilleAuthorUnsubmitted @lebedev.ri do you mean add information about forking of llvm/llvm-project, clone the fork and then creating a commit? winksaville: @lebedev.ri do you mean add information about forking of llvm/llvm-project, clone the fork and… | |||||
lebedev.riUnsubmitted Not Done ReplyInline ActionsI'm only highlighting "The mailing list should be added as a subscriber on all reviews" from https://llvm.org/docs/Phabricator.html lebedev.ri: I'm only highlighting "The mailing list should be added as a subscriber on all reviews" from… | |||||
winksavilleAuthorUnsubmitted Not Done ReplyInline ActionsOk, so instead of: "It requires one or more Reviewers and allows zero or more Subscribers, finding-reviewer-names_." How about: "arc requires one or more Reviewers (finding-reviewer-names_) and Subscribers should be llvm-commits plus the appropriate -dev list(s) from listinfo." I'll update llvm/docs/Phabricator-commit-message.png when I get it right :) winksaville: Ok, so instead of:
"It requires **one or more** Reviewers and allows **zero or more**… | |||||
lebedev.riUnsubmitted Not Done ReplyInline Actions
Hm, not exactly. "arc requires one or more Reviewers (finding-reviewer-names_) and Subscribers should be the appropriate -commits list(s) from listinfo."` lebedev.ri: > and Subscribers should be llvm-commits plus the appropriate -dev list(s) from listinfo."
Hm… | |||||
hintondaUnsubmitted Not Done ReplyInline ActionsAlthough I normally add subscribers out of habit, Herald takes care of this for you when you submit a patch -- believe this is all maintained by @benhamilton. hintonda: Although I normally add subscribers out of habit, Herald takes care of this for you when you… | |||||
lebedev.riUnsubmitted Not Done ReplyInline ActionsThere is a sufficient number of cases where that isn't so, lebedev.ri: There is a sufficient number of cases where that isn't so,
be it either intentionally going… | |||||
winksavilleAuthorUnsubmitted Not Done ReplyInline ActionsThis leads to a different question. I use git gui to amend the commits locally and then arc diff master to update. Do I need to manually update Subscriber with benhamilton so I don't accidentally remove it after @hintonda has added it? winksaville: This leads to a different question.
I use git gui to amend the commits locally and then `arc… | |||||
hintondaUnsubmitted Not Done ReplyInline ActionsNot sure I understand your question -- don't use gui's, but figure they can't be that different. In any case, after you create a new patch in phab, you can update it by simply typeing arc diff. If you want to do it from somewhere other than where you originally ran the initial arc diff, you need to use the --update option, e.g., arc diff --update D61267. You don't need to do anything else. For example, look at the first few lines of history at the top of this page. You'll see that Herald add the llvm project, and subscribed llvm-commits. It's a one-time thing you don't need to worry about. As for @lebedev.ri comment about it not always working, personally, I'd consider that a bug and raise it on the list. I think @benhamilton fixed a few of those for me a couple of years ago. However, it doesn't hurt to mention it in the docs. hth... don hintonda: Not sure I understand your question -- don't use gui's, but figure they can't be that different. | |||||
winksavilleAuthorUnsubmitted It seems adding "xxx-commit" doesn't hurt and its nice Hearld will do the right thing. I went ahead and updated the text and Phabricator-commit-message.png image. If you guys would like something different, let me know. winksaville: It seems adding "xxx-commit" doesn't hurt and its nice Hearld will do the right thing. I went… | |||||
benhamiltonUnsubmitted Not Done ReplyInline ActionsThe repository field is set automatically based on the closest .arcconfig file going up from the directory in which you ran arc diff. benhamilton: The repository field is set automatically based on the closest `.arcconfig` file going up from… | |||||
benhamiltonUnsubmitted Not Done ReplyInline Actions(I think there are scenarios where someone can run arc diff from a different directory or upload a diff directly to Phabricator which can bypass its detection of the repository, which could explain @lebedev.ri 's issues. Regardless, directly reporting problems can help.) benhamilton: (I think there are scenarios where someone can run `arc diff` from a different directory or… | |||||
winksavilleAuthorUnsubmitted So is there any more changes anyone would like me to make? winksaville: So is there any more changes anyone would like me to make? | |||||
Add additional information to help beginners submit changes for review. | |||||
:: | |||||
arc diff | |||||
You can learn more about how to use arc to interact with | You can learn more about how to use arc to interact with | ||||
Phabricator in the `Arcanist User Guide`_. | Phabricator in the `Arcanist User Guide`_. | ||||
.. _phabricator-request-review-web: | .. _phabricator-request-review-web: | ||||
Requesting a review via the web interface | Requesting a review via the web interface | ||||
----------------------------------------- | ----------------------------------------- | ||||
The tool to create and review patches in Phabricator is called | The tool to create and review patches in Phabricator is called | ||||
*Differential*. | *Differential*. | ||||
Note that you can upload patches created through various diff tools, | Note that you can upload patches created through various diff tools, | ||||
including git and svn. To make reviews easier, please always include | including git and svn. To make reviews easier, please always include | ||||
**as much context as possible** with your diff! Don't worry, Phabricator | **as much context as possible** with your diff! Don't worry, Phabricator | ||||
will automatically send a diff with a smaller context in the review | will automatically send a diff with a smaller context in the review | ||||
email, but having the full file in the web interface will help the | email, but having the full file in the web interface will help the | ||||
reviewer understand your code. | reviewer understand your code. | ||||
To get a full diff, use one of the following commands (or just use Arcanist | To get a full diff, use one of the following commands (or just use Arcanist | ||||
to upload your patch): | to upload your patch): | ||||
* ``git show HEAD -U999999 > mypatch.patch`` | * ``git show HEAD -U999999 > mypatch.patch`` | ||||
Not Done ReplyInline ActionsI think it would be helpful to include the subscribers info @benhamilton provided. hintonda: I think it would be helpful to include the subscribers info @benhamilton provided. | |||||
Not Done ReplyInline ActionsI'd add one more sentence here, to tell people that in the case that they are doing the currently-recommend thing, they do _NOT_ need to worry about this. Something like: LGTM otherwise! jyknight: I'd add one more sentence here, to tell people that in the case that they are doing the… | |||||
* ``git format-patch -U999999 @{u}`` | * ``git format-patch -U999999 @{u}`` | ||||
* ``svn diff --diff-cmd=diff -x -U999999`` | * ``svn diff --diff-cmd=diff -x -U999999`` | ||||
To upload a new patch: | To upload a new patch: | ||||
* Click *Differential*. | * Click *Differential*. | ||||
* Click *+ Create Diff*. | * Click *+ Create Diff*. | ||||
* Paste the text diff or browse to the patch file. Click *Create Diff*. | * Paste the text diff or browse to the patch file. Click *Create Diff*. | ||||
Show All 29 Lines | |||||
.. _finding-potential-reviewers: | .. _finding-potential-reviewers: | ||||
Finding potential reviewers | Finding potential reviewers | ||||
--------------------------- | --------------------------- | ||||
Here are a couple of ways to pick the initial reviewer(s): | Here are a couple of ways to pick the initial reviewer(s): | ||||
* Use ``svn blame`` and the commit log to find names of people who have | * Use ``git blame`` and the commit log to find names of people who have | ||||
Not Done ReplyInline ActionsYou could also mention arc cover here. hintonda: You could also mention `arc cover` here. | |||||
I had it previously, but then felt it needed more explanation, so was lazy and removed it. But it does reasonable information, I'll add it back with an example section. winksaville: I had it previously, but then felt it needed more explanation, so was lazy and removed it. But… | |||||
recently modified the same area of code that you are modifying. | recently modified the same area of code that you are modifying. | ||||
* Look in CODE_OWNERS.TXT to see who might be responsible for that area. | * Look in CODE_OWNERS.TXT to see who might be responsible for that area. | ||||
* If you've discussed the change on a dev list, the people who participated | * If you've discussed the change on a dev list, the people who participated | ||||
might be appropriate reviewers. | might be appropriate reviewers. | ||||
Even if you think the code owner is the busiest person in the world, it's still | Even if you think the code owner is the busiest person in the world, it's still | ||||
okay to put them as a reviewer. Being the code owner means they have accepted | okay to put them as a reviewer. Being the code owner means they have accepted | ||||
responsibility for making sure the review happens. | responsibility for making sure the review happens. | ||||
.. _finding-reviewer-names: | |||||
Finding Reviewers Names | |||||
----------------------- | |||||
The names for Reviewers and Subscribers is the registered Phabricator name for | |||||
the individual. Type the persons "real name" or a portion of the persons "real name" or | |||||
"user name" as known by Phabricator. Hopefully this is related to the information | |||||
in the history or provided by the arc core utility. | |||||
.. image:: Phabricator-search.png | |||||
.. _set-local-editor: | |||||
Set Local Editor | |||||
---------------- | |||||
The names for Reviewers and Subscribers is the registered Phabricator name for | |||||
Not Done ReplyInline ActionsCut-n-paste error? hintonda: Cut-n-paste error? | |||||
Yes :) winksaville: Yes :) | |||||
the individual. Type the persons "real name" or a portion of the persons "real name" or | |||||
"user name" as known by Phabricator. Hopefully this is related to the information | |||||
in the history or provided by the arc core utility. | |||||
Reviewing code with Phabricator | Reviewing code with Phabricator | ||||
------------------------------- | ------------------------------- | ||||
Phabricator allows you to add inline comments as well as overall comments | Phabricator allows you to add inline comments as well as overall comments | ||||
to a revision. To add an inline comment, select the lines of code you want | to a revision. To add an inline comment, select the lines of code you want | ||||
to comment on by clicking and dragging the line numbers in the diff pane. | to comment on by clicking and dragging the line numbers in the diff pane. | ||||
When you have added all your comments, scroll to the bottom of the page and | When you have added all your comments, scroll to the bottom of the page and | ||||
click the Submit button. | click the Submit button. | ||||
▲ Show 20 Lines • Show All 126 Lines • Show Last 20 Lines |
It really should be emphasized that either the appropriate repo field should be set,
or appropriate -commits list be subscribed, from the start.