Aaron Ballman via llvm-dev
2021-May-04 11:23 UTC
[llvm-dev] [cfe-dev] [RFC] Deprecate email code reviews in favor of Phabricator
On Mon, May 3, 2021 at 1:24 PM Krzysztof Parzyszek via cfe-dev <cfe-dev at lists.llvm.org> 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).”Personally, I am in favor of this policy for initiating code reviews, but am opposed to it for post-commit feedback. The problem, as I see it, is that not every change *gets* code review via Phab and the email lists are the only place on which to provide that post-commit feedback. This largely comes up in two ways: NFC changes and changes made by code owners in the area of the compiler which they own. We still need *some* mechanism by which to provide them post-commit feedback. Currently, the way we provide that is frequently via an email reply to the commit message on the *-commits list so that the issue can be seen by both the patch author and the community at large.> Current situation: > > 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.) > 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. > 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: > > 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. > Review authors and reviewers would only need to monitor one source of comments without the fear that a review comment may end up overlooked. > Local Phabricator extensions would no longer be needed. > > > > Concerns: > > 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.The Phab commit message does not have any subscribers though, and so my understanding is that comments on that Phab interface are not communicated out to the community as a whole, which means the community may miss out on important post-commit-review information like general awareness of the problem, workarounds people can use until the author corrects something, alternative ideas on how to fix the issue, etc. Or do I misunderstand the way Phab works in this workflow? Also, "the commenter would need to find the Phabricator commit" concerns me -- adding extra burden on the people providing feedback means that *some* amount of those people will struggle enough to simply not provide that feedback. Responding to an email is about as low as I think we can set that bar, so the current approach has better ergonomics for giving feedback. When I look at an existing NFC commit email (https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20210503/368413.html), I don't see any direct link back to the Phabricator commit, so I have to know to get the hash and try using that with an https://reviews.llvm.org/rG in front of it. None of the existing links in the email get me to where I'd need to go to add my review feedback, but hitting the Reply button in my mail client will work. Adding a third link and telling people "click on the link in the email" means they've got a 50/50 shot of getting the right link because one of the links goes to GitHub where you can also add comments.> 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. > 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].Tangential complaint -- our use of Herald causes some pain for me as a list moderator because we've reached the point where Herald automatically adds so many subscribers to a review that it gets marked as spam for every email that is generated for the review. It's to the point on cfe-dev where over half of the moderated emails are consistently not spam some weeks. This delays the community receiving the information while the patch reviewers/subscribers continue to get it. In turn, this causes a problem where sometimes the people subscribed to the patch say something is OK and the patch lands before the community ever sees the review and has a chance to comment on it. I'm wary of suggestions that involve heavier use of Herald until we get that mailing list issue resolved.> 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.Given how often Phabricator goes down (which is not super often, but often enough that people know it happens), I am deeply uncomfortable with the idea of shutting down the current *-commits mailing lists because of the chance for data loss. Personally, I think the *-commits lists are working well and I would prefer they be left alone. ~Aaron> > 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 kparzysz at quicinc.com AI tools development > > > > _______________________________________________ > cfe-dev mailing list > cfe-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Christian Kühnel via llvm-dev
2021-May-04 13:15 UTC
[llvm-dev] [cfe-dev] [RFC] Deprecate email code reviews in favor of Phabricator
Hi Aaron,> > “Code reviews are conducted on our web-based code-review tool (see Code > Reviews with Phabricator).” > > Personally, I am in favor of this policy for initiating code reviews, > but am opposed to it for post-commit feedback. The problem, as I see > it, is that not every change *gets* code review via Phab and the email > lists are the only place on which to provide that post-commit > feedback.You can set up notifications on commits in Phabricator. Phabricator adds the user "llvm-commits" as subscriber to certain reviews: https://reviews.llvm.org/H615 We could do the same thing for commits... So you can reply to any commit via the web UI (or email notification) and the author gets notified as well. One example that worked for me: https://reviews.llvm.org/rG7f9717b922d421c30f889034488563c67076aca1 The Phab commit message does not have any subscribers though, and so> my understanding is that comments on that Phab interface are not > communicated out to the community as a whole, which means the > community may miss out on important post-commit-review information > like general awareness of the problem, workarounds people can use > until the author corrects something, alternative ideas on how to fix > the issue, etc. Or do I misunderstand the way Phab works in this > workflow? >We can add automatically subscribers to commits as well if we want to. One random example where a subscriber was added to a commit via a Herald rule: https://reviews.llvm.org/rG93537fabcee8fcfa3316d7abd5e935f7fe9b468f> Also, "the commenter would need to find the Phabricator commit" > concerns me -- adding extra burden on the people providing feedback > means that *some* amount of those people will struggle enough to > simply not provide that feedback. Responding to an email is about as > low as I think we can set that bar, so the current approach has better > ergonomics for giving feedback.If we set up the notifications via Phabricator, you can reply via email. These contain a link at the bottom that will take you directly to the commit page in Phabricator. Not sure why we don't have these on the mailing list...> Tangential complaint -- our use of Herald causes some pain for me as a > list moderator because we've reached the point where Herald > automatically adds so many subscribers to a review that it gets marked > as spam for every email that is generated for the review. >So this would be a reason to replace the XXX-commits mailing lists with something else...> Given how often Phabricator goes down (which is not super often, but > often enough that people know it happens), I am deeply uncomfortable > with the idea of shutting down the current *-commits mailing lists > because of the chance for data loss. Personally, I think the *-commits > lists are working well and I would prefer they be left alone. >If Phabricator is down, you're not getting any email notifications from it anyway. So we might already be losing data right now... But fair point: The more we rely on Phabricator, the higher the reliability requirements. Best, Christian -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210504/4a73cdf4/attachment.html>
Krzysztof Parzyszek via llvm-dev
2021-May-04 13:50 UTC
[llvm-dev] [cfe-dev] [RFC] Deprecate email code reviews in favor of Phabricator
You're right that doing post-commit reviews on Phabricator is not seamless---the rG link is not included anywhere. Hopefully that could be fixed with some Phabricator configuration tweaks, like sending the commit email to the -commits list. I'm not sure if there is a general fix for the spam issue. I have had this problem as well with the existing lists, so it's not limited to Phabricator. In our spam filter I have whitelisted enough emails to avoid the unnecessary filtering almost completely, but it is an issue nevertheless. -- Krzysztof Parzyszek kparzysz at quicinc.com AI tools development -----Original Message----- From: Aaron Ballman <aaron at aaronballman.com> Sent: Tuesday, May 4, 2021 6:24 AM To: Krzysztof Parzyszek <kparzysz at quicinc.com> Cc: llvm-dev <llvm-dev at lists.llvm.org>; clangd-dev at lists.llvm.org; openmp-dev at lists.llvm.org; lldb-dev at lists.llvm.org; cfe-dev at lists.llvm.org; libcxx-dev at lists.llvm.org; flang-dev at lists.llvm.org; parallel_libs-dev at lists.llvm.org Subject: [EXT] Re: [cfe-dev] [RFC] Deprecate email code reviews in favor of Phabricator On Mon, May 3, 2021 at 1:24 PM Krzysztof Parzyszek via cfe-dev <cfe-dev at lists.llvm.org> 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).”Personally, I am in favor of this policy for initiating code reviews, but am opposed to it for post-commit feedback. The problem, as I see it, is that not every change *gets* code review via Phab and the email lists are the only place on which to provide that post-commit feedback. This largely comes up in two ways: NFC changes and changes made by code owners in the area of the compiler which they own. We still need *some* mechanism by which to provide them post-commit feedback. Currently, the way we provide that is frequently via an email reply to the commit message on the *-commits list so that the issue can be seen by both the patch author and the community at large.> Current situation: > > 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.) 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. > 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: > > 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. > Review authors and reviewers would only need to monitor one source of comments without the fear that a review comment may end up overlooked. > Local Phabricator extensions would no longer be needed. > > > > Concerns: > > 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.The Phab commit message does not have any subscribers though, and so my understanding is that comments on that Phab interface are not communicated out to the community as a whole, which means the community may miss out on important post-commit-review information like general awareness of the problem, workarounds people can use until the author corrects something, alternative ideas on how to fix the issue, etc. Or do I misunderstand the way Phab works in this workflow? Also, "the commenter would need to find the Phabricator commit" concerns me -- adding extra burden on the people providing feedback means that *some* amount of those people will struggle enough to simply not provide that feedback. Responding to an email is about as low as I think we can set that bar, so the current approach has better ergonomics for giving feedback. When I look at an existing NFC commit email (https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20210503/368413.html), I don't see any direct link back to the Phabricator commit, so I have to know to get the hash and try using that with an https://reviews.llvm.org/rG in front of it. None of the existing links in the email get me to where I'd need to go to add my review feedback, but hitting the Reply button in my mail client will work. Adding a third link and telling people "click on the link in the email" means they've got a 50/50 shot of getting the right link because one of the links goes to GitHub where you can also add comments.> 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. > 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].Tangential complaint -- our use of Herald causes some pain for me as a list moderator because we've reached the point where Herald automatically adds so many subscribers to a review that it gets marked as spam for every email that is generated for the review. It's to the point on cfe-dev where over half of the moderated emails are consistently not spam some weeks. This delays the community receiving the information while the patch reviewers/subscribers continue to get it. In turn, this causes a problem where sometimes the people subscribed to the patch say something is OK and the patch lands before the community ever sees the review and has a chance to comment on it. I'm wary of suggestions that involve heavier use of Herald until we get that mailing list issue resolved.> 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.Given how often Phabricator goes down (which is not super often, but often enough that people know it happens), I am deeply uncomfortable with the idea of shutting down the current *-commits mailing lists because of the chance for data loss. Personally, I think the *-commits lists are working well and I would prefer they be left alone. ~Aaron> > 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-rev > iew > > [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 kparzysz at quicinc.com AI tools development > > > > _______________________________________________ > cfe-dev mailing list > cfe-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Mehdi AMINI via llvm-dev
2021-May-05 00:34 UTC
[llvm-dev] [cfe-dev] [RFC] Deprecate email code reviews in favor of Phabricator
On Tue, May 4, 2021 at 4:24 AM Aaron Ballman via cfe-dev < cfe-dev at lists.llvm.org> wrote:> On Mon, May 3, 2021 at 1:24 PM Krzysztof Parzyszek via cfe-dev > <cfe-dev at lists.llvm.org> 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).” > > Personally, I am in favor of this policy for initiating code reviews, > but am opposed to it for post-commit feedback. The problem, as I see > it, is that not every change *gets* code review via Phab and the email > lists are the only place on which to provide that post-commit > feedback. This largely comes up in two ways: NFC changes and changes > made by code owners in the area of the compiler which they own. We > still need *some* mechanism by which to provide them post-commit > feedback. Currently, the way we provide that is frequently via an > email reply to the commit message on the *-commits list so that the > issue can be seen by both the patch author and the community at large. > > > Current situation: > > > > 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.) > > 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. > > 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: > > > > 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. > > Review authors and reviewers would only need to monitor one source of > comments without the fear that a review comment may end up overlooked. > > Local Phabricator extensions would no longer be needed. > > > > > > > > Concerns: > > > > 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. > > The Phab commit message does not have any subscribers though, and so > my understanding is that comments on that Phab interface are not > communicated out to the community as a whole, which means the > community may miss out on important post-commit-review information > like general awareness of the problem, workarounds people can use > until the author corrects something, alternative ideas on how to fix > the issue, etc. Or do I misunderstand the way Phab works in this > workflow? > > Also, "the commenter would need to find the Phabricator commit" > concerns me -- adding extra burden on the people providing feedback > means that *some* amount of those people will struggle enough to > simply not provide that feedback. Responding to an email is about as > low as I think we can set that bar, so the current approach has better > ergonomics for giving feedback. When I look at an existing NFC commit > email ( > https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20210503/368413.html > ), > I don't see any direct link back to the Phabricator commit, so I have > to know to get the hash and try using that with an > https://reviews.llvm.org/rG in front of it.This is a simple problem to solve by fixing the format of the emails that are sent, I believe we have control over this right now. We could: - add the https://reviews.llvm.org/rG<hash> to every email, and not to GitHub. - make sure that the lists are subscribed on the commits on Phab so that notifications are generated to the list on post-commit feedback.> None of the existing links > in the email get me to where I'd need to go to add my review feedback, > but hitting the Reply button in my mail client will work. Adding a > third link and telling people "click on the link in the email" means > they've got a 50/50 shot of getting the right link because one of the > links goes to GitHub where you can also add comments. > > > 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. > > 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]. > > Tangential complaint -- our use of Herald causes some pain for me as a > list moderator because we've reached the point where Herald > automatically adds so many subscribers to a review that it gets marked > as spam for every email that is generated for the review. It's to the > point on cfe-dev where over half of the moderated emails are > consistently not spam some weeks.I'm surprised that the reviews are going to cfe-dev@ and not cfe-commits@? But also, I didn't know that the mailman instance had a spam filter: I thought this was based purely on whether the sender is subscribed (or whitelisted)?> This delays the community receiving > the information while the patch reviewers/subscribers continue to get > it. In turn, this causes a problem where sometimes the people > subscribed to the patch say something is OK and the patch lands before > the community ever sees the review and has a chance to comment on it. > I'm wary of suggestions that involve heavier use of Herald until we > get that mailing list issue resolved. > > > 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. > > Given how often Phabricator goes down (which is not super often, but > often enough that people know it happens),Data-driven: I am aware of two outages for the last entire year, and each lasted for < 1/2 day.> I am deeply uncomfortable > with the idea of shutting down the current *-commits mailing lists > because of the chance for data loss. Personally, I think the *-commits > lists are working well and I would prefer they be left alone. > > ~Aaron > > > > > 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 kparzysz at quicinc.com AI tools development > > > > > > > > _______________________________________________ > > cfe-dev mailing list > > cfe-dev at lists.llvm.org > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev > _______________________________________________ > cfe-dev mailing list > cfe-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210504/f2b3bc34/attachment.html>