DO NOT SUBMIT. Draft for guidelines on using Phabricator.

[kristina]

Summary

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.

[kristina]

Spellcheck.

[kristina]

docs/Phabricator.rst

34

I think this point needs to be rewritten, main emphasis being on not leaving "stray" revisions behind at which point it's hard to tell if they've been committed but not linked or abandoned etc.

63

For significant*

[JonasToth]

I like this guide!
Do you think we should include running clang-tidy and other linters over it?

@MyDeveloperDay has some thoughts and provided some linting for documentation already. @Eugene.Zelenko has interest too I guess.

[MyDeveloperDay]

$ ./validate_check.py --rst ../../../../../docs/Phabricator.rst
Checking ..\..\..\..\..\docs\Phabricator.rst...
warning: line 97 multiple blank lines
warning: line 166 is in excess of 80 characters (82) : as it will retrieve reviewers, the `Differential Revision`, etc from the review
...
warning: line 175 contains double spaces
warning: line 188 is in excess of 80 characters (97) : The first command will take the latest version of the reviewed patch and apply it to the working
...
warning: line 194 is in excess of 80 characters (111) : This presumes that the git repository has been configured as described in :ref:developers-work-with-git-svn.
...
warning: line 203 multiple blank lines
warning: line 205 is in excess of 80 characters (84) : current `master and will create a commit corresponding to D<Revision>` with a
...
warning: line 208 is in excess of 80 characters (85) : Check you are happy with the commit message and amend it if necessary. Now switch to
...
warning: line 218 multiple blank lines
warning: line 219 multiple blank lines

[kristina]

In D56482#1350854, @MyDeveloperDay wrote:

$ ./validate_check.py --rst ../../../../../docs/Phabricator.rst
Checking ..\..\..\..\..\docs\Phabricator.rst...
warning: line 97 multiple blank lines
warning: line 166 is in excess of 80 characters (82) : as it will retrieve reviewers, the `Differential Revision`, etc from the review
...
warning: line 175 contains double spaces
warning: line 188 is in excess of 80 characters (97) : The first command will take the latest version of the reviewed patch and apply it to the working
...
warning: line 194 is in excess of 80 characters (111) : This presumes that the git repository has been configured as described in :ref:developers-work-with-git-svn.
...
warning: line 203 multiple blank lines
warning: line 205 is in excess of 80 characters (84) : current `master and will create a commit corresponding to D<Revision>` with a
...
warning: line 208 is in excess of 80 characters (85) : Check you are happy with the commit message and amend it if necessary. Now switch to
...
warning: line 218 multiple blank lines
warning: line 219 multiple blank lines

I only added a section, all the warnings are from existing bits below. Not sure if it's a good idea to reformat it as part of the diff as it will make it harder to see the added section. Can do it afterwards but that's what's currently being used to generate documentation.

[Eugene.Zelenko]

It'll be worth to add next suggestion:

• Use [product/library] as revision title prefix.
• Specifying project may attract attention of reviewers.
• Revision dependencies are useful for cross-project changes.

[MyDeveloperDay]

! In D56482#1350874, @kristina wrote:

I only added a section, all the warnings are from existing bits below. Not sure if it's a good idea to reformat it as part of the diff as it will make it harder to see the added section. Can do it afterwards but that's what's currently being used to generate documentation.

Sorry I wasn't clear, I'm using a script from D55523: [clang-tidy] Linting .rst documentation to validate .rst files, if you read the revision there is some info on the rational. (I simply ran it here on a copy of the file you are editing prior to your revision). You'll see from D55523 that I mention that I saw a lot of review comments which amounted to stylistic changes in the .rst files, and yet I see a lot of reviewers time pointing these out, time and time again and its all simple stuff that could be automated. (or at least added to such a how to guide)

it would be good if we could ensure .rst files are checked prior to review (how we do this I don't know) without initially generating 10,000 of minor whitespaces changes.

when I reviewed all .rst files currently checked into LLVM I found 10,000's of simple violations (maybe people aren't worried about it but certainly it comes up a lot in reviews and causes the second or third revisions)

I think another point is that lot of review comments could potentially be caught by clang-tidy, there is sometimes lots of the review conversations especially with new contributors, about "don't use anonymous namspaces but prefer static functions","clang-format the file", don't overly use auto", don't have "const value types" all stuff that is in the CodingStyle but is easily missed when making or updating a review.

It would be good if we had a mechanism to be able to validate a patch prior to commit, maybe using something like clang-tidy-diff.py in combination with something like the .rst linter to reduce the iterations on a revision, speed up development and reduce the burden that seems present on a group of dedicated but possible select people.

[fhahn]

It might be good to mention those guidelines explicitly here https://llvm.org/docs/Contributing.html#how-to-submit-a-patch.

[MyDeveloperDay]

