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 | |||||
Use GitHub, Google or create a Phabricator profile by clicking on appropriate | |||||
button above. | |||||
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. | ||||
You can learn more about how to use arc to interact with | There are many possible workflows for creating changes, what follows is | ||||
Phabricator in the `Arcanist User Guide`_. | 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… | |||||
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… | |||||
@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… | |||||
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… | |||||
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**… | |||||
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… | |||||
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… | |||||
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… | |||||
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… | |||||
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. | |||||
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… | |||||
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… | |||||
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… | |||||
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? | |||||
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… | |||||
:: | |||||
arc diff master | |||||
| | |||||
Here is the editor running allowing you to edit your commit message. A | |||||
summary message and reviewer has been added and its ready to be written. | |||||
.. image:: Phabricator-commit-message.png | |||||
| | |||||
After writing and quitting the editor you'll see a summary of what ``arc`` did: | |||||
.. image:: Phabricator-done.png | |||||
| | |||||
You can now navigate to the `Differential revision` at | |||||
`<https://reviews.llvm.org/D61267>`_. | |||||
.. image:: Phabricator-D61267.png | |||||
| | |||||
See `Arcanist diff <https://secure.phabricator.com/book/phabricator/article/arcanist\_diff>`_ | |||||
documentation and more about interacting with 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*. | ||||
▲ Show 20 Lines • Show All 49 Lines • ▼ Show 20 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.