In my view, this email is really too different topics. Given that, my
response is split into two parts.
First, should we make phabricator our default for code review? I am not
opposed to this. I don't particular support it either, but I would not
spend time arguing against it. I would suggest that we re-frame the
proposal to distinguish precommit and post commit review - with only the
former moving to phabricator. I have not seen post-commit done
successfully on phabricator to date in any wide spread manner.
Second, should we consider retiring llvm-commits and the other mailing
lists? My gut response is a flat out NO!!!! What we have works. I am
highly reluctant to run the risk of breaking our existing processes -
which for all their problems mostly work - for the, to me, seemingly
very minimal value obtained by moving away from email discussion. Post
commit review in email works. I strongly suspect that if you try to
change that, you will either simply drive out post commit review
discussion (bad idea!) or discussions will move to private email
exchanges (bad idea!). I'm open to being convinced here, but the burden
of proof is high. The risk we'd be talking about with such a transition
is immense.
Philip
On 5/3/2021 10:24 AM, Krzysztof Parzyszek via llvm-dev 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).”
*Current situation:*
1. 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.)
2. 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.
3. 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:*
1. 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.
2. Review authors and reviewers would only need to monitor one source
of comments without the fear that a review comment may end up
overlooked.
3. Local Phabricator extensions would no longer be needed.
*Concerns:*
1. 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
<https://reviews.llvm.org/rG06234f758e19>). Those are communicated
(perhaps ironically) via email, which implies that those automatic
emails should remain in place.
2. 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.
3. 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].
* 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.
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
<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
<https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html>
[3] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html
<https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html>
[4] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html
<https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html>
[5] https://reviews.llvm.org/project/view/104/
<https://reviews.llvm.org/project/view/104/>
[6] https://reviews.llvm.org/D101432 <https://reviews.llvm.org/D101432>
[7] https://reviews.llvm.org/H769 <https://reviews.llvm.org/H769>
--
Krzysztof Parzyszek kparz...@quicinc.com
<mailto:kparz...@quicinc.com> AI tools development
_______________________________________________
LLVM Developers mailing list
llvm-...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
