[llvm-dev] Anyone doing code reviews via mailing lists?
Christian Kühnel via llvm-dev
Philip Reames via llvm-dev
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.
Philip
_______________________________________________ LLVM Developers mailing list llvm...@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Christian Kühnel via llvm-dev
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.
- 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.
Philip Reames via llvm-dev
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.
Philip
Hubert Tong via llvm-dev
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.
Mehdi AMINI via llvm-dev
Krzysztof Parzyszek via llvm-dev
I think we should phase-out email reviews in favor of doing it directly via Phabricator.
My reasons:
- Phabricator allows for both pre- and post-commit reviews. You can “raise concern” with any commit, including those that did not have a pre-commit review on phab.
- The email-phabricator integration is still deficient despite a lot of effort having been put into it. Moreover, it’s not likely that it will ever be fully functional.
- Email communication is “fragile”. This one is based more on my personal experience, but even simply following a discussion on llvm-dev has been difficult. I now have to use Outlook (corporate reasons…) and Outlook fails at keeping email threads together. Replies to the same tread are scattered into a mini-forest instead staying as a single tree. There are issues with every email client formatting the replies differently: top-post mixed with bottom-post mixed with inline text, with people quoting 500 lines of text only to insert a single-line response, and so on, and so forth. Some of it is due to my use of Outlook, some of it is independent from it. This “infinite flexibility” of email structure is the reason why I doubt that the Phabricator integration will ever work.
- Phabricator’s interface makes every review look the same, is readable, and doesn’t make it easy to unintentionally clutter it with junk.
- Finally, it seems like nowadays it’s easier to create a Phabricator account than to sign up to the mailing lists…
--
Krzysztof Parzyszek kpar...@quicinc.com AI tools development
Philip Reames via llvm-dev
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.)
I think we should phase-out email reviews in favor of doing it directly via Phabricator.
My reasons:
- Phabricator allows for both pre- and post-commit reviews. You can “raise concern” with any commit, including those that did not have a pre-commit review on phab.
- The email-phabricator integration is still deficient despite a lot of effort having been put into it. Moreover, it’s not likely that it will ever be fully functional.
- Email communication is “fragile”. This one is based more on my personal experience, but even simply following a discussion on llvm-dev has been difficult. I now have to use Outlook (corporate reasons…) and Outlook fails at keeping email threads together. Replies to the same tread are scattered into a mini-forest instead staying as a single tree. There are issues with every email client formatting the replies differently: top-post mixed with bottom-post mixed with inline text, with people quoting 500 lines of text only to insert a single-line response, and so on, and so forth. Some of it is due to my use of Outlook, some of it is independent from it. This “infinite flexibility” of email structure is the reason why I doubt that the Phabricator integration will ever work.
- Phabricator’s interface makes every review look the same, is readable, and doesn’t make it easy to unintentionally clutter it with junk.
As someone with vision restrictions, please be careful about this
line of argument. One of the major advantages of email is that I
can use my own client at whatever zoom/scale I want. Phab "sorta
works" from an accessibility perspective, but frankly is inferior
to plain old email. I frequently end up reading phabricator
emails, and then replying through the web interface.
I'm just point this out because I find that visual appear is often rated much more highly by some folks than others. Personally, functionality is pretty much the only thing I care about.
- Finally, it seems like nowadays it’s easier to create a Phabricator account than to sign up to the mailing lists…
--
Krzysztof Parzyszek kpar...@quicinc.com AI tools development
From: llvm-dev <llvm-dev...@lists.llvm.org> On Behalf Of Mehdi AMINI via llvm-dev
Sent: Friday, April 23, 2021 11:01 AM
To: Hubert Tong <hubert.rein...@gmail.com>
Cc: llvm-dev <llvm...@lists.llvm.org>
Subject: [EXT] Re: [llvm-dev] Anyone doing code reviews via mailing lists?
On Fri, Apr 23, 2021 at 8:57 AM Hubert Tong via llvm-dev <llvm...@lists.llvm.org> wrote:
On Fri, Apr 23, 2021 at 11:25 AM Christian Kühnel via llvm-dev <llvm...@lists.llvm.org> wrote:
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.
I recall noticing that Phabricator also doesn't emit inline code change suggestions into the e-mail record.
Yes: which is an indication that not all the content is on the mailing-list either :(
--
Mehdi
Krzysztof Parzyszek via llvm-dev
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).
Chris Lattner via llvm-dev
Renato Golin via llvm-dev
Martin Storsjö via llvm-dev
> 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.
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.
// Martin
James Henderson via llvm-dev
Krzysztof Parzyszek via llvm-dev
Just FYI, we are working on an official proposal to deprecate email reviews.
From: llvm-dev <llvm-dev...@lists.llvm.org> On Behalf Of
James Henderson via llvm-dev
Sent: Monday, April 26, 2021 3:01 AM
To: Martin Storsjö <mar...@martin.st>
Cc: llvm-dev <llvm...@lists.llvm.org>
Subject: [EXT] Re: [llvm-dev] Anyone doing code reviews via mailing lists?
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.