Announcement: Phabricator for code reviews

Dear LLVM / Clang community,

we’d like to open the use of Phabricator as an optional tool for doing code reviews to a wider audience. Please feel free to start your code reviews by following the documentation at http://llvm.org/docs/Phabricator.html.

Note that e-mail is still the reference medium for code reviews. Please let me know about any problems with Phabricator or the documentation you find.

Cheers,
/Manuel

You might want to mention other functionality, like Herald, which I
think fills a need that a lot of developers have.

Well, I guess since this message is on the list, I can just say:
Phabricator's "Herald" tool lets you (among other things) set up
actions to happen in response to certain events; one huge use case for
this is setting up alerts when parts of the tree that you are
interested in watching get changed: this alleviates the problem of
missing the chance to post-commit review on changes due to messages
getting lost in the flood of llvm-commits mails.

There's a lot of other functionality in Phabricator that I think might
be useful; the big question is how to have it integrate properly into
the mailing list---that's the reason I have held back in exploring
some of the features.

-- Sean Silva

You might want to mention other functionality, like Herald, which I
think fills a need that a lot of developers have.

Well, I guess since this message is on the list, I can just say:
Phabricator's "Herald" tool lets you (among other things) set up
actions to happen in response to certain events; one huge use case for
this is setting up alerts when parts of the tree that you are
interested in watching get changed: this alleviates the problem of
missing the chance to post-commit review on changes due to messages
getting lost in the flood of llvm-commits mails.

There's a lot of other functionality in Phabricator that I think might
be useful; the big question is how to have it integrate properly into
the mailing list---that's the reason I have held back in exploring
some of the features.

I'd say the biggest impact of a tool like Phabricator is for larger
pre-commit code reviews (for example by answering the question "what has
changed since the last patch"). Thus, that's the workflow we spent some
time tuning. If we see that more people use and like Phabricator, we'll put
more effort into making sure other features are integrated nicely with
llvm's existing workflows.

Cheers,
/Manuel

Still basically unusable with Apple Mail’s obstinate refusal to respect “In-reply-to” or “References”. I’m definitely giving Phabricator patches less attention because the CCs to cfe-commits are too hard to follow. I realize that you shouldn’t have to change a nice, standards-conforming tool to fit our closed-source app’s “feature”, but…

Any news on turning on auto-“Re” for every reply after the initial patch posting? I can’t think of any ill effects from that, and it matches what people do for mailing-list reviews anyway.

Thanks,
Jordan

I'd say the biggest impact of a tool like Phabricator is for larger
pre-commit code reviews (for example by answering the question "what has
changed since the last patch").

I was thinking of the other features as more of a "gateway drug" for
the improved code-review :wink:

-- Sean Silva

Still basically unusable with Apple Mail's obstinate refusal to respect
"In-reply-to" or "References". I'm definitely giving Phabricator patches
less attention because the CCs to cfe-commits are too hard to follow. I
realize that you shouldn't have to change a nice, standards-conforming tool
to fit our closed-source app's "feature", but...

Any news on turning on auto-"Re" for every reply *after* the initial
patch posting? I can't think of any ill effects from that, and it matches
what people do for mailing-list reviews anyway.

I just contacted the phab folks and implemented and deployed a fix.

Cheers,
/Manuel

Hi Manuel,

we'd like to open the use of Phabricator as an optional tool for doing code
reviews to a wider audience. Please feel free to start your code reviews by
following the documentation at http://llvm.org/docs/Phabricator.html.

sorry for the silly question but... how do you sign up? The "sign up" section
doesn't have a "sign up here" link. It does have a link to "Code Repository
Browser", which asks for a phabricator, github or google login. It looks like
you can register with phabricator using a github or google account; is that
required, or is it possible to sign up for phabricator without going via github
or google?

Thanks for your help, Duncan.

This looks great! One hitch though: it looks like I can monitor files
by name but what I really want to watch is a whole directory
"/llvm/trunk/include/llvm-c". I didn't see any way to do that. Do you
have any idea if that is possible? I can add a rule for each file
which is probably OK but then I'll miss any new files.

Best, Keith

Does that mean you don't think the filename being matched includes the
directory in any way? I'd assumed it did.

Tim.

Yes, that was my (maybe wrong) assumption. -Keith

Yeah, I think that is the problem. My rule for watching any TableGen file is

any changed filename -- contains -- TableGen
Author -- is not any of -- silvas

-- Sean Silva

Hi Duncan,

Because Facebook is for posting political rants and funny pictures of cats. :slight_smile:

Facebook is not the only OAuth provider though. We should be able to
support essentially any you would prefer if that's all. Manuel's comment
still stands if OAuth is a problem.

My point is that using an OAuth provider should be an option, not a de-facto requirement.

Facebook is not the only OAuth provider though. We should be able to
support essentially any you would prefer if that's all. Manuel's comment
still stands if OAuth is a problem.

My point is that using an OAuth provider should be an option, not a
de-facto requirement.

I hear you, but I'd be interested in why OAuth is a problem for you - as I
said, if we have good arguments, the phab guys are really quick to come up
with changes. I'm not deeply familiar with authentication schemes.

Thanks,
/Manuel

What I don't like about it is that those who do not have Facebook, Google or Github accounts will now need to get one in order to access service that's entirely unrelated to any of the account providers. Also, those people who do compiler work for a living may be required to follow certain protocols when communicating with the outside world. Tying it to Facebook or Google may simply not be an option.

-K

Manuel,

Thanks, I’ve created https://secure.phabricator.com/T1930.
Until that is resolved I’ll create accounts for anybody who doesn’t want to use OAuth - just shoot me a mail.

Cheers,
/Manuel

The privacy policies you would have to agree to are really scary, possibly even illegal in Europe.

It seems like a completely unnecessary obstacle to participating in code review.

Are you concerned about unauthorized reviews? Drive-by LGTMs?

/jakob