Skip to content

Commit

Permalink
Clarify and simplify some sections.
Browse files Browse the repository at this point in the history
Also standardize on two spaces following a sentence.
  • Loading branch information
Mark Cote committed Aug 29, 2017
1 parent d088c7a commit a25303b
Showing 1 changed file with 52 additions and 50 deletions.
102 changes: 52 additions & 50 deletions phabricator-user.rst
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ Differential revision being created or updated). There is a short
<https://secure.phabricator.com/book/phabricator/article/herald/>`_
available.

.. _quick-start:

***********
Quick Start
***********
Expand Down Expand Up @@ -231,10 +233,9 @@ Going to the revision's URL will show the change in the activity log.
There will also be new entries in the "History" and "Commits" tabs in
the "Revision Contents" table. You can use the History tab to switch
between various diff views: the current patch, the patch at a
particular point in history, and the changes between different commits
(effectively an interdiff of the patch history). Here are the changes
between the first and second commit ("Diff 1" and "Diff 2" in
Phabricator language):
particular point in history, and the changes between different
commits, i.e., an interdiff. Here are the changes between the first
and second commit ("Diff 1" and "Diff 2" in Phabricator language):

.. image:: images/interdiff.png
:align: center
Expand Down Expand Up @@ -267,17 +268,16 @@ ability to see the different patches and differences between them.
Series of Commits
-----------------

It is possible to chain a series of revisions together in Differential,
although it is currently a manual process. Each revision can have one
or more parents and one or more children. This feature can be used to
represent a stack of commits to split up a complicated patch, which is
a good practice to make testing and review easier.
It is possible to chain a series of revisions together in
Differential, although it is currently a manual process. This feature
can be used to represent a stack of commits to split up a complicated
patch, which is a good practice to make testing and reviewing easier.

To use this pattern, you will need to specify the exact commit you
want to send to Differential, since the default is to send all your
draft commits to a single revision, i.e. the :ref:`fix-up-commits`
draft commits to a single revision, i.e., the :ref:`fix-up-commits`
method, which is not what we want here. To send the currently
checked-out commit, run ``arc diff .^``.
checked-out Mercurial commit, run ``arc diff .^``.

To set the parent-child relationship, go to your first commit, choose
"Edit Related Revisions..." from the right-hand menu, then "Edit Child
Expand All @@ -302,6 +302,9 @@ all the stacked commits together without applying the commits
locally. Also, when you update any commits, you'll need to run ``arc
diff .^`` for each child commit as well.

We will be working on a solution to automate the submission and
updating of commit series.

Reviewing Patches
=================

Expand Down Expand Up @@ -330,7 +333,7 @@ Leaving Reviews
Performing a review involves two steps, both of which are technically
optional but will usually be used together:

1. Leaving comments on the diff or on the revision generally.
1. Leaving comments on the diff and/or on the revision generally.
2. Choosing an action to indicate the next step for the author.

Leaving comments is fairly straightforward. For inline diff comments,
Expand Down Expand Up @@ -367,7 +370,7 @@ Mozilla's Phabricator instance is a stock installation, with a small patch
applied, and some custom extensions. The patch and extensions are
intentionally small in scope and are limited to supporting integration
points with `bugzilla.mozilla.org <https://bugzilla.mozilla.org>`_
(henceforth referred to as "BMO").
("BMO").

We are using various GitHub repos for our code: the
`deployment scripts and config <https://github.com/mozilla-services/mozphab>`_
Expand All @@ -392,26 +395,27 @@ applications for Mozilla's use case. In addition to Differential and
Herald, described above, we support or are trialing several other
applications and utilities:

* Dashboards allow users to set up custom pages to display useful
* **Dashboards** allow users to set up custom pages to display useful
information, for example assigned reviews. It seems somewhat
limited, though, so we'll evaluate how useful it really is.

* Pholio is an application for reviewing mock-ups and designs.
* **Pholio** is an application for reviewing mock-ups and designs.
Mozilla doesn't have a central application for this, so we'd like
your input on whether Pholio is useful.

* Badges, macros, and tokens: These are mostly bits of whimsy that
might enhance user experience by providing some levity. If they're
fun, or at least harmless, we'll leave them; if they become annoying
or distracting, we may remove them.
* **Badges**, **macros**, and **tokens**: These are mostly bits of
whimsy that might enhance user experience by providing some levity.
If they're fun, or at least harmless, we'll leave them; if they
become annoying or distracting, we may remove them.

