[cfe-dev] [Proposal] Automatically Cc: cfe-commits@ on Clang reviews

[Ben Hamilton via cfe-dev]

Hi folks,

I'm working with Manuel Klimek and Sam McCall to streamline the code review
process for LLVM/Clang/etc. diffs.

tl;dr: I want to set up separate Differential repositories for each
LLVM/Clang/etc. project, so Herald can automatically Cc: the correct list
for each review request.

I set up a proof-of-concept repository for clang-tools-extra in
https://reviews.llvm.org/D40179 and a Herald rule in
https://reviews.llvm.org/H268 to confirm this technique works.

*Top-Level Goal*

When sending a code review through reviews.llvm.org, engineers currently
have to remember to manually Cc: cfe-commits at lists.llvm.org on every review
request.

We don't want to unnecessarily Cc: this list for non-Clang review requests,
so we need a solution to set up the proper Cc:s for each review.

*Current Roadblocks*

Normally, Differential's "repositories" feature
<https://secure.phabricator.com/book/phabricator/article/diffusion_managing/>
allows
Herald rules
<https://secure.phabricator.com/book/phabricator/article/herald/> to match
reviews to set up Cc:s, etc. easily.

Currently, the LLVM project uses a single Differential repository for all
projects <https://reviews.llvm.org/diffusion/L/>, including clang,
clang-tools-extra, etc. — but we don't want to Cc: cfe-commits on review
requsts for LLVM so we cannot match on repository.

If we used a monorepo, we could match on path to assign the Cc:, but we
have separate .arcconfig files already  at the root of each repository.

This means Differential gets *project-relative paths* in its review
requests, which means we cannot use Herald to easily distinguish diffs
meant for different projects.

For example, if a review request is for lib/CMakeLists.txt, we have no way
to know if that is for llvm/lib/CMakeLists.txt or
llvm/tools/clang/lib/CMakeLists.txt, etc.

*Proposed Solution*

Since we already have .arcconfig files at the base of each repository, we
can just create separate Differential repositories corresponding to each
.arcconfig, then set up Herald rules to automatically Cc: the appropriate
list for each review request.

As noted in the tl;dr: above, I set up a proof-of-concept repository for
clang-tools-extra in https://reviews.llvm.org/D40179 and a Herald rule in
https://reviews.llvm.org/H268 to confirm this technique works.

*Potential Issues*


   - Separate Differential repositories require a bit more back-end storage
   and CPU on the Phabricator instance (but aside from LLVM and Clang, I think
   the other repos are small enough that this won't be significant.)
   - Moving forward, only LLVM commits will be identified with the prefix rL
   (as in https://reviews.llvm.org/rL12345) — each project will get its own
   unique prefix, which might surprise some users or require changes to tools
   which bake in the rL prefix
   - Moving to a monorepo in the future won't require too much work, but we
   will either need to move to a single Differential repository and re-adjust
   the Herald rules to handle absolute paths, or continue using separate
   Differential repositories even though the underlying storage is a monorepo.

Please share thoughts and feedback, and thanks!

Ben
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20171120/8266dcb8/attachment.html>