Phabricator for patches and code review

Wed Jun 18 07:36:27 UTC 2014

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