<div dir="ltr">Hi folks,<div><br></div><div>I'm working with Manuel Klimek and Sam McCall to streamline the code review process for LLVM/Clang/etc. diffs.</div><div><br></div><div>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.</div><div><br>I set up a proof-of-concept repository for <font face="monospace">clang-tools-extra</font> in <a href="https://reviews.llvm.org/D40179">https://reviews.llvm.org/D40179</a> and a Herald rule in <a href="https://reviews.llvm.org/H268">https://reviews.llvm.org/H268</a> to confirm this technique works.</div><div><br></div><div><font size="4"><b>Top-Level Goal</b></font></div><div><br></div><div>When sending a code review through <a href="http://reviews.llvm.org">reviews.llvm.org</a>, engineers currently have to remember to manually Cc: <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a> on every review request.</div><div><br></div><div>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.</div><div><br></div><div><font size="4"><b>Current Roadblocks</b></font></div><div><br></div><div>Normally, <a href="https://secure.phabricator.com/book/phabricator/article/diffusion_managing/">Differential's "repositories" feature</a> allows <a href="https://secure.phabricator.com/book/phabricator/article/herald/">Herald rules</a> to match reviews to set up Cc:s, etc. easily.</div><div><br></div><div>Currently, the LLVM project uses <a href="https://reviews.llvm.org/diffusion/L/">a single Differential repository for all projects</a>, including clang, clang-tools-extra, etc. — but we don't want to Cc: <font face="monospace">cfe-commits</font> on review requsts for LLVM so we cannot match on repository.</div><div><br></div><div>If we used a monorepo, we could match on path to assign the Cc:, but we have separate <font face="monospace">.arcconfig</font> files already  at the root of each repository.</div><div><br></div><div>This means Differential gets <b>project-relative paths</b> in its review requests, which means we cannot use Herald to easily distinguish diffs meant for different projects.</div><div><br></div><div>For example, if a review request is for <font face="monospace">lib/CMakeLists.txt</font>, we have no way to know if that is for <font face="monospace">llvm/lib/CMakeLists.txt</font> or <font face="monospace">llvm/tools/clang/lib/CMakeLists.txt</font>, etc.</div><div><br></div><div><font size="4"><b>Proposed Solution</b></font></div><div><br></div><div>Since we already have <font face="monospace">.arcconfig</font> files at the base of each repository, we can just create separate Differential repositories corresponding to each <font face="monospace">.arcconfig</font>, then set up Herald rules to automatically Cc: the appropriate list for each review request.</div><div><br></div><div><div>As noted in the tl;dr: above, I set up a proof-of-concept repository for clang-tools-extra in <a href="https://reviews.llvm.org/D40179">https://reviews.llvm.org/D40179</a> and a Herald rule in <a href="https://reviews.llvm.org/H268">https://reviews.llvm.org/H268</a> to confirm this technique works.</div><br class="inbox-inbox-Apple-interchange-newline"></div><div><font size="4"><b>Potential Issues</b></font></div><div><font size="4"><b><br></b></font></div><div><ul><li>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.)<br></li><li>Moving forward, only LLVM commits will be identified with the prefix <font face="monospace">rL</font> (as in <a href="https://reviews.llvm.org/rL12345">https://reviews.llvm.org/rL12345</a>) — each project will get its own unique prefix, which might surprise some users or require changes to tools which bake in the <font face="monospace">rL</font> prefix</li><li>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.</li></ul><div>Please share thoughts and feedback, and thanks!</div></div><div><br>Ben</div></div>