I just read in our Code Review policy, that contributors can also do the code reviews on the mailing lists. I checked the XXX-commits mailing list archives since Feb 1st 2021 and could not find any code review there. I only found automatic emails from Phabricator reviews or git commits.
Is anyone actually doing code reviews on the mailing list any more?
Can you send me a link to a recent example?
If nobody is doing code reviews on the mailing lists any more, we could also remove it from our policy…
It’s not uncommon to have email responses to phab emails that don’t make it into the web interface. A recent example is D99976.
There’s also a bunch of post commit discussion which happens entirely in email. Check any of the commit threads with responses in the last week. There are many.
for sharing this. That helped me better understand how people are interacting with the mailing lists!
It’s not uncommon to have email responses to phab emails that don’t make it into the web interface. A recent example is D99976.
I missed those when scanning the lists! I was looking for a code review that started on the mailing list, but that assumption was wrong.
So it looks like the information we have on the mailing list and in Phabricator is diverging, as those emails do not get parsed back into Phabricator. Which is unfortunate as folks not following the llvm-commits mailing lists (like me) will then not see discussions happening there.
However I do understand that it’s sometimes more convenient to reply via email client than via Phabricator. It looks like Phabricator would actually support this. Not sure how difficult it would be to set that up.
There’s also a bunch of post commit discussion which happens entirely in email. Check any of the commit threads with responses in the last week. There are many.
Same here: I missed the replies to commit messages. Here’s one example in case someone else is interested.
Code reviews don’t usually start on the mailing list.
Folks do use the mailing lists to reply to commit and review emails, so we still need those.
For the future we might want to look into parsing these replies back into Phabricator to get a consistent view there. Not sure how hard that would be to set up.
JFYI, our copy of phabricator carries a bunch of custom patches for email integration. It’s an area we put a lot of effort into in the past. The case I pointed to below just happens to be one in the long tail where that integration didn’t catch the response.
If you want to propose this, please move it to a top level thread for visibility.
(My thoughts inline to help you refine your proposal.)
Phabricator is also fragile unfortunately. I find phabricators inline discussions to be very hard to follow for anything which becomes involved. As such, I tend to default to phab, but quickly move to email if discussion gets complicated.
Thanks for the feedback. I didn’t take accessibility concerns into account. I’ll make sure to mention it if I decide to create a proposal (which seems likely).
I think it makes sense to phase out the email review path as well - it is better to have a single way to do things, and Phabricator is where our center of gravity is.
Yeah, I can’t remember the last time I did a review by email. But I’m sure that they were replies to a commit that broke our bots, and not an actual review of a pull request.
Worse, I think if someone does a review by email and doesn’t copy me directly, I won’t even notice.
Especially for cases where a commit didn't go through review on Phabricator, it's probably common to react to it via e.g. a reply to the commit mail, as it both goes to the list and hopefully reaches the author that way.
As an interesting coincidence and data point, my sony.com email just got CC’ed to a new patch request, posted directly on llvm-commits. I’m guessing the email actually bounced or got stuck in some sort of moderation queue, as I haven’t seen it appear in the archives to link to it directly. Anyway, it appears the user didn’t know about Phabricator being the preferred method (not helped due to the previously mentioned documentation no doubt), so I’ve pointed them at it, along with the relevant llvm.org pages to help them out.
(+1 to the side point of switching entirely to Phabricator reviews by the way)
