Re: Phabricator for patches and code review

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 :

...

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@haskell.org] On Behalf Of | Austin Seipp | Sent: 06 June 2014 05:06 | To: ghc-devs@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@haskell.org | http://www.haskell.org/mailman/listinfo/ghc-devs

_______________________________________________ ghc-devs mailing list ghc-devs@haskell.org http://www.haskell.org/mailman/listinfo/ghc-devs

_______________________________________________ ghc-devs mailing list ghc-devs@haskell.org http://www.haskell.org/mailman/listinfo/ghc-devs

_______________________________________________ ghc-devs mailing list ghc-devs@haskell.org http://www.haskell.org/mailman/listinfo/ghc-devs

_______________________________________________ ghc-devs mailing list ghc-devs@haskell.org http://www.haskell.org/mailman/listinfo/ghc-devs

_______________________________________________ ghc-devs mailing list ghc-devs@haskell.org http://www.haskell.org/mailman/listinfo/ghc-devs