Phabricator for patches and code review

Mon Jun 23 14:44:23 UTC 2014

Hi all,

I went ahead and took some time to write some stuff down about
Phabricator, including some basic tips on the workflows and
applications here:

https://ghc.haskell.org/trac/ghc/wiki/Phabricator

It's definitely going to need more expanding. Do let me know if
anything is confusing.

On Wed, Jun 18, 2014 at 2:38 AM, Jan Stolarek <jan.stolarek at p.lodz.pl> wrote:
> Duh, ignore what I wrote. I just realized I'm working on a non-rebased branch :-)
>
> Janek
>
> Dnia środa, 18 czerwca 2014, Jan Stolarek napisał:
>> I read the friendly Arcanist manual and I wonder if we intend to have a
>> default .arcconfig file in the GHC repo? From the docs it seems like a good
>> idea.
>>
>> Janek
>>
>> Dnia wtorek, 17 czerwca 2014, Simon Marlow napisał:
>> > On 13/06/14 10:47, Jan Stolarek wrote:
>> > > It seems that most people are in favour of using Phabricator for code
>> > > review. So what are the next steps? Can we just start using the
>> > > existing phabricator instance? I'm working on some code right now that
>> > > definitely needs reviewing.
>> >
>> > You can use it, and a few of us have already been doing so.  There isn't
>> > any Trac integration yet, but it works nicely for patch review.
>> >
>> > There's a short intro doc here:
>> > https://secure.phabricator.com/book/phabricator/article/differential/,
>> > but it's not hard to figure out the basics, and you'll learn by watching
>> > how other people use it.  If you go to the Herald tool you have yourself
>> > automatically subscribed to diffs that touch areas of the code that
>> > you're interested in.
>> >
>> > Pro tip: the keyboard shortcuts are really useful, especially "z".  Hit
>> > "?" to see all the shortcuts.
>> >
>> > Cheers,
>> > Simon
>> >
>> > > Janek
>> > >
>> > > Dnia niedziela, 8 czerwca 2014, Simon Marlow napisał:
>> > >> On 07/06/2014 07:21, Manuel M T Chakravarty wrote:
>> > >>> So, why not put everything on GutHub and use pull requests and so on?
>> > >>
>> > >> github just isn't great for doing code reviews. No side-by-side diffs,
>> > >> and it sends you a separate email for every single comment, there's no
>> > >> concept of a "review" consisting of multiple inline comments (unless
>> > >> I've missed something). I'm afraid if we were using this for regular
>> > >> reviews I would have to disable the email notifications, which makes
>> > >> it significantly less useful.
>> > >>
>> > >>> SimonM writes that Phabricator is better than GitHub. I’m happy to
>> > >>> believe that, but he also writes that using it requires installing
>> > >>> local software and quite a bit of work. Moreover, I like to add that
>> > >>> lots of people already know how to use GitHub and probably few know
>> > >>> Phabricator.
>> > >>>
>> > >>> So, we are talking about having a somewhat better tool in return for
>> > >>> three very significant disadvantages: (1) local installation, (2)
>> > >>> work to set up and maintain Phabricator, and (3) effort by many
>> > >>> people to learn to use it.
>> > >>
>> > >> Well, you've tipped the balance with "somewhat" and "significant"
>> > >> here, I'd say Phabricator is "significantly" better than github for
>> > >> code reviews, while installing arc is "somewhat" annoying :-)
>> > >>
>> > >> I have to admit it's not a no-brainer, but I do worry that github just
>> > >> wouldn't cut it for doing a lot of code reviewing, whereas I spend my
>> > >> life inside Phabricator so I know it works really well.
>> > >>
>> > >> What's more, github doesn't let you put animated gifs in code reviews.
>> > >> Need I say more?
>> > >>
>> > >> Cheers,
>> > >> Simon
>> > >>
>> > >>> We also have a constant lack of sufficient men power. So, why spend
>> > >>> effort on building our own infrastructure, which will only increase
>> > >>> the hurdle for contributors (as they have to deal with an unknown
>> > >>> system)? Let’s outsource the effort to GitHub.
>> > >>>
>> > >>> Manuel
>> > >>>
>> > >>> Simon Peyton Jones <simonpj at microsoft.com>:
>> > >>>> At the moment GHC's main sources aren't on github, which means that
>> > >>>> that (in my highly imperfect understanding) people can't submit pull
>> > >>>> requests or use their code review mechanisms.  Moreover, most people
>> > >>>> don't have commit rights on the main GHC server, so if someone wants
>> > >>>> to offer a patch they can really only do so in textual form attached
>> > >>>> to Trac. People with commit rights can make a branch, but there's a
>> > >>>> danger that over a decade we'll accumulate zillions of dead branches
>> > >>>> which people forgot to delete.  I think on github the branch is in a
>> > >>>> different repo, belonging to the patch author.
>> > >>>>
>> > >>>> 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?
>> > >>>>
>> > >>>> Also before going too far I'd really like someone to document the
>> > >>>> workflow carefully, and make sure it works from Windows equally
>> > >>>> well.
>> > >>>>
>> > >>>> 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.
>> > >>>>
>> > >>>> 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
>> > >>>>
>> > >>>> _______________________________________________
>> > >>>> ghc-devs mailing list
>> > >>>> ghc-devs at haskell.org
>> > >>>> http://www.haskell.org/mailman/listinfo/ghc-devs
>> > >>>
>> > >>> _______________________________________________
>> > >>> ghc-devs mailing list
>> > >>> ghc-devs at haskell.org
>> > >>> http://www.haskell.org/mailman/listinfo/ghc-devs
>> > >>
>> > >> _______________________________________________
>> > >> ghc-devs mailing list
>> > >> ghc-devs at haskell.org
>> > >> http://www.haskell.org/mailman/listinfo/ghc-devs
>> > >
>> > > _______________________________________________
>> > > ghc-devs mailing list
>> > > ghc-devs at haskell.org
>> > > http://www.haskell.org/mailman/listinfo/ghc-devs
>>
>> _______________________________________________
>> 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/