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

Tue May 4 17:34:24 PDT 2021

On Tue, May 4, 2021 at 4:24 AM Aaron Ballman via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> On Mon, May 3, 2021 at 1:24 PM Krzysztof Parzyszek via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
> >
> > Statement:
> >
> > Our current code review policy states[1]:
> >
> > “Code reviews are conducted, in order of preference, on our web-based
> code-review tool (see Code Reviews with Phabricator), by email on the
> relevant project’s commit mailing list, on the project’s development list,
> or on the bug tracker.”
> >
> > This proposal is to limit code reviews only to Phabricator.  This would
> apply to all projects in the LLVM monorepo.  With the change in effect, the
> amended policy would read:
> >
> > “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. This largely comes up in two ways: NFC changes and changes
> made by code owners in the area of the compiler which they own. We
> still need *some* mechanism by which to provide them post-commit
> feedback. Currently, the way we provide that is frequently via an
> email reply to the commit message on the *-commits list so that the
> issue can be seen by both the patch author and the community at large.
>
> > Current situation:
> >
> > In a recent llvm-dev thread[2], Christian Kühnel pointed out that
> pre-commit code reviews rarely originate via an email (most are started on
> Phabricator), although, as others pointed out, email responses to an
> ongoing review are not uncommon.  (That thread also contains examples of
> mishaps related to the email-Phabricator interactions, or email handling
> itself.)
> > I don’t have specific information about post-commit reviews.  It seems
> like the most common form is an email reply to the auto-generated commit
> message, although (in my personal experience), “raising a concern” in the
> commit on Phabricator or commenting in the pre-commit review is usually
> sufficient to get author’s attention.
> > We have Phabricator patches that automatically apply email comments to
> the Phabricator reviews, although reportedly this functionality is not
> fully reliable[3,4].  This can cause review comments to be lost in the
> email traffic.
> >
> >
> >
> > Benefits:
> >
> > Single way of doing code reviews: code reviews are a key part of the
> development process, and having one way of performing them would make the
> process clearer and unambiguous.
> > Review authors and reviewers would only need to monitor one source of
> comments without the fear that a review comment may end up overlooked.
> > Local Phabricator extensions would no longer be needed.
> >
> >
> >
> > Concerns:
> >
> > For post-commit reviews, the commenter would need to find either the
> original review, or the Phabricator commit (e.g.
> https://reviews.llvm.org/rG06234f758e19).  Those are communicated
> (perhaps ironically) via email, which implies that those automatic emails
> should remain in place.
>
> 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?
>
> 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. When I look at an existing NFC commit
> email (
> https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20210503/368413.html
> ),
> I don't see any direct link back to the Phabricator commit, so I have
> to know to get the hash and try using that with an
> https://reviews.llvm.org/rG in front of it.

This is a simple problem to solve by fixing the format of the emails that
are sent, I believe we have control over this right now. We could:

- add the https://reviews.llvm.org/rG<hash> to every email, and not to
GitHub.
- make sure that the lists are subscribed on the commits on Phab so that
notifications are generated to the list on post-commit feedback.

> None of the existing links
> in the email get me to where I'd need to go to add my review feedback,
> but hitting the Reply button in my mail client will work. Adding a
> third link and telling people "click on the link in the email" means
> they've got a 50/50 shot of getting the right link because one of the
> links goes to GitHub where you can also add comments.
>
> > The current policy has been in place for a long time and it’s expected
> that some people will continue using email for reviews out of habit or due
> to lack of awareness of the policy change.
> > Because of the larger variety, email clients may offer better
> accessibility options than web browsers.
> >
> >
> >
> > Potential future direction:
> >
> > This section presents a potential future evolution of the review
> process.  Christian has conducted experiments suggesting that we can
> replace the XXX-commits mailing lists with notifications directly from
> Phabricator:
> >
> > For each of the mailing lists, we create a "project" with the same name
> in Phabricator, e.g. [5]. Every Phabricator user can join/leave these
> projects on their own.
> > Everyone on these projects will receive the same email notifications
> from Phabricator as we have on the mailing lists. This is configured via
> "Herald" rules in Phabricator, as today, e.g. [7].
>
> 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. It's to the
> point on cfe-dev where over half of the moderated emails are
> consistently not spam some weeks.

I'm surprised that the reviews are going to cfe-dev@ and not cfe-commits@?
But also, I didn't know that the mailman instance had a spam filter: I
thought this was based purely on whether the sender is subscribed (or
whitelisted)?

> This delays the community receiving
> the information while the patch reviewers/subscribers continue to get
> it. In turn, this causes a problem where sometimes the people
> subscribed to the patch say something is OK and the patch lands before
> the community ever sees the review and has a chance to comment on it.
> I'm wary of suggestions that involve heavier use of Herald until we
> get that mailing list issue resolved.
>
> > Users can reply to these email notifications and Phabricator will
> incorporate these responses with their email client, see [6] for some
> example emails. Quoting and markup is supported as well.
> > We do NOT migrate the membership lists. Users need to sign up to the
> projects manually. We will send an email with instructions to the mailing
> lists once everything is set up.
> > The current XXX-commits mailing lists will be shut down
> > The timeline for the migration is to be defined.
>
> Given how often Phabricator goes down (which is not super often, but
> often enough that people know it happens),

Data-driven: I am aware of two outages for the last entire year, and each
lasted for < 1/2 day.

> 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.
>
> ~Aaron
>
> >
> > For experimenting, feel free to sign up to the prototype project at [5]
> . This project receives all commit and code review notifications.
> >
> >
> >
> >
> >
> > [1]
> https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review
> >
> > [2] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html
> >
> > [3] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html
> >
> > [4] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html
> >
> > [5] https://reviews.llvm.org/project/view/104/
> >
> > [6] https://reviews.llvm.org/D101432
> >
> > [7] https://reviews.llvm.org/H769
> >
> >
> >
> >
> >
> > --
> >
> > Krzysztof Parzyszek  kparzysz at quicinc.com   AI tools development
> >
> >
> >
> > _______________________________________________
> > cfe-dev mailing list
> > cfe-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210504/f2b3bc34/attachment.html>