<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@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;}
/* Style Definitions */
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;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
font-size:10.0pt;
font-family:"Courier New";}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0in;
margin-right:0in;
margin-bottom:0in;
margin-left:.5in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.PlainTextChar
{mso-style-name:"Plain Text Char";
mso-style-priority:99;
mso-style-link:"Plain Text";
font-family:Consolas;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;}
span.EmailStyle23
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:16348053;
mso-list-template-ids:-431868154;}
@list l0:level1
{mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level2
{mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level3
{mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level4
{mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level5
{mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level6
{mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level7
{mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level8
{mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level9
{mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1
{mso-list-id:211894339;
mso-list-template-ids:599848150;}
@list l1:level1
{mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level2
{mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level3
{mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level4
{mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level5
{mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level6
{mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level7
{mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level8
{mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level9
{mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2
{mso-list-id:710689788;
mso-list-type:hybrid;
mso-list-template-ids:801288930 67698703 67698713 67698715 67698703 67698713 67698715 67698703 67698713 67698715;}
@list l2:level1
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level2
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level3
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l2:level4
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level5
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level6
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l2:level7
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level8
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level9
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l3
{mso-list-id:1288269876;
mso-list-template-ids:-1433732354;}
@list l3:level1
{mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l3:level2
{mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l3:level3
{mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l3:level4
{mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l3:level5
{mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l3:level6
{mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l3:level7
{mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l3:level8
{mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l3:level9
{mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l4
{mso-list-id:1732537191;
mso-list-template-ids:1531375384;}
@list l4:level1
{mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l4:level2
{mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l4:level3
{mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l4:level4
{mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l4:level5
{mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l4:level6
{mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l4:level7
{mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l4:level8
{mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l4:level9
{mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;}
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]-->
</head>
<body lang="EN-US" link="#0563C1" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal">I’m opposed to separating the pre- and post-commit reviews. One of the goals of this proposal is to have the entire review history in one place, and using a combination of email and Phabricator would prevent that. If I want to find out
why a commit has been reverted, I have to search the post-commit emails to see what happened. I guess one could argue that pre- and post-commit reviews could happen on different pages (Dxxx vs rGxxx), but, in my view, that is still better than emails. The
two concerns I have about post-commit reviews on Phabricator are that<o:p></o:p></p>
<ol style="margin-top:0in" start="1" type="1">
<li class="MsoListParagraph" style="margin-left:0in;mso-list:l2 level1 lfo5">People will keep doing it via email, because there is no mechanism (short of making the -commits lists read-only) that would actively inform them about the change in policy.<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0in;mso-list:l2 level1 lfo5">It takes a bit of manual work to get to the rG page for the commit, and if the commit message doesn’t include the link to the pre-commit review, the corresponding D page may be hard
to locate.<o:p></o:p></li></ol>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Regarding point (2), I’m hoping that something can be done in Phabricator to make the post-commit reviews easier: it may be as simple as sending the commit message to a list of subscribers.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Consolas">-- </span>
<span style="font-size:9.0pt;font-family:Consolas"><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:8.0pt;font-family:Consolas">Krzysztof Parzyszek
<a href="mailto:kparzysz@quicinc.com">kparzysz@quicinc.com</a> AI tools development<o:p></o:p></span></p>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> Philip Reames <listmail@philipreames.com> <br>
<b>Sent:</b> Monday, May 3, 2021 5:08 PM<br>
<b>To:</b> Krzysztof Parzyszek <kparzysz@quicinc.com>; llvm-dev <llvm-dev@lists.llvm.org><br>
<b>Cc:</b> clangd-dev@lists.llvm.org; openmp-dev@lists.llvm.org; lldb-dev@lists.llvm.org; cfe-dev@lists.llvm.org; libcxx-dev@lists.llvm.org; flang-dev@lists.llvm.org; parallel_libs-dev@lists.llvm.org<br>
<b>Subject:</b> [EXT] Re: [llvm-dev] [RFC] Deprecate email code reviews in favor of Phabricator<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p>In my view, this email is really too different topics. Given that, my response is split into two parts.<o:p></o:p></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.
<o:p></o:p></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.
<o:p></o:p></p>
<p>Philip<o:p></o:p></p>
<div>
<p class="MsoNormal">On 5/3/2021 10:24 AM, Krzysztof Parzyszek via llvm-dev wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoPlainText"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoPlainText"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Statement:</span></b><o:p></o:p></p>
<p class="MsoPlainText"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Our current code review policy states[1]:</span><o:p></o:p></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.”</span><o:p></o:p></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:</span><o:p></o:p></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).”</span><o:p></o:p></p>
<p class="MsoPlainText"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoPlainText"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Current situation:</span></b><o:p></o:p></p>
<ol style="margin-top:0in" start="1" type="1">
<li class="MsoPlainText" style="mso-list:l3 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.)</span><o:p></o:p></li><li class="MsoPlainText" style="mso-list:l3 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.</span><o:p></o:p></li><li class="MsoPlainText" style="mso-list:l3 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.</span><o:p></o:p></li></ol>
<p class="MsoPlainText"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoPlainText"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Benefits:</span></b><o:p></o:p></p>
<ol style="margin-top:0in" start="1" type="1">
<li class="MsoPlainText" style="mso-list:l4 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.</span><o:p></o:p></li><li class="MsoPlainText" style="mso-list:l4 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.</span><o:p></o:p></li><li class="MsoPlainText" style="mso-list:l4 level1 lfo2"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Local Phabricator extensions would no longer be needed.</span><o:p></o:p></li></ol>
<p class="MsoPlainText"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoPlainText"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Concerns:</span></b><o:p></o:p></p>
<ol style="margin-top:0in" start="1" type="1">
<li class="MsoPlainText" style="mso-list:l0 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">https://reviews.llvm.org/rG06234f758e19</a>). Those are communicated (perhaps ironically) via email, which implies that those automatic emails should remain in place.</span><o:p></o:p></li><li class="MsoPlainText" style="mso-list:l0 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.</span><o:p></o:p></li><li class="MsoPlainText" style="mso-list:l0 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.</span><o:p></o:p></li></ol>
<p class="MsoPlainText"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoPlainText"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Potential future direction:</span></b><o:p></o:p></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>
<ol start="1" type="1">
<li class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1 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:l1 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:l1 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:l1 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:l1 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:l1 level1 lfo4">
The timeline for the migration is to be defined.<o:p></o:p></li></ol>
<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"> </span><o:p></o:p></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">
https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review</a></span><o:p></o:p></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">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html</a></span><o:p></o:p></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">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html</a></span><o:p></o:p></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">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html</a></span><o:p></o:p></p>
<p class="MsoNormal">[5] <a href="https://reviews.llvm.org/project/view/104/">https://reviews.llvm.org/project/view/104/</a><o:p></o:p></p>
<p class="MsoNormal">[6] <a href="https://reviews.llvm.org/D101432">https://reviews.llvm.org/D101432</a><o:p></o:p></p>
<p class="MsoNormal">[7] <a href="https://reviews.llvm.org/H769">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"> </span><o:p></o:p></p>
<p class="MsoPlainText"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoPlainText">-- <o:p></o:p></p>
<p class="MsoPlainText">Krzysztof Parzyszek <a href="mailto:kparzysz@quicinc.com">
kparzysz@quicinc.com</a> AI tools development<o:p></o:p></p>
<p class="MsoPlainText"> <o:p></o:p></p>
<p class="MsoNormal"><br>
<br>
<o:p></o:p></p>
<pre>_______________________________________________<o:p></o:p></pre>
<pre>LLVM Developers mailing list<o:p></o:p></pre>
<pre><a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a><o:p></o:p></pre>
<pre><a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><o:p></o:p></pre>
</blockquote>
</div>
</body>
</html>