Note that Phabricator also has a post-commit review system called
Audit. This application is mandatory, that is, it cannot be
disabled. However, at the moment Mozilla has no processes for
post-commit review of Firefox and related code, so we do not recommend
its use, at least until such time as a process is deemed necessary and
implemented. Audit may, of course, be useful to projects hosted on
the Mozilla Phabricator instance outside of Firefox.
**Audit**. This application is mandatory, that is, it cannot be
disabled in a Phabricator installation. However, at the moment
Mozilla has no defined engineering processes for post-commit review of
Firefox and related code, so we do not recommend its use, at least
until such time as a process is deemed necessary and implemented.
Audit may, of course, be useful to projects hosted on the Mozilla
Phabricator instance outside of Firefox.

.. _bmo-integration:

Expand All @@ -423,48 +427,46 @@ Since issue tracking and code review are tightly related, and since
BMO is currently the authority for identity and authorization around
both issue tracking and code review, including security and other
confidential bugs and fixes, our Phabricator instance is integrated
with BMO. This integration is intentionally lightweight in order
to limit customization of Phabricator, which has both maintenance and
opportunity costs, consisting of identity, authorization, links
with BMO. This integration is intentionally lightweight in order to
limit customization of Phabricator, which has both maintenance and
opportunity costs. It consists of identity, authorization, links
between bugs and revisions, and basic review-status mirroring.

Identity
========

The main way to log into Phabricator is via BMO's auth delegation. A
user logging into Phabricator is taken to BMO to log in as usual and
will be redirected back to Phabricator if the login succeeds. If this
is the first time the user has logged into Phabricator, they will be
prompted to create an account. They can choose to use their BMO email
address or provide a new one, which will be separately verified. New
users will also be prompted to enter a separate username, unlike
BMO. This username will be used by Autoland to denote reviewers when
constructing the final commit message.
As described in the :ref:`quick-start` guide, the main way to log into
Phabricator is via BMO's auth delegation. A user logging into
Phabricator is taken to BMO to log in as usual and will be redirected
back to Phabricator if the login succeeds. If this is the first time
the user has logged into Phabricator, they will be prompted to create
an account. New users will also be prompted to enter a separate
username, unlike BMO.

Authorization
=============

If a bug has one or more security groups applied to it, that is, it
has restricted visibility, any Differential revisions associated with
it are similarly restricted in visibility. This will initially only
it are similarly restricted in visibility. This will initially only
apply to Firefox security groups, that is, groups with names matching
``*core-security*``. Any revision associated with a bug restricted via
other groups, e.g. infra, is visible only to the author and admins. We
can add proper support for such groups on request.
``*core-security*``. Any revision associated with a bug restricted
via other groups, e.g. infra, is visible only to the author and
admins. We can add proper support for such groups on request.

Links from Differential to BMO
==============================

A bug number must be entered when a patch is submitted to
Phabricator. This is stored in the revision metadata and provided in
the UI as a link to the associated bug on BMO.
A bug number must be entered when a patch is submitted to Phabricator.
This is stored in the revision metadata and provided in the UI as a
link to the associated bug on BMO.

Links from BMO to Differential
==============================

Upon the creation of a new revision in Differential, a stub
attachment, containing only the URL of the revision, is added to the
associated bug. Based on the attachment type, BMO automatically
associated bug. Based on the attachment type, BMO automatically
redirects to Differential if the attachment link is clicked.

Review flags
Expand All @@ -473,9 +475,9 @@ Review flags
For simplicity, and since Differential's review system does not map
cleanly to BMO's review flags, r+ flags, and only r+ flags, are set on
the stub attachment associated with a Differential revision when a
Phabricator user performs an "Accept Revision" action. The flag is
Phabricator user performs an "Accept Revision" action. The flag is
removed if the reviewer later issues a "Request Changes" or a "Resign
as Reviewer" action. Similarly, all r+ flags are removed if the author
selects any of the "Plan Changes", "Request Review", or "Abandon
Revision" actions. In the last case, the stub attachment is also be
obsoleted.
as Reviewer" action. Similarly, all r+ flags are removed if the
author selects any of the "Plan Changes", "Request Review", or
"Abandon Revision" actions. In the last case, the stub attachment is
also be obsoleted.

0 comments on commit a25303b

Please sign in to comment.