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

[Christian Kühnel via cfe-dev]

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

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


> 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/cfe-dev/attachments/20210504/4a73cdf4/attachment-0001.html>