Phabricator for patches and code review

Jan Stolarek jan.stolarek at p.lodz.pl
Wed Jun 18 07:38:04 UTC 2014 

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

More information about the ghc-devs mailing list