[clangd-dev] [cfe-dev] [llvm-dev] [RFC] Deprecate email code reviews in favor of Phabricator

[via clangd-dev]

I’m not hearing any particular objection to moving to Phabricator-only for reviews.  The trend has been in that direction for years anyway.  The barriers for one-time contributors are (a) registration, and (b) unfamiliarity with a user-hostile web UI.  Regarding registration, it’s either subscribing to llvm-commits or to Phab; and IIUC, Phab understands github IDs, which makes that barrier pretty low.  Regarding the web UI, we have (or did have, admittedly I haven’t looked lately) step-by-step instructions in the docs for how to navigate Phab successfully, which mitigates (b) as much as we can.

I am in the camp of, why mess with llvm-commits?  The commit emails that Phab sends out already have the commit URL in them right at the top, which looks like it goes to a page where comments can be added (I haven’t actually tried adding a comment).  If those posted comments go to llvm-commits and the author, then I think the only necessary step to turn off email post-commit review is to make llvm-commits read-only except for Phabricator itself.
We don’t need the ability to have email replies on llvm-commits be recorded in Phab, if we can almost as easily make comments on Phab that go to llvm-commits.  Possibly I am overlooking some problem that abandoning llvm-commits entirely is going to solve?
--paulr

From: cfe-dev <cfe-dev-bounces at lists.llvm.org> On Behalf Of Christian Kühnel via cfe-dev
Sent: Tuesday, May 4, 2021 9:15 AM
To: Aaron Ballman <aaron at aaronballman.com>
Cc: llvm-dev <llvm-dev at lists.llvm.org>; clangd-dev at lists.llvm.org; openmp-dev at lists.llvm.org; lldb-dev at lists.llvm.org; cfe-dev at lists.llvm.org; libcxx-dev at lists.llvm.org; flang-dev at lists.llvm.org; parallel_libs-dev at lists.llvm.org
Subject: Re: [cfe-dev] [llvm-dev] [RFC] Deprecate email code reviews in favor of Phabricator

Hi Aaron,

> “Code reviews are conducted on our web-based code-review tool (see Code Reviews with Phabricator).”

Personally, I am in favor of this policy for initiating code reviews,
but am opposed to it for post-commit feedback. The problem, as I see
it, is that not every change *gets* code review via Phab and the email
lists are the only place on which to provide that post-commit
feedback.

You can set up notifications on commits in Phabricator. Phabricator adds the user "llvm-commits" as subscriber to certain reviews: https://reviews.llvm.org/H615<https://urldefense.com/v3/__https:/reviews.llvm.org/H615__;!!JmoZiZGBv3RvKRSx!p37c8uYDXWnnl1JAGj92AupZsGAGPG0jsP8mxCiLLHtiY0ihBoKrW5hV90ZiYb2jOw$>
We could do the same thing for commits...

So you can reply to any commit via the web UI (or email notification) and the author gets notified as well. One example that worked for me:
 https://reviews.llvm.org/rG7f9717b922d421c30f889034488563c67076aca1<https://urldefense.com/v3/__https:/reviews.llvm.org/rG7f9717b922d421c30f889034488563c67076aca1__;!!JmoZiZGBv3RvKRSx!p37c8uYDXWnnl1JAGj92AupZsGAGPG0jsP8mxCiLLHtiY0ihBoKrW5hV90YQuXXWaw$>

The Phab commit message does not have any subscribers though, and so
my understanding is that comments on that Phab interface are not
communicated out to the community as a whole, which means the
community may miss out on important post-commit-review information
like general awareness of the problem, workarounds people can use
until the author corrects something, alternative ideas on how to fix
the issue, etc. Or do I misunderstand the way Phab works in this
workflow?

We can add automatically subscribers to commits as well if we want to.
One random example where a subscriber was added to a commit via a Herald rule:
https://reviews.llvm.org/rG93537fabcee8fcfa3316d7abd5e935f7fe9b468f<https://urldefense.com/v3/__https:/reviews.llvm.org/rG93537fabcee8fcfa3316d7abd5e935f7fe9b468f__;!!JmoZiZGBv3RvKRSx!p37c8uYDXWnnl1JAGj92AupZsGAGPG0jsP8mxCiLLHtiY0ihBoKrW5hV90ZMMYsijg$>

Also, "the commenter would need to find the Phabricator commit"
concerns me -- adding extra burden on the people providing feedback
means that *some* amount of those people will struggle enough to
simply not provide that feedback. Responding to an email is about as
low as I think we can set that bar, so the current approach has better
ergonomics for giving feedback.

If we set up the notifications via Phabricator, you can reply via email. These contain a link at the bottom that will take you directly to the commit page in Phabricator. Not sure why we don't have these on the mailing list...

Tangential complaint -- our use of Herald causes some pain for me as a
list moderator because we've reached the point where Herald
automatically adds so many subscribers to a review that it gets marked
as spam for every email that is generated for the review.

So this would be a reason to replace the XXX-commits mailing lists with something else...

Given how often Phabricator goes down (which is not super often, but
often enough that people know it happens), I am deeply uncomfortable
with the idea of shutting down the current *-commits mailing lists
because of the chance for data loss. Personally, I think the *-commits
lists are working well and I would prefer they be left alone.

If Phabricator is down, you're not getting any email notifications from it anyway. So we might already be losing data right now...

But fair point: The more we rely on Phabricator, the higher the reliability requirements.

Best,
Christian


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/clangd-dev/attachments/20210504/3c5c664d/attachment-0001.html>