<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 3, 2021 at 3:07 PM Philip Reames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p>In my view, this email is really too different topics. Given
that, my response is split into two parts.</p></div></blockquote><div>I agree: in particular our code review process seems a bit disjoint from the "I want to keep a feed of the project activity" that the commit lists are offering. Seems best to me to look at these two considerations separately.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div>
<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. </p></div></blockquote><div>In general I do all of my post-commit review by commenting on the original revision. This has the nice side-effect that it allows me to do it without being subscribed to a commit mailing-list (I'm subscribed to llvm-commits@ but not to *all* the project specific ones). Phab also has the ability to comment on commits themselves, so that it works even when someone skipped the review, but the advantage of the original review is that all the reviewers/subscribers get CC'd on your post-commit review and this keeps a single thread of discussion.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div><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. </p></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div><p> <br>
</p>
<p>Philip<br>
</p>
<div>On 5/3/2021 10:24 AM, Krzysztof
Parzyszek via llvm-dev wrote:<br>
</div>
<blockquote type="cite">
<div>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif"><u></u> <u></u></span></p>
<p><b><span style="font-size:11pt;font-family:Calibri,sans-serif">Statement:<u></u><u></u></span></b></p>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif">Our
current code review policy states[1]:<u></u><u></u></span></p>
<p style="margin-left:0.5in"><span style="font-size:11pt;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.”<u></u><u></u></span></p>
<p><span style="font-size:11pt;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:<u></u><u></u></span></p>
<p style="margin-left:0.5in"><span style="font-size:11pt;font-family:Calibri,sans-serif">“Code
reviews are conducted on our web-based code-review tool (see
Code Reviews with Phabricator).”<u></u><u></u></span></p>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif"><u></u> <u></u></span></p>
<p><b><span style="font-size:11pt;font-family:Calibri,sans-serif">Current
situation:<u></u><u></u></span></b></p>
<ol style="margin-top:0in" type="1" start="1">
<li><span style="font-size:11pt;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.)<u></u><u></u></span></li>
<li><span style="font-size:11pt;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.<u></u><u></u></span></li>
<li><span style="font-size:11pt;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.<u></u><u></u></span></li>
</ol>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif"><u></u> <u></u></span></p>
<p><b><span style="font-size:11pt;font-family:Calibri,sans-serif">Benefits:<u></u><u></u></span></b></p>
<ol style="margin-top:0in" type="1" start="1">
<li><span style="font-size:11pt;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.<u></u><u></u></span></li>
<li><span style="font-size:11pt;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.<u></u><u></u></span></li>
<li><span style="font-size:11pt;font-family:Calibri,sans-serif">Local
Phabricator extensions would no longer be needed.<u></u><u></u></span></li>
</ol>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif"><u></u> <u></u></span></p>
<p><b><span style="font-size:11pt;font-family:Calibri,sans-serif">Concerns:<u></u><u></u></span></b></p>
<ol style="margin-top:0in" type="1" start="1">
<li><span style="font-size:11pt;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" target="_blank">https://reviews.llvm.org/rG06234f758e19</a>).
Those are communicated (perhaps ironically) via email,
which implies that those automatic emails should remain in
place.<u></u><u></u></span></li>
<li><span style="font-size:11pt;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.<u></u><u></u></span></li>
<li><span style="font-size:11pt;font-family:Calibri,sans-serif">Because
of the larger variety, email clients may offer better
accessibility options than web browsers.<u></u><u></u></span></li>
</ol>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif"><u></u> <u></u></span></p>
<p><b><span style="font-size:11pt;font-family:Calibri,sans-serif">Potential
future direction:<u></u><u></u></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:<u></u><u></u></p>
<ul type="disc">
<li class="MsoNormal">
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. <u></u><u></u></li>
<li class="MsoNormal">
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].<u></u><u></u></li>
<li class="MsoNormal">
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.<u></u><u></u></li>
<li class="MsoNormal">
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.<u></u><u></u></li>
<li class="MsoNormal">
The current XXX-commits mailing lists will be shut down<u></u><u></u></li>
<li class="MsoNormal">
The timeline for the migration is to be defined.<u></u><u></u></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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif"><u></u> <u></u></span></p>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif">[1]
<a href="https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review" target="_blank">
https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review</a><u></u><u></u></span></p>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif">[2]
<a href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html" target="_blank">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html</a><u></u><u></u></span></p>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif">[3]
<a href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html" target="_blank">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html</a><u></u><u></u></span></p>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif">[4]
<a href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html" target="_blank">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html</a><u></u><u></u></span></p>
<p class="MsoNormal">[5] <a href="https://reviews.llvm.org/project/view/104/" target="_blank">https://reviews.llvm.org/project/view/104/</a><u></u><u></u></p>
<p class="MsoNormal">[6] <a href="https://reviews.llvm.org/D101432" target="_blank">https://reviews.llvm.org/D101432</a><u></u><u></u></p>
<p class="MsoNormal">[7] <a href="https://reviews.llvm.org/H769" target="_blank">https://reviews.llvm.org/H769</a><u></u><u></u></p>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif"><u></u> <u></u></span></p>
<p><span style="font-size:11pt;font-family:Calibri,sans-serif"><u></u> <u></u></span></p>
<p>-- <u></u><u></u></p>
<p>Krzysztof Parzyszek <a href="mailto:kparzysz@quicinc.com" target="_blank">
kparzysz@quicinc.com</a> AI tools development<u></u><u></u></p>
<p><u></u> <u></u></p>
</div>
<br>
<fieldset></fieldset>
<pre>_______________________________________________
LLVM Developers mailing list
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
</pre>
</blockquote>
</div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>