<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body>
<p>In my view, this email is really too different topics. Given
that, my response is split into two parts.</p>
<p>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. <br>
</p>
<p>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.
<br>
</p>
<p>Philip<br>
</p>
<div class="moz-cite-prefix">On 5/3/2021 10:24 AM, Krzysztof
Parzyszek via llvm-dev wrote:<br>
</div>
<blockquote type="cite"
cite="mid:BL0PR02MB3748C3ABC055E82CEEBD45BADD5B9@BL0PR02MB3748.namprd02.prod.outlook.com">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style>@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}p.MsoPlainText, li.MsoPlainText, div.MsoPlainText
{mso-style-priority:99;
mso-style-link:"Plain Text Char";
margin:0in;
font-size:10.0pt;
font-family:Consolas;}span.PlainTextChar
{mso-style-name:"Plain Text Char";
mso-style-priority:99;
mso-style-link:"Plain Text";
font-family:Consolas;}.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}div.WordSection1
{page:WordSection1;}ol
{margin-bottom:0in;}ul
{margin-bottom:0in;}</style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<div class="WordSection1">
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Statement:<o:p></o:p></span></b></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Our
current code review policy states[1]:<o:p></o:p></span></p>
<p class="MsoPlainText" style="margin-left:.5in"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">“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.”<o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">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:<o:p></o:p></span></p>
<p class="MsoPlainText" style="margin-left:.5in"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">“Code
reviews are conducted on our web-based code-review tool (see
Code Reviews with Phabricator).”<o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Current
situation:<o:p></o:p></span></b></p>
<ol style="margin-top:0in" type="1" start="1">
<li class="MsoPlainText" style="mso-list:l2 level1 lfo1"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">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.)<o:p></o:p></span></li>
<li class="MsoPlainText" style="mso-list:l2 level1 lfo1"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">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.<o:p></o:p></span></li>
<li class="MsoPlainText" style="mso-list:l2 level1 lfo1"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">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.<o:p></o:p></span></li>
</ol>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Benefits:<o:p></o:p></span></b></p>
<ol style="margin-top:0in" type="1" start="1">
<li class="MsoPlainText" style="mso-list:l1 level1 lfo2"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">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.<o:p></o:p></span></li>
<li class="MsoPlainText" style="mso-list:l1 level1 lfo2"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Review
authors and reviewers would only need to monitor one
source of comments without the fear that a review comment
may end up overlooked.<o:p></o:p></span></li>
<li class="MsoPlainText" style="mso-list:l1 level1 lfo2"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Local
Phabricator extensions would no longer be needed.<o:p></o:p></span></li>
</ol>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Concerns:<o:p></o:p></span></b></p>
<ol style="margin-top:0in" type="1" start="1">
<li class="MsoPlainText" style="mso-list:l3 level1 lfo3"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">For
post-commit reviews, the commenter would need to find
either the original review, or the Phabricator commit
(e.g.
<a href="https://reviews.llvm.org/rG06234f758e19"
moz-do-not-send="true">https://reviews.llvm.org/rG06234f758e19</a>).
Those are communicated (perhaps ironically) via email,
which implies that those automatic emails should remain in
place.<o:p></o:p></span></li>
<li class="MsoPlainText" style="mso-list:l3 level1 lfo3"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">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.<o:p></o:p></span></li>
<li class="MsoPlainText" style="mso-list:l3 level1 lfo3"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Because
of the larger variety, email clients may offer better
accessibility options than web browsers.<o:p></o:p></span></li>
</ol>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Potential
future direction:<o:p></o:p></span></b></p>
<p class="MsoNormal">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:<o:p></o:p></p>
<ul type="disc">
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0
level1 lfo4">
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. <o:p></o:p></li>
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0
level1 lfo4">
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].<o:p></o:p></li>
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0
level1 lfo4">
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.<o:p></o:p></li>
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0
level1 lfo4">
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.<o:p></o:p></li>
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0
level1 lfo4">
The current XXX-commits mailing lists will be shut down<o:p></o:p></li>
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0
level1 lfo4">
The timeline for the migration is to be defined.<o:p></o:p></li>
</ul>
<p class="MsoNormal">For experimenting, feel free to sign up to
the prototype project at [5] . This project receives all
commit and code review notifications.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">[1]
<a
href="https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review"
moz-do-not-send="true">
https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review</a><o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">[2]
<a
href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html"
moz-do-not-send="true">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html</a><o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">[3]
<a
href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html"
moz-do-not-send="true">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html</a><o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">[4]
<a
href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html"
moz-do-not-send="true">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html</a><o:p></o:p></span></p>
<p class="MsoNormal">[5] <a
href="https://reviews.llvm.org/project/view/104/"
moz-do-not-send="true">https://reviews.llvm.org/project/view/104/</a><o:p></o:p></p>
<p class="MsoNormal">[6] <a
href="https://reviews.llvm.org/D101432"
moz-do-not-send="true">https://reviews.llvm.org/D101432</a><o:p></o:p></p>
<p class="MsoNormal">[7] <a href="https://reviews.llvm.org/H769"
moz-do-not-send="true">https://reviews.llvm.org/H769</a><o:p></o:p></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText">-- <o:p></o:p></p>
<p class="MsoPlainText">Krzysztof Parzyszek <a
href="mailto:kparzysz@quicinc.com" moz-do-not-send="true">
kparzysz@quicinc.com</a> AI tools development<o:p></o:p></p>
<p class="MsoPlainText"><o:p> </o:p></p>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<pre class="moz-quote-pre" wrap="">_______________________________________________
LLVM Developers mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
</pre>
</blockquote>
</body>
</html>