In a later change it might be worth mentioning...

To submit an updated patch:

• Click *Differential*.
• Click *+ Create Diff*.

This can actually be more easily performed by going to the revision and selecting "Update diff", which can help to ensure you don't select the wrong revision

[kristina]

In D56482#1351638, @MyDeveloperDay wrote:

! In D56482#1350874, @kristina wrote:

I only added a section, all the warnings are from existing bits below. Not sure if it's a good idea to reformat it as part of the diff as it will make it harder to see the added section. Can do it afterwards but that's what's currently being used to generate documentation.

Sorry I wasn't clear, I'm using a script from D55523: [clang-tidy] Linting .rst documentation to validate .rst files, if you read the revision there is some info on the rational. (I simply ran it here on a copy of the file you are editing prior to your revision). You'll see from D55523 that I mention that I saw a lot of review comments which amounted to stylistic changes in the .rst files, and yet I see a lot of reviewers time pointing these out, time and time again and its all simple stuff that could be automated. (or at least added to such a how to guide)

it would be good if we could ensure .rst files are checked prior to review (how we do this I don't know) without initially generating 10,000 of minor whitespaces changes.

when I reviewed all .rst files currently checked into LLVM I found 10,000's of simple violations (maybe people aren't worried about it but certainly it comes up a lot in reviews and causes the second or third revisions)

I think another point is that lot of review comments could potentially be caught by clang-tidy, there is sometimes lots of the review conversations especially with new contributors, about "don't use anonymous namspaces but prefer static functions","clang-format the file", don't overly use auto", don't have "const value types" all stuff that is in the CodingStyle but is easily missed when making or updating a review.

It would be good if we had a mechanism to be able to validate a patch prior to commit, maybe using something like clang-tidy-diff.py in combination with something like the .rst linter to reduce the iterations on a revision, speed up development and reduce the burden that seems present on a group of dedicated but possible select people.

That would be nice, I mean the issue with Phab is that Arcanist is difficult/not-nice to use (IMO), and I think most of its uses of conduit APIs here are pretty limited, would be nice to have something like a lighter Arcanist in python, that doesn't require a local PHP installation. Without changing Phab itself it would be a nice place to add things like pre-diff submission hooks, ideally with something people can extend that fits into their build systems/CI. Would be an interesting project as far as review efficiency goes. At least in my opinion, I tried using Arcanist for like two commits and went back to manual after since it just felt clunky.

As far as code style goes, a lot of parts like MCStreamer are difficult to automatically format and in some places it can produce unfavorable results, so I'm 50/50 on automatically running a linter on diffs since the actual diff may come out weird. Readability is a big aspect to consider and I've seen a lot of code produced by clang-format that isn't necessarily easy to read, but is committed anyway because clang-format is considered to be a de-facto tool for ensuring code style is followed. A lot of corner cases, for example in MC layer in LLVM, an automatic linter may be less desirable than someone familiar with the style guide manually formatting the patch. Again just an opinion, not sure how others feel about that.

[Eugene.Zelenko]

In D56482#1351638, @MyDeveloperDay wrote:

I think another point is that lot of review comments could potentially be caught by clang-tidy, there is sometimes lots of the review conversations especially with new contributors, about "don't use anonymous namspaces but prefer static functions","clang-format the file", don't overly use auto", don't have "const value types" all stuff that is in the CodingStyle but is easily missed when making or updating a review.

It would be good if we had a mechanism to be able to validate a patch prior to commit, maybe using something like clang-tidy-diff.py in combination with something like the .rst linter to reduce the iterations on a revision, speed up development and reduce the burden that seems present on a group of dedicated but possible select people.

Default Clan-tidy configuration is too permissive on my view. There are much more useful checks then enabled by default. But this should be subject for much wider discussion.

Other problem that there is only three checks for compliance with LLVM coding guidelines.

[kristof.beyls]

docs/Phabricator.rst

35–40

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

55–56

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.

60–62

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.

68–73

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.

[kristina]

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

docs/Phabricator.rst

35–40

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.

52

Make sure capitalization of "Developer Policy" is consistent.

55–56

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.

60–62

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.

68–73

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?

[kristof.beyls]

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.

docs/Phabricator.rst

55–56

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

68–73

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.

[kristina]

Abandoning in favor of D71916.

docs/Phabricator.rst

 Make *sure* that the email address registered with Phabricator is subscribed
 to the relevant -commits mailing list. If you are not subscribed to the commit
 list, all mail sent by Phabricator on your behalf will be held for moderation.
 
 Note that if you use your Subversion user name as Phabricator user name,
 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
 ----------------------------------------
 
 Phabricator has a tool called *Arcanist* to upload patches from
 the command line. To get you set up, follow the
 `Arcanist Quick Start`_ instructions.
 
 You can learn more about how to use arc to interact with