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 reviewers that + way. +* Try to include reviewers 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 committing + 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 ----------------------------------------