Phabricator for patches and code review

Austin Seipp austin at well-typed.com
Fri Jun 6 07:04:27 UTC 2014


On Fri, Jun 6, 2014 at 1:28 AM, Simon Peyton Jones
<simonpj at microsoft.com> wrote:
> So we really don't have a good work flow for creating, reviewing, modifying, and finally apply patches.  I am no expert on these matters. If Phabricator would help with that I'm all for it.  But perhaps there are other alternatives?  Or is Phab the lead thing.  Will it stay around?

Yes, that's right - I think it will really help the whole workflow.

As for Phabricator: it is used by several very large companies,
including Facebook (so Simon might feel at home at least ;), it is
actively developed by a team, and the Haskell.org admin team wants to
keep using it. I don't think there are any plans for it to go away at
this point, but it's still new.

There are some other projects, like Review Board, but that leads me to...

> Also before going too far I'd really like someone to document the workflow carefully, and make sure it works from Windows equally well.

Good news: the workflow is quite easy at first, you can submit patches
from the command line, and yes, the command line tool works on
Windows! I *specifically* wanted this when choosing it.

I can write up some documents discussing the basic idea. At my last
job, we fiddled endlessly with Review Board for basic stuff like
"create a review from the commit I just made". Being able to submit
and interate patches so quickly is a huge, massive time saver for
everyone.

> I'm not too stressed out about losing the review trail of a patch.  Much of it will be commenting on stuff that no longer appears in the final patch.  Anything that's important should appear in a Note in the source code; even the commit messages are invisible until you really start digging.

Right. Actually, by default, when you merge in a set of changes using
the Phabricator tool (which is mostly what I will do), it squashes
them into a single commit, and adds a message pointing back to the
review. So the commit is self contained, logical, and small, but you
still get the full breadcrumbs, at least.

> Simon
>
> | -----Original Message-----
> | From: ghc-devs [mailto:ghc-devs-bounces at haskell.org] On Behalf Of Austin
> | Seipp
> | Sent: 06 June 2014 05:06
> | To: ghc-devs at haskell.org
> | Subject: RFC: Phabricator for patches and code review
> |
> | 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/
> | _______________________________________________
> | ghc-devs mailing list
> | ghc-devs at haskell.org
> | http://www.haskell.org/mailman/listinfo/ghc-devs
>



-- 
Regards,

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


More information about the ghc-devs mailing list