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

Thu Jan 10 02:41:42 PST 2019

kristina marked 5 inline comments as done.
kristina added a comment.

Addressed all comments, will wait for feedback and then do the final draft, and if people are happy with that, I'll lint the whole file and then commit it.

Any feedback on the idea of an alternative, much lighter Conduit API client (since we only use a subset of Phab features) where it's also easier to (by default) hook linters or various other tools to? Not sure how others feel about Arcanist especially considering it covers a massive set of APIs beyond what's commonly used here and a local PHP installation just to run it is often undesirable (A simpler one-file client in Python should suffice, especially considering that many are changing their workflows due to the Git migration).

================
Comment at: docs/Phabricator.rst:35-40
+  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.
----------------
kristof.beyls wrote:
> This feels like a bit of duplication in the documentation. I wonder if something like
> "The easiest way is to insert a line in your commit message, as documented below_[link to below]'
> ?
> It will keep the documentation a bit shorter, which should help for people to actually read through all of it...
Ah, yes the first point needs rewriting since there's duplication of comments regarding closing/abandoning but I wanted to have the part about commit messages right at the top so it's visible, as currently not many diffs are handled that way post review, and this is by far the best way as it not only links the Phabricator Differential ID on the Phabricator side but also makes sure it's in the commit message making it much easier to find from the commit logs. I almost feel like it should be a policy if requesting a Differential Review through Phabricator to ensure the commit links back to the Differential (especially useful when looking at things like the buildmaster console). It's very little effort and it makes sure it's easy to relate reviews and commits from both ends.

================
Comment at: docs/Phabricator.rst:52
+  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``).
----------------
Make sure capitalization of "Developer Policy" is consistent. 

================
Comment at: docs/Phabricator.rst:55-56
+* 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
----------------
kristof.beyls wrote:
> This seems to be inconsistent with a maybe unwritten policy which seems to exist that "if you asked for review, you must wait until someone reviewed it."
> This sentence seems to suggest that "it's ok to put something up for review and then later commit it without sign-off in at least some circumstances".
> I think it would be useful if there was a way to somehow share with the community "I intend to commit this - it seems obviously good to me - but I want to give time for concerns to be raised."
> I think currently, this is mostly done by just going for post-commit review.
> I'm not sure what a good solution for the above is.
I think for smaller commits where a review is requested but nothing happens after several weeks, instead of a ping, it may be possible to announce the intention to commit if there are no objections, and then land it. For bigger diffs it takes a long time to review and often some discussion takes place outside Phab or even mailing lists (for example on IRC), not to mention that bigger commits are much easier to get wrong or miss out on tests. 

Maybe it's best to reward it to something like "If possible, try to split up large revisions into several patchsets, even if they are around the same area, smaller patches are easier to review, even if they're landed as a stack." (and possibly explain how to annotate dependencies on Phab to show that patch A depends on patch B etc).

That follows the spirit of the developer policy regarding smaller incremental changes being more favorable.

================
Comment at: docs/Phabricator.rst:60-62
+* 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.
----------------
kristof.beyls wrote:
> Maybe also state: "If you do commit with unaddressed issues on the Phabricator review, please add a comment to the Phabricator review why those unaddressed issues can be ignored before committing."
> I think it's nice for people to be able to find after the fact why those unaddressed issues were felt to not be important enough to block a commit.
> Adding a comment on the Phabricator review may well be the most discoverable place for such rationales.
Good point, I think it's fine to do just before or just after committing as the reviewers will see them anyway and the mailing list will still get CC'd (phrasing it like "This also reduces the likelihood of concerns with the patch being raised on the mailing lists"). Maybe the point about actual post-commit review being best done on mailing lists can be merged in here as well.

================
Comment at: docs/Phabricator.rst:68-73
+  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. 
----------------
kristof.beyls wrote:
> Maybe better to move the suggestion to the section "Finding potential reviewers" and just point to that section from here?
> There are more suggestions already in that section.
I agree, I think moving it down would allow expanding it a bit more since finding suitable reviewers is very difficult for new contributors. And I want to keep this section as small as possible so it's easier to skim over to make sure points about common bottlenecks in the review process are not missed out.

It would be nice to have something similar to a less formal CODE_OWNERS file where reviewers can add themselves as some kind of directory to help people find appropriate reviewers. That way anyone familiar with a particular area and willing to review patches for it can add themselves to it. (considering code owners sometimes don't comment on reviews but major reviewers signing off on it is good enough)

What's your opinion on having a directory like that? Since it A). Makes it clear which areas reviewers are familiar with (which helps avoiding adding reviewers who aren't) B). I believe faster and more quality reviews come from areas reviewers are interested in, having something like that could help establish that without individually looking up reviewers who are not code owners?

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56482/new/

https://reviews.llvm.org/D56482