[PATCH] D15801: Improve the documentation on committing code reviewed on Phabricator to trunk.

[Dan Liew via llvm-commits]

delcypher added inline comments.

================
Comment at: docs/Phabricator.rst:130-136
@@ -129,3 +129,9 @@
 
-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 you must ensure that your commit message ends with the line:
+
+::
+
+  Differential Revision: <URL>
+
----------------
klimek wrote:
> 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:
> ...
> 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.

================
Comment at: docs/Phabricator.rst:145-147
@@ +144,5 @@
+
+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 **must** add
+the ``Differential Revision`` line to the commit message yourself.
+
----------------
klimek wrote:
> Drop the **must**, it really is optional.
I'm happy to weaken the language a little (e.g. turning **must** into **should**) here if that helps.


http://reviews.llvm.org/D15801