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

[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  kparz...@quicinc.com<mailto:kparz...@quicinc.com>   AI 
tools development


_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Reply via email to