Usability of phabricator review threads for non-phab-users

Specifically the problem I've been seeing is that people using the website are
unable to CC mailing list-based developers. As a result I don't get copied in on
responses to my review comments, and rarely get any kind of direct mail with
threading. You end up having to dig up historic responses in the mailing list
archive which becomes tedious.

Often the CC on website reviews will include arbitrary names of people who have
website accounts, while excluding the actual code owners and recent committers
who you'd expect would be relevant. This leads me to guess that the website is
actively blocking the email addresses of LLVM developers from getting added to
the CC list unless they open an account on the service.

In fact as far as I can tell, mailing list-based developers are *completely*
excluded from the CC list visible on the website. This creates a really poor
workflow with responses often getting missed, and the right people not seeing
patches (and conversely, it looks like people who aren't really relevant end up
getting pressured into reviewing a patch in some area).

+1

I've found this frustrating, especially coupled with the fact that folks' email addresses, phab usernames, and svn usernames are not always obviously related to each other.

+1

It would be nice to enforce (or very strongly suggest) a bijection on phab usernames and svn usernames, and then display them in the tool as something like: `jsmith2 "John Smith" <john@smith.com>` (for some hypothetical developer).

I don't think that enforcing username conventions is the right solution though. Personally I think the easiest and most convenient thing is to just be able to use an email address and have phab treat that interchangeably with the phab username (or even as the canonical name with the phab username as a convenient alias). I think this is what bugzilla does and it seems to work well.

+1

Specifically the problem I've been seeing is that people using the
website are unable to CC mailing list-based developers. As a result I don't
get copied in on responses to my review comments, and rarely get any kind
of direct mail with threading. You end up having to dig up historic
responses in the mailing list archive which becomes tedious.

Often the CC on website reviews will include arbitrary names of people
who have website accounts, while excluding the actual code owners and
recent committers who you'd expect would be relevant. This leads me to
guess that the website is actively blocking the email addresses of LLVM
developers from getting added to the CC list unless they open an account on
the service.

To back this up, I get about a dozen mails a month saying "I can't find
you on Phabricator", to which I usually reply "Just enter my committer name
/ email address".

AFAICT people rarely do that, or the site blocks the email address and
tries to make me create an account which I'm not planning to do at present.

Crazy idea, but we already have an adjunct service that we all successfully
use side-by-side with the mailing lists: bugzilla. Why not have a "log in
with bugzilla credentials" option? It might be easy to implement by just
having Phab piggyback off of bugzilla's auth cookie.

-- Sean Silva

Thanks everybody for taking the time to report the issues. I’ll keep this thread updated while we work through them, so you have a convenient place to mute it in your favorite email client if you’re uninterested.

+1

I’d like single sign on for all LLVM development. We’d likely have to rehost the servers to make that possible or do some OAuth thing.

Alex

Manuel Klimek wrote:

Alp noted that the current setup on how phab reviews land on the list
are not working for him. I'd be curious whether his setup is special, or
whether there are more widespread problems. If this is more widely
perceived as a problem, please speak up, and I'll make sure to
prioritize the fixes (note that this is unrelated to the "lost email"
problem - those are always highest priority and as far as I'm aware we
diagnosed and fixed all of them within 1-2 business days).

If you have the feeling that the phab email workflow makes it hard for
you to jump into reviews, keep track of reviews, or understand reviews
if you're not a phab user, please reply to this thread. You don't need
to provide details, "+1", "please fix", or "doesn't work well for me"
are all acceptable replies here - I want to get a feeling for the
magnitude of the problem.

I really don't like using phab. I don't like the lack attached patch files on the mailing list to do a normal review. I don't like going to the phab site and trying to add comments and it won't let me ... but only sometimes, and for no reason I can see. I hate the patch view in phab, the side-by-side view doesn't fit 80 chars of text in my browser for either side leading to wrapping.

The emails with comments that other people have made are nice.

I did recently discover that it has a dashboard of reviews addressed to me. If that's accurate, that may change my workflow going forward. Today I still rely on people to ping their patches.

Nick

Wait what? The emails I get from phab *have* an attached patch file. That
was a hard requirement when we first set up Phabricator.

I have definitely run into this myself. IIRC, it seems to be when
there's an update to an existing phab review, it'll have the link to
phab, but no attached patch. This usually coincides with an
inexplicable thread split.

Some examples:

Re: [PATCH] Revert the lsda change to scan_eh_tab. (6/29)
Re: [PATCH] PR10405 Missing actual type (aka) in error message when
using decltype as a template parameter (5/29)
Re: [PATCH] Add a matcher for SubstNonTypeTemplateParmExpr. (6/29)

~Aaron

Chandler Carruth wrote:

    I don't like the lack attached patch files on the mailing list to do
    a normal review.

Wait what? The emails I get from phab *have* an attached patch file.
That was a hard requirement when we first set up Phabricator.

Aaron nailed it. The initial emails come with attached patches. The problem is when people comment with the changes they made to the code, but there's no updated patch attached to that email. Aaron found examples so I'll defer to those. I can also keep an eye out for the next time it happens if you want.

Nick

Chandler Carruth wrote:

    I don't like the lack attached patch files on the mailing list to do
    a normal review.

Wait what? The emails I get from phab *have* an attached patch file.
That was a hard requirement when we first set up Phabricator.

Aaron nailed it. The initial emails come with attached patches. The
problem is when people comment with the changes they made to the code, but
there's no updated patch attached to that email. Aaron found examples so
I'll defer to those. I can also keep an eye out for the next time it
happens if you want.

You should usually see 2 messages directly after each other - one with the
patch, and one with the comment updates.

Here’s an example of a use case that would be nice to fix:

http://reviews.llvm.org/D4425

It’s possible this has already been pointed out earlier in the thread. The situation was, I forgot to include lldb-commits on the original patch, and then added it subsequently. I could not find any way to get it to send out a new email containing the full patch + summary, so as a result I had to manually copy/paste the summary + patch text into an email response, and manually attach the patch as a file.

Manuel is planning to look into this but is on vacation so I just wanted to
follow up with a concrete suggestion:

If you are using Phabricator (which I still think is very useful), I think
it is important to actively look at the mailing list results. If you meant
to update the patch and the email didn't have one attached, reply to the
email with an attachment of the updated patch for folks to use.

While I'm looking forward to improvements that fix these issues, I still
find Phab very helpful as-is and just plan to observe and manually correct
any bad behavior on the email thread as that is (and should always be) the
canonical review log.

First, I think this is a great way to mitigate issues by taking the time to
flesh out the email thread when it doesn't get the right information on it.

Second, I've done this about 8 times now and found a (potentially) better
way of fixing it: abandon the review in phabricator, and create a new
review with the mailing list CC'ed. The result of this is:

1) The mailing list get's a fresh email with the right base information and
patch file attached.
2) Any specific reviewers CC'ed on the first review will get two emails,
but hopefully that's not too onerous.
2b) If you keep the subject exactly the same, then some mail readers will
(perhaps incorrectly, but usefully here) fold this into a single email
thread.

Hope that helps folks out when compensating for human errors here that
result in bad behavior of the tools.
-Chandler

Digging up this old thread because I thought of another use case that would be nice to support. I would like to be able to attach files generated with git format-patch to Phabricator reviews. I guess it chokes on the header information though and rejects the patch as invalid.

Hi Zachary,

have you tried that recently? If yes, can you re-open
https://secure.phabricator.com/D8547

(because that’s marked as fixed upstream)

Thanks,
/Manuel