[PATCH] D56482: DO NOT SUBMIT. Draft for guidelines on using Phabricator.

[Kristina Brooks via Phabricator via llvm-commits]

kristina created this revision.
kristina added reviewers: chandlerc, dblaikie, pcc.
Herald added a subscriber: llvm-commits.

This is a WIP draft of some guidelines on using Phab for pre-commit review with the intention on making it clearer on when to use certain features and avoiding a few common reoccurring patterns that slow down code review. I will take another look at it later, any comments or suggestions are welcome. The intention is to make it easier for new contributors to correctly use Phab as a review tool.


Repository:
  rL LLVM

https://reviews.llvm.org/D56482

Files:
  docs/Phabricator.rst


Index: docs/Phabricator.rst
===================================================================
--- docs/Phabricator.rst
+++ docs/Phabricator.rst
@@ -28,6 +28,64 @@
 Phabricator will automatically connect your submits to your Phabricator user in
 the `Code Repository Browser`_.
 
+General Guidelines
+------------------
+
+* As a general rule, please make sure that abandoned revisions are marked as
+  such and that once a revision is committed it is closed. The easiest way as
+  mentioned below involves simply putting the following on a separate line 
+  in your commit message where ``xxxxx`` is the revision number in the URL:
+  ``Differential Revision: https://reviews.llvm.org/Dxxxxx``
+  This will automatically update your revision with the committed one, link
+  it to the commit and close it.
+* In a similar fashion, if you're doing work on the revision, mark it as
+  ``Changes Planned`` which in turn means significant changes are planned to
+  the revision and that it should not be signed off on (although any feedback
+  is generally welcome on such revisions).
+* If you decide to not go through with the revision, please make sure to mark
+  it as Abandoned (using ``Abandon Revision``). This will close it making
+  it clear that it has not been committed as is and no review is requested for
+  it. As a general rule, don't reopen abandoned revisions, instead submit a new
+  patch if you need a review.
+* Make sure revisions have an associated mailing list as a subscriber. This
+  usually happens automatically if you select the right repository for the
+  patch. This ensures review happens in accordance with the developer policy
+  and the mailing list is notified (main one being ``llvm-commits``).
+* Pre-commit reviews are much better than post-commit reviews, in general, as
+  per developer policy, most commits should be reviewed or at the very least
+  be given time for others to voice potential concerns or point out issues.
+* Post-commit concerns are best addressed by replying directly to the relevant
+  mailing list. Reopening the revision is an alternative if the commit is 
+  relatively recent, but reopening old revisions is generally discouraged.
+* Unless discussed outside of Phabricator, in general, don't commit revisions
+  with unaddressed issues raised by reviewers. Doing so defeats the point of
+  pre-commit review.
+* Significant patches that affect multiple components (for example if you need
+  a patch for LLVMSupport followed by a patch to Clang's CodeGen), it may be
+  useful to split the two up since it's easier to find individual reviewrs that
+  way.
+* Try to include reviewrs relevant to the area the revision is for, it may be
+  difficult to work out at first, so searching for past reviews for similar
+  area is a good way to start. Some revisions are often added with a single
+  reviewer relevant to only one area, with followup revisions having the
+  same pattern despite being related to different areas. For bigger changes, it
+  is worth considering adding multiple reviewers depending on the size and
+  nature of the patches. This in turn ensures better review quality. 
+* Using Phabricator for in-progress work where at least some code is available
+  is acceptable, in which case please preface the revision title with
+  ``DO NOT SUBMIT``. Similar to normal review, make sure to close the revision
+  once it is no longer relevant. For RFCs/ideas without any proof-of-concept
+  implementations, development mailing lists are generally better for such
+  discussions.
+* If you don't have commit access and need someone to land the revision for you
+  after review, please ask, this speeds things up as it means reviewers don't
+  have to ask explicitly which may take several days. (For reviewers: Please
+  follow the attribution guidelines in the Developer Policy when commiting
+  patches on behalf of someone else).
+* Please make sure you submit patches with full context (as explained below) as
+  patches submitted without context will often be requested to be resubmitted,
+  slowing down the review process.
+
 Requesting a review via the command line
 ----------------------------------------
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D56482.180800.patch
Type: text/x-patch
Size: 4196 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190109/baa978b0/attachment.bin>