Update Phabricator.rst

[winksaville]

Summary

Add additional information to help beginners submit reviews.

[lebedev.ri]

llvm/docs/Phabricator.rst

70–71

It really should be emphasized that either the appropriate repo field should be set,
or appropriate -commits list be subscribed, from the start.

[winksaville]

Update with additional information

[winksaville]

llvm/docs/Phabricator.rst

70–71

I'm don't know what you mean by 'appropriate repo field'. I've emphasied there must be one or more Reviewers.
I could explicitly say that Differential is automatically adding "llvm-commits" to Subscribers.

Or do you mean something else?

[winksaville]

llvm/docs/Phabricator.rst

70–71

@lebedev.ri do you mean add information about forking of llvm/llvm-project, clone the fork and then creating a commit?

[lebedev.ri]

llvm/docs/Phabricator.rst

70–71

I'm only highlighting "The mailing list should be added as a subscriber on all reviews" from https://llvm.org/docs/Phabricator.html
I think that a similar phrase really should be repeated here.

[winksaville]

llvm/docs/Phabricator.rst

70–71

Ok, 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 :)

[lebedev.ri]

llvm/docs/Phabricator.rst

70–71

and Subscribers should be llvm-commits plus the appropriate -dev list(s) from listinfo."

Hm, not exactly.

"arc requires one or more Reviewers (finding-reviewer-names_) and Subscribers should be the appropriate -commits list(s) from listinfo."`

[hintonda]

llvm/docs/Phabricator.rst

70–71

Although 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.

[lebedev.ri]

llvm/docs/Phabricator.rst

70–71

There is a sufficient number of cases where that isn't so,
be it either intentionally going against the docs,
or just confusing the phabricator from auto-subscribing.

[winksaville]

llvm/docs/Phabricator.rst

70–71

This 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?

[hintonda]

llvm/docs/Phabricator.rst

70–71

Not 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

[winksaville]

Apply changes from review comments

[hintonda]

Btw, thanks again for doing this. I'll try take a closer look tonight.

[winksaville]

llvm/docs/Phabricator.rst

70–71

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.

[benhamilton]

llvm/docs/Phabricator.rst

70–71

The repository field is set automatically based on the closest .arcconfig file going up from the directory in which you ran arc diff.

[benhamilton]

llvm/docs/Phabricator.rst

70–71

(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.)

[winksaville]

llvm/docs/Phabricator.rst

70–71

So is there any more changes anyone would like me to make?

[hintonda]

I've added a few reviewers more familiar with this documentation.

My main concern is the addition of images. None of the other documentation has any, and I'm not sure about the policy, so I'll leave that question to others.

However, I have made a couple comments below.

llvm/docs/Phabricator.rst

71

I think it would be helpful to include the subscribers info @benhamilton provided.

168

You could also mention arc cover here.

197

Cut-n-paste error?

[winksaville]

Changes suggested by Don Hinton

[winksaville]

llvm/docs/Phabricator.rst

168

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.

197

Yes :)

[winksaville]

Are there any other edits people would like?

[jyknight]

llvm/docs/Phabricator.rst

71

I'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:
"If you are working off a clone of the official https://github.com/llvm/llvm-project/ repository, Phabricator will subscribe the correct lists automatically. Otherwise, please add the relevant ones yourself."

LGTM otherwise!

[winksaville]

Update as suggested by jyknight

Add an additional sentence to the "Before uploading ..." paragraph
so clarify that Phabicator can update Subscribers automatically.

[winksaville]

I believe this is ready for merging, is there something more I need to do?

[winksaville]

I think this is ready to be commit, if additional changes are needed let me know.

llvm/docs/Phabricator.rst

 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
 to respond to free-form comments in mail sent to the commits list.
 
 Sign up
 -------
 
 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,
-a Google account, or you can create your own profile.
+click the Log In button in the top right side:
+
+.. 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
 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.
 
 Note that if you use your Subversion user name as Phabricator user name,
 Phabricator will automatically connect your submits to your Phabricator user in
 the `Code Repository Browser`_.
 
+.. _phabricator-request-review-command-line:
+
 Requesting a review via the command line
 ----------------------------------------
 
 Phabricator has a tool called *Arcanist* to upload patches from
 the command line. To get you set up, follow the
 `Arcanist Quick Start`_ instructions.
 
-You can learn more about how to use arc to interact with
-Phabricator in the `Arcanist User Guide`_.
+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. ``arc`` requires one or more
+Reviewers (finding-reviewer-names_) and Subscribers should be the appropriate
+**-commits** list(s) from `listinfo <https://lists.llvm.org/mailman/listinfo>`_.
+
+::
+
+  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:
 
 Requesting a review via the web interface
 -----------------------------------------
 
 The tool to create and review patches in Phabricator is called
 *Differential*.

 
 .. _finding-potential-reviewers:
 
 Finding potential reviewers
 ---------------------------
 
 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
   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.
 * If you've discussed the change on a dev list, the people who participated
   might be appropriate reviewers.
 
 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
 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
+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
 -------------------------------
 
 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 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
 click the Submit button.