Improve the documentation on committing code reviewed on Phabricator to trunk.

[delcypher]

Summary

[docs] Improve the documentation on committing code reviewed on
Phabricator to trunk.

The previous documentation had a few issues:

• It did not make it explicit that code could be

committed without using the Arcanist tool and how this should be done.

• There was also an implicit assumption on using Subversion

rather than git-svn in the example using Arcanist. The documentation now
explicitly mentions both cases and details how to commit to trunk in
each case.

[probinson]

Thanks for describing the git-svn sequence. Some inline comments.

docs/Phabricator.rst

132 ↗

(On Diff #43714)

"Whichever" is one word.

146 ↗

(On Diff #43714)

Remove the last "this" on the line.

148 ↗

(On Diff #43714)

I'd write that last sentence in a more active-voice style: "If you don't want to use Arcanist, you will need to add the Differential Revision line to the commit message yourself."

[probinson]

Did you also want to add a pointer to the Phab page on the markup language? It isn't necessarily obvious that Phabricator is actually a separate project with its own documentation; the code-review pages don't have any "help" links for example. (The only docs I was ever able to find were pretty generic "code reviews are good for you" pages, never did find any UI help of any kind.)

[nwilson]

docs/Phabricator.rst

146 ↗

(On Diff #43714)

Maybe replace the first this with something like hyperlinked commit line since that's what we're talking about and is being referenced in the previous paragraph.

[delcypher]

• Address probinson comments
• Address nwilson comments
• Introduce how arcanist can help commit code in a slightly more verbose way.

[delcypher]

Mark comments as done.

[delcypher]

In D15801#317794, @probinson wrote:

Did you also want to add a pointer to the Phab page on the markup language? It isn't necessarily obvious that Phabricator is actually a separate project with its own documentation; the code-review pages don't have any "help" links for example. (The only docs I was ever able to find were pretty generic "code reviews are good for you" pages, never did find any UI help of any kind.)

This is out of scope for what this commit is trying to address but I could certainly address it in a later commit. There doesn't seem to be a relevant section for this sort of information. Perhaps there should be a Tips on usiing Phabricator section or something?

[probinson]

In D15801#317952, @delcypher wrote:

In D15801#317794, @probinson wrote:

Did you also want to add a pointer to the Phab page on the markup language? It isn't necessarily obvious that Phabricator is actually a separate project with its own documentation; the code-review pages don't have any "help" links for example. (The only docs I was ever able to find were pretty generic "code reviews are good for you" pages, never did find any UI help of any kind.)

This is out of scope for what this commit is trying to address but I could certainly address it in a later commit. There doesn't seem to be a relevant section for this sort of information. Perhaps there should be a Tips on usiing Phabricator section or something?

The "Reviewing code with Phabricator" section seems like it would be a reasonable place to link to the markup description. Or a new "tips" section could work too. Deferring that to a followup is fine.

Current revision LGTM.

[klimek]

docs/Phabricator.rst

130–136 ↗

(On Diff #43774)

I would phrase that less strongly. Remember that email is still the only system of record, so there's no requirement to make phab auto-close issues.

I'd probably rephrase to something like:
Note that you can get phabricator to automatically close the review when it finds the following line in the commit message:
...

145–147 ↗

(On Diff #43774)

Drop the must, it really is optional.

[delcypher]

docs/Phabricator.rst

130–136 ↗

(On Diff #43774)

I would phrase that less strongly. Remember that email is still the only system of record, so there's no requirement to make phab auto-close issues.

I don't really agree with that. Although there is no requirement to use Phabricator for reviews if the decision has been made to use it then I think we should be documenting the way Phabricator should be used that is most helpful to LLVM:

• Having closed reviews makes searching for un-committed patches easier
• Having a link in a commit message to the Phabricator review is useful when looking back at LLVM's history

I'm happy to weaken the language a little (e.g. turning must into should) if that helps.

145–147 ↗

(On Diff #43774)

I'm happy to weaken the language a little (e.g. turning must into should) here if that helps.

[klimek]

docs/Phabricator.rst

130–136 ↗

(On Diff #43774)

How about saying "... consider to ..."?

Phab is currently really just there to make it easier to do email based code reviews. If you want to propose a change to that policy, feel free to propose it on the llvm-dev/cfe-dev mailing lists.

[delcypher]

• Weaken "must" as requested by Manuel. It now says "recommended"
• Rebase on current trunk to encorporate the changes in r257180 which would not merge cleanly.

[delcypher]

@klimek

Is this new version satisfactory to you?

[probinson]

In D15801#322606, @delcypher wrote:

• Weaken "must" as requested by Manuel. It now says "recommended"
• Rebase on current trunk to encorporate the changes in r257180 which would not merge cleanly.

Sorry! Forgot this was still in progress...

[klimek]

docs/Phabricator.rst

130–132 ↗

(On Diff #44375)

s/you follow you/you follow/

[klimek]

LG apart from the superfluous "you".

[delcypher]

• Fix typo

llvm/trunk/docs/GettingStarted.rst

[imap]
         sslverify = false
   ; in English
         folder = "[Gmail]/Drafts"
   ; example for Japanese, "Modified UTF-7" encoded.
         folder = "[Gmail]/&Tgtm+DBN-"
   ; example for Traditional Chinese
         folder = "[Gmail]/&g0l6Pw-"
 
+.. _developers-work-with-git-svn:
+
 For developers to work with git-svn
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 To set up clone from which you can submit code using ``git-svn``, run:
 
 .. code-block:: console
 
   % git clone http://llvm.org/git/llvm.git

llvm/trunk/docs/Phabricator.rst

 people prefer it over a web interface, we do not generate automated mail
 when a review changes state, for example by clicking "Accept Revision" in
 the web interface. Thus, please type LGTM into the comment box to accept
 a change from Phabricator.
 
 Committing a change
 -------------------
 
-Arcanist can manage the commit transparently. It will retrieve the description,
-reviewers, the ``Differential Revision``, etc from the review and commit it to the repository.
+Once a patch has been reviewed and approved on Phabricator it can then be
+committed to trunk.  There are multiple workflows to achieve this. Whichever
+method you follow it is recommend that your commit message ends with the line:
+
+::
+
+  Differential Revision: <URL>
+
+where ``<URL>`` is the URL for the code review, starting with
+``http://reviews.llvm.org/``.
+
+This allows people reading the version history to see the review for
+context. This also allows Phabricator to detect the commit, close the
+review, and add a link from the review to the commit.
+
+Note that if you use the Arcanist tool the ``Differential Revision`` line will
+be added automatically. If you don't want to use Arcanist, you can add the
+``Differential Revision`` line (as the last line) to the commit message
+yourself.
+
+Using the Arcanist tool can simplify the process of committing reviewed code
+as it will retrieve reviewers, the ``Differential Revision``, etc from the review
+and place it in the commit message. Several methods of using Arcanist to commit
+code are given below. If you do not wish to use Arcanist then simply commit
+the reviewed patch as you would normally.
+
+Note that if you commit the change without using Arcanist and forget to add the
+``Differential Revision`` line to your commit message then it is recommended
+that you close the review manually. In the web UI, under "Leap Into Action" put
+the SVN revision number in the Comment, set the Action to "Close Revision" and
+click Submit.  Note the review must have been Accepted first.
+
+Subversion and Arcanist
+^^^^^^^^^^^^^^^^^^^^^^^
+
+On a clean Subversion working copy run the following (where ``<Revision>`` is
+the Phabricator review number):
 
 ::
 
   arc patch D<Revision>
   arc commit --revision D<Revision>
 
+The first command will take the latest version of the reviewed patch and apply it to the working
+copy. The second command will commit this revision to trunk.
+
+git-svn and Arcanist
+^^^^^^^^^^^^^^^^^^^^
 
-When committing a change that has been reviewed using
-Phabricator, the convention is for the commit message to end with the
-line:
+This presumes that the git repository has been configured as described in :ref:`developers-work-with-git-svn`.
+
+On a clean Git repository on an up to date ``master`` branch run the
+following (where ``<Revision>`` is the Phabricator review number):
 
 ::
 
-  Differential Revision: <URL>
+  arc patch D<Revision>
 
-where ``<URL>`` is the URL for the code review, starting with
-``http://reviews.llvm.org/``.
 
-Note that Arcanist will add this automatically.
+This will create a new branch called ``arcpatch-D<Revision>`` based on the
+current ``master`` and will create a commit corresponding to ``D<Revision>`` with a
+commit message derived from information in the Phabricator review.
+
+Check you are happy with the commit message and amend it if necessary. Now switch to
+the ``master`` branch and add the new commit to it and commit it to trunk. This
+can be done by running the following:
+
+::
+
+  git checkout master
+  git merge --ff-only arcpatch-D<Revision>
+  git svn dcommit
 
-This allows people reading the version history to see the review for
-context.  This also allows Phabricator to detect the commit, close the
-review, and add a link from the review to the commit.
 
-If you use ``git`` or ``svn`` to commit the change and forget to add the line
-to your commit message, you should close the review manually. In the web UI,
-under "Leap Into Action" put the SVN revision number in the Comment, set the
-Action to "Close Revision" and click Submit. Note the review must have been
-Accepted first.
 
 Abandoning a change
 -------------------
 
 If you decide you should not commit the patch, you should explicitly abandon
 the review so that reviewers don't think it is still open. In the web UI,
 scroll to the bottom of the page where normally you would enter an overall
 comment. In the drop-down Action list, which defaults to "Comment," you should