I’m not hearing any particular objection to moving to Phabricator-only for reviews. The trend has been in that direction for years anyway. The barriers for one-time contributors are (a) registration, and (b) unfamiliarity with a user-hostile web UI. Regarding registration, it’s either subscribing to llvm-commits or to Phab; and IIUC, Phab understands github IDs, which makes that barrier pretty low. Regarding the web UI, we have (or did have, admittedly I haven’t looked lately) step-by-step instructions in the docs for how to navigate Phab successfully, which mitigates (b) as much as we can.
I am in the camp of, why mess with llvm-commits? The commit emails that Phab sends out already have the commit URL in them right at the top, which looks like it goes to a page where comments can be added (I haven’t actually tried adding a comment). If those posted comments go to llvm-commits and the author, then I think the only necessary step to turn off email post-commit review is to make llvm-commits read-only except for Phabricator itself. We don’t need the ability to have email replies on llvm-commits be recorded in Phab, if we can almost as easily make comments on Phab that go to llvm-commits. Possibly I am overlooking some problem that abandoning llvm-commits entirely is going to solve? --paulr From: cfe-dev <cfe-dev-boun...@lists.llvm.org> On Behalf Of Christian Kühnel via cfe-dev Sent: Tuesday, May 4, 2021 9:15 AM To: Aaron Ballman <aa...@aaronballman.com> Cc: llvm-dev <llvm-...@lists.llvm.org>; clangd-...@lists.llvm.org; openmp-...@lists.llvm.org; lldb-dev@lists.llvm.org; cfe-...@lists.llvm.org; libcxx-...@lists.llvm.org; flang-...@lists.llvm.org; parallel_libs-...@lists.llvm.org Subject: Re: [cfe-dev] [llvm-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<https://urldefense.com/v3/__https:/reviews.llvm.org/H615__;!!JmoZiZGBv3RvKRSx!p37c8uYDXWnnl1JAGj92AupZsGAGPG0jsP8mxCiLLHtiY0ihBoKrW5hV90ZiYb2jOw$> 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<https://urldefense.com/v3/__https:/reviews.llvm.org/rG7f9717b922d421c30f889034488563c67076aca1__;!!JmoZiZGBv3RvKRSx!p37c8uYDXWnnl1JAGj92AupZsGAGPG0jsP8mxCiLLHtiY0ihBoKrW5hV90YQuXXWaw$> 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<https://urldefense.com/v3/__https:/reviews.llvm.org/rG93537fabcee8fcfa3316d7abd5e935f7fe9b468f__;!!JmoZiZGBv3RvKRSx!p37c8uYDXWnnl1JAGj92AupZsGAGPG0jsP8mxCiLLHtiY0ihBoKrW5hV90ZMMYsijg$> 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
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
