A proper code review system for Cabal

[ezyang]

Continuation from #3039. CC @BardurArantsson.

No direct experience with Phab, but doesn't it require some sort of weird custom command line tool instead of just using standard git commands? (When reviewing and pushing patches to review, obviously.)

Yes, but it's pretty tolerable (since there isn't actually a standard git command that works well for the Phabricator, per-patch workflow.) Phabricator's big weakness is it can't deal with pull requests with lots of commits; you have to submit each one individually.

[BardurArantsson]

That sounds a royal pain.

I'm used to Gerrit. It integrates almost perfectly with git's normal workflow -- which could be explained by the fact that it was written explicitly for working with git :). When you want to submit a changeset (multiple logically coherent commits) you just push to a special URL.

[BardurArantsson]

(There would probably be some issues around auth, though. It's based on OpenID auth and I'm not sure if GitHub supports such things.)

There is this, though: http://gerrithub.io/

EDIT: ... and then I look at the pricing. I'm sure the product is wonderful, but... wow.

[ezyang]

I guess the reason GHC ended up using Phabricator is because Simon Marlow is now at Facebook, and that's the CR tool they use at Facebook.

I have to admit, having had to interact with Gerrit when I was doing stuff with OpenAFS, it's "Git native" workflow is extremely inscrutable. I was looking at some more recent workflow documentation and it looks like everyone recommends installing a helper program "git review"... in which case, I actually don't feel there is much substantative difference.

Also, Gerrit seems to have the same limitation as Phabricator: a Gerrit changeset seems to be a single commit, c.f. https://code.google.com/p/gerrit/issues/detail?id=51 To be clear, in Phabricator, you CAN upload a topic branch for review; but the CR interface will just display it as a single megapatch.

[BardurArantsson]

Re: "git review". I've never heard of it and get along with Gerrit very well. It may be because we've set it up such that changes always (and only) go to master. In that case you can just add a special remote in your .git/config and off you go. I'm sure it can get a little more annoying if you need to enforce review for multiple "target" branches.

Re: changeset: Oh, I misunderstood what you meant. Yes, gerrit merges changesets as individual commits[1], but you do reviews on the individual commits... which is what you (generic you) usually want if you follow the usual git workflows and guidelines around commits. I must be missing something, but that seems to be all that's needed? What does "whole changeset" review add in your opinion?

[1] They are "chained together" in sequence, so no merging a commit which depends on an earlier commit. You also get a display of all the commits that are involved in a logical change (it's a little list) -- you don't get to add additional data on the "changeset as whole", but I've literally never felt a need for that.

[BardurArantsson]

(I guess my view might be limited, but git doesn't really have any formal concept of anything bigger than a commit AFAICS. There are things that point to commits, but that's about it.)

[ezyang]

Yeah, I guess that's the primary Gerrit/Phabricator workflow difference: https://phabricator.wikimedia.org/T125 Apparently there's been very little demand for this! https://phabricator.wikimedia.org/T23#360

If you want to see/review the individual commits, you have to submit them separately, so it's discouraged. Here's a Phabricator diff where it would be nice to see this https://phabricator.haskell.org/D38 (you can see the individual commits but the actual diff is everything at once.)

[thoughtpolice]

The reason Phabricator was chosen is far more detailed than just SimonM being familiar. Basically: It slotted in as an existing code review mechanism, without disturbing other things (we can just ignore everything else Phab does). This ended up being exceptionally important for a lot of reasons (because GHC already has a lot of supporting infrastructure that we don't want to redo), and was part of the reason I chose it.

(That said, I do like the concept Gerrit has where there is a singular remote to push every change into, which gets dealt with from there - this is far better than GitHub where every popular project has 50,000 permaforks which are dead because a fork is required for a PR.)

Finally, as for the concept of reviewing multiple commits, I do believe this can be fixed in a few ways. In particular, see https://secure.phabricator.com/T5000 and https://secure.phabricator.com/D9599, and also https://phabricator.freedesktop.org/diffusion/GITPHAB/, which can 'split' commits into multiple reviews (purely from the client side). We could do something similar for our users, and I've been meaning to try out 'git-phab' specifically since I think it might work well for us.

[BardurArantsson]

I think I'll just close this, if nobody minds. For my part, I wasn't seriously suggesting moving away -- mostly just venting a bit...