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

Fri Jan 11 01:32:10 PST 2019

kristof.beyls added a comment.

> 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).

Even if there would be an easy to install and use tool to upload reviews into which we can hook linters and other tools, I think it'll remain very hard to require that everyone needs to use that tool to request review feedback.
Assuming that is true, it means the linter and other tools remain optional to be run on patches for review, and it'll be hard to maintain the consistency we'd like those linters and other tools to offer across the code base.
My guess is that somehow integrating the linters and other tools on the phab server side is what's needed to really have a significant impact on the consistency of the code going into the repo.
As an alternative, maybe having a bot running linters or other tools post-commit and feeding back to the authors might also be an option that may end up working reasonable in practice - not sure.

================
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
----------------
kristina wrote:
> 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.
> 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. 

I'm sympathetic to this idea, but I think you may need to give this idea more visibility through an "RFC" discussion on llvm-dev. It is a change in the de facto policy, so probably needs to have more visibility than just on this phab review.

> 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).

This feels like it belongs slightly more in the developer policy rather than on the documentation on how to use Phab - but I agree it's worthwhile to at least repeat it here. Should there also be a suggestion that if you split up your work in several patches you should try to somehow convey the overview of where you're going with the multiple patches somehow? (Could be a comment on the initial patch, an RFC thread on the mailing list, or something else still; with the best option depending on the situation)?

================
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. 
----------------
kristina wrote:
> 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?
I don't see code owners as go to reviewers TBH. If you'd look at who actually does reviews in specific areas, my impression is that most review is done by others than the code owner. This may of course depend on the area. I don't mean this as giving criticism to the code owners - there just seems to be way too much review work that needs to be done for the small-ish set of code owners to be able to do it all. Overall, I'm sceptical about how much value the concept of code owners actually brings. Anyway - that is slightly besides the question you were asking.

I'm not opposed to having a file documenting willing reviewers, but my feel is that something that doesn't need to be maintained might stay more relevant and up to date. For example, I wouldn't be surprised if a significant set of people in the CODE_OWNERS file happen to no longer be mainly working on LLVM-related stuff, and therefore just won't have enough time to fulfil the role of "go-to" reviewer.
For example, what if we could somehow automate the process of "look at previous reviews in the same area - who did perform those reviews?".
I think I've seen someone on IRC already show a small script to automate "who are the previous authors who touched the lines of code my patch touches?" as a way to automatically suggest a reasonable set of reviewers.
My personal bias would be to invest in having one or two scripts in llvm/utils that can take a patch as input and give answers to the above questions rather than try to manually maintain a set of potential reviewers per area.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56482