RFC: Phabricator for patches and code review

[Austin Seipp]

I haven't looked deeply into Trac integration yet, but I believe this
should generally be possible. I'll probably pester the developers
about it later today. I'm glad people seem receptive to it.

I don't think Arcanist will be a barrier, actually. Here's what I
propose: we add arcanist and libphutil to the GHC repository as
submodules (perhaps under ./utils), and just have a top level script
to execute 'arc', e.g. './arc' just executes 'php
./utils/arcanist/bin/arc' or whatnot. Then any user can just checkout
GHC and use the './arc' command to submit diffs - no install
necessary, even on Windows. You'd just need PHP.

This also has the advantage we'll generally want Arcanist to be in
sync for all developers, and roughly in sync with the Phabricator
server. As me and Herbert are admins anyway, keeping the submodule up
to date once every week or two isn't a huge deal.

On Fri, Jun 6, 2014 at 4:54 AM, Simon Marlow <marlowsd at gmail.com> wrote:
> tl;dir I strongly support this, but for code review only, and only if we can
> integrate it well with Trac.
>
> Phabricator is what we use internally at Facebook, and it's a really good
> code review tool (better than github, IMO).  For one thing, you only get one
> email for a complete review, rather than one email for each comment(!).  The
> UI is clean and streamlined, and reviewing diffs is a pleasure.
>
> If done right, this could improve our workflow a lot.  If done wrong, it
> could just make things more complex - we'd want to steer people away from
> both github pull requests and attaching patches in Trac and towards
> Phabricator instead.  They have to install a local tool (arc), which could
> be a barrier to contribution.
>
> We would need some integration between Phabricator and Trac.  I don't know
> whether any of this exists, but for example we'd want to have a way to link
> to Trac tickets from Differential, and to have diffs automatically show up
> attached to Trac tickets when they are linked to the ticket (by mentioning
> the ticket in the commit message, for example).
>
> So I think this is a lot of work, but if someone were prepared to do it then
> I think it could improve our lives.
>
> Cheers,
> Simon
>
>
> On 06/06/2014 05:05, Austin Seipp wrote:
>>
>> Hello all,
>>
>> Recently, while doing server maintenance, several of the
>> administrators for Haskell.org set up an instance of Phabricator[1],
>> located at https://phabricator.haskell.org
>>
>> For those who aren't aware, Phabricator (or "Phab") is a suite of
>> tools for software development. Think of it like a polished,
>> semi-private GitHub with a lot of applications and tools for all kinds
>> of needs. We've been using it to do issue tracking for Haskell.org
>> maintenance and like it a lot so far.
>>
>> One very nice aspect of Phabricator though is it has a very nice code
>> review tool, called 'Differential', that is very useful. For people
>> who have used a tool like Review Board, it's similar. Furthermore, it
>> has a very convenient userland tool called 'Arcanist' which makes it
>> easy for newcomers to post a review and get it merged when it's ready
>> all from the command line.
>>
>> I'd like to see if people are interested in using Phab _strictly_ for
>> code review of GHC patches. It is a dedicated tool specifically for
>> this, and I think it works much better than Trac or inline GitHub
>> comments.
>>
>> Also, Phab can also support post-commit reviews. So if I touch
>> something in the runtime system and just push, perhaps Simon or Edward
>> would like to look, and they can be alerted right when I do this, and
>> then yell if I did something stupid.
>>
>> Before I go much further, I'd like to ask: is there *any* interest in
>> this? Or are people satisifed with Trac? The primary motivations are
>> roughly, in no particular order:
>>
>>   1) Code review is good for everyone, a good way for people to learn
>> the code and ask questions, and useful to give feedback to newcomers.
>> And even experienced GHC hackers can learn things from reading code,
>> as we all do regularly, or find things that need cleanup.
>>
>>   2) Phabricator in particular makes it very easy to submit patches for
>> review. To submit a patch, I just run the command 'arc diff' and it
>> Does The Right Thing. It also makes it easy to ensure people are
>> *alerted* when a patch might be relevant to them.
>>
>>   3) They can be uploaded and created from the command line, and merged
>> easily afterwords the same way. This is particularly useful for
>> newcomers, and for me. :)
>>
>>   4) Differential is dedicated to code review, and much better at it
>> than just reading patches on Trac IMO.
>>
>>   5) It supports both post-commit code review, as well as pre-commit
>> review. Post commit would be especially useful for us too, I think.
>>
>> Point #2 and #3 are mostly relevant for me, because I mostly handle
>> incoming patches. But I think in general it would be nice, and make it
>> a lot easier for newcomers to submit patches, and us to look over
>> them.
>>
>> Here's an example of a Differential code review:
>>
>> https://phabricator.haskell.org/D4
>>
>> This is a demo using my 'wip/ermsb' patch. You'll need to create an
>> account to login, but it shouldn't be much trouble, you can login
>> several ways. I'll fix the login requirement soon. Feel free to read
>> the code, comment on it, and play around. It's more of a
>> demonstration, but real code review would be welcome too. :)
>>
>> If people are interested in doing this, I can add notes to the wiki
>> pages for newcomers, and I'll send another email about Phab so people
>> can understand it a little better. But I want to ask first.
>>
>> There is an argument that our team is so small, code review has
>> unnecessary burdens. But I think Phab could help a lot with tracking
>> outside patches and getting good reviews for incoming patches, and
>> it'll make it easier for newcomers. And experienced pros can probably
>> learn a thing as well.
>>
>> Again, to be clear, I don't propose we migrate anything to Phabricator
>> from, say, Trac. There's no real pressure to do so and it would be
>> tons of work. I only propose we use it for code review, which is
>> perfectly fine, and how other projects like LLVM do code review (they
>> use Bugzilla).
>>
>> I also don't think the usage of Phabricator should be mandatory
>> (unless we decide that later because we like it), but I would like to
>> see people use it if possible.
>>
>> [1] http://phabricator.org
>>
>



-- 
Regards,

Austin Seipp, Haskell Consultant
Well-Typed LLP, http://www.well-typed.com/