Go Proposal: Accept GitHub PRs

https://github.com/golang/go/issues/18517

by zalmoxes • 10 years ago

122 52 10 years ago

hota_mazi• 10 years ago

I'd be curious to hear more about why the Go team thinks Gerrit is superior to Github for code reviews.

As far as I can tell, Github's latest additions to their code review flow pretty much brings Github on par with Gerrit (and with also a much larger mind share. Gerrit has always remained quite obscure and marginal).

eloff2• 10 years ago

IIRC one of the problems was github notifies you as soon as a comment is posted, potentially wasting your time if you go look before the comments are finalized. For example you may see a comment asking how something works, and waste time replying only to see the comment was since deleted because the reviewer figured it out.

I don't think the issues were really that serious, but the Go team likely saw no reason to change away from the tool they were comfortable with for a slightly worse tool that they would have to adapt to.

rickcnagy• 10 years ago

The new GitHub changes allow you to group a set of comments into a "review" that then notifies the recipient all at once, along with letting you mark the PR as "approved" or "needs changes".

wyldfire2• 10 years ago

Interesting. I found Gerritt's behavior (the flipside) to be a little frustrating. I just have one comment, right here: <x> -- why do I need to publish it and give it a meta-comment?

zalmoxes• 10 years ago

The new github workflow is an improvement, but still has a lot of things which need to be better design/implementation.

I regularly collaborate on github with people who are familiar with either Gerrit or Phabricator, and I've yet to see someone praise github for having a better workflow. Much of what they announced recently feels like alpha quality/playing catch up.

rasjani• 10 years ago

Phabricator is shit from my experience but only used it with svn backend. Gerrit's main downside for me personally is that the web interface captures bowsers search function. While it's nice, I don't like my 'os' level functionality to be overwritten.

steveklabnik4• 10 years ago

I strongly prefer GitHub's flow to Gerrit or Phabriactor. Those tools have far too much complexity; I find the new GitHub stuff to strike a nice balance.

I haven't used Gerrit in a few years though, maybe it's changed.

jzelinskie• 10 years ago

Changesets are a big deal for seeing progress towards code review feedback.

Changesets take the typical GitHub workflow where the author force pushes to the PR to address comments and creates another "version" of that PR, with a new set of inline comments, but sharing the overarching PR comments. You can compare and contrast any set of "PR versions" in order to determine the changes applied by the author.

I desperately wish GitHub PRs worked this way.

eridius3• 10 years ago

I use a tool https://reviewable.io/ which adds this functionality (and some other stuff) on top of GitHub PRs and it's invaluable.

closeparen• 10 years ago

You should try Phabricator. Diffs work this way.

shurcooL• 10 years ago

While recent PR improvements have made it a lot better, there are still a few things missing that I know of.

For example, if a contributor force pushes a new commit to a PR branch, as a reviewer, I can't see the old version and compare against it. I'd have to tell the contributor to not do that (doesn't work for first time contributors), or always try to manually backup each stage of a PR and compare with locally. This doesn't happen on Gerrit because it tracks the entire history of the change.

Second thing. Once a PR gets large and lasts a while, it's hard to see what is going on, what review comments still need to be addressed, and where new replies are showing up. Consider this PR [0] as an example, can you tell where my "latest reply" was? It's not at the bottom because it was a reply to one of the early inline conversations, and you have to expand all the "Show outdated" links manually to find it.

Those are just a few things off the top of my head, as someone who used PRs and Gerrit. I prefer PRs but I don't think they're viable for a project that takes code quality and code review as seriously as Go does. I do hope PRs get the remaining missing pieces to be able to match Gerrit.

[0] https://github.com/google/go-github/pull/497

avar• 10 years ago

    > For example, if a contributor force pushes a new commit to a PR
    > branch, as a reviewer, I can't see the old version and compare
    > against it.

This is not fully true. While you can't "git fetch" that branch anymore, you can see comments against those old diffs in the pull request UI, which are the things likely to have changed.

For example, I submitted this PR against uWSGI a few months ago: https://github.com/unbit/uwsgi/pull/1392

Initially I screwed it up and inserted a field in the middle of a struct, whereas within a stable release it should of course be added to the end. If you click on "show outdated" you can see that old diff where I'm adding the field to the middle of the struct, but click on "files changed" and it's the current change where I fixed the issue.

I do think GitHub could improve here, they could expose forced pushes via refs/pull/NUM-COUNTER instead of just refs/pull/NUM as they do now, but it's important to point out that they're handling the common case. When someone force-pushes a pull request they're usually doing so in response to comments on the request, and those comments will still make sense because GitHub saves away the old diff, which you can compare with the current state.

remus• 10 years ago

Id be surprised if there's a really compellingreason to go with gerrit over github in terms of features, but the overhead of switching all the tooling (gocontribute tool, trybots etc.) is probably pretty high. No reason to spend a big chunk of time switching from one pretty good toolset to another pretty good tool set if you can avoid it.

nishs2• 10 years ago

Automation would be a primary reason. Gerrit and Rietveld [1] allow Google to build bots and tooling to land commits, allocate build infrastructure resources for tests, and rollback tree-breaking changes. GitHub would not be quite so hackable.

The automation would be even more important for a codebase like Chromium that receives 5-8 change lists every minute [2].

[1] https://github.com/rietveld-codereview/rietveld

[2] https://codereview.chromium.org/

steveklabnik4• 10 years ago

GitHub absolutely is scriptable this way; we at the Rust project use various bots extensibly for landing commits, build infrastructure for tests, welcoming new contributors...

We don't need to roll back tree-breaking changes because the bots are the only ones allowed to land changes in the first place, and they make sure all tests pass before they land. There's no reason a bot couldn't do this, though.

technion• 10 years ago

The other arguable positive here is one that may be hard for them to say: That using GitHub to "lower the bar for contribution" may itself not be a positive.

xyproto• 10 years ago

Gerrit has superior configuration in best programming language Prolog. Hail Prolog!

Arzh• 10 years ago

How many other review / CVS have you used. I really hate github and pull requests. They are by far the worst tools I've used and I've used too many to count.

yetanother14• 10 years ago

I contribute pull requests to quite a few github projects. However I avoid contributing to (or even using) projects with a Contributor License Agreement. Such projects are too bureaucratic.

pcl• 10 years ago

I avoid contributing to (or even using) projects with a Contributor License Agreement.

That's a pretty tough thing to do. Most (all?) Apache projects require a CLA, as does Go, Java, Mysql, Cassandra... It's pretty standard for any project that is both managed by a corporation and accepts third-party contributions.

EDIT: removed Postgres. Thanks, 'anarazel!

Nadya• 10 years ago

All of those projects also have alternatives that don't require a CLA. The solution is to avoid projects by bureaucratic corporations.

yetanother14• 10 years ago

~~You got me on Postgres. Don't use the others.~~

Edit: another post states that Postgres doesn't require a CLA for contributors. So I don't use any software requiring a CLA.

anarazel• 10 years ago

Postgres doesn't have a CLA.

shadowfacts• 10 years ago

Why? Aside from having to click a button saying that you agree to the CLA, how does it affect you in any way?

CLAs aren't about making a project more bureaucratic, they're about transferring the rights over the code you're contributing from you to the project, so that there's no dispute over who the code belongs to.

yetanother14• 10 years ago

I don't like the idea of handing over my contributions to another party under their terms. Either release software under a common open source license (BSD, MIT) or not. Don't add additional terms with some custom license that certainly wouldn't help me.

wyldfire2• 10 years ago

I agree that CLAs are a bit of a hassle, but it makes things much saner for licensors and licensees alike. One high profile project that doesn't use them is the linux kernel. It's mostly only the uniformity of mind among contributors that keeps things on course. Can you imagine the consequences if one or more developers decided that they wanted to change the terms for their contributions?

--- scratch that! Ok, I'm a bit out of date: linux's DCO solves this problem without requiring a DCA. But think of other projects that want to have long term growth and what challenges could arise in the face of changing attitudes over time.

strictfp4• 10 years ago

I can't help thinking that requiring a specific code review tool is a bit ridiculous, especially considering the hoops they want to jump through to enforce it.

dom02• 10 years ago

Because maintainer timer is, in your world, a worthless thing?

strictfp4• 10 years ago

No, because the tools are very similar and they can just ask contributors to adapt to the maintaners wishes. Code diffs are such a small part of reviewing a new feature. "Just deal with it" is what I think. I don't require notepad on my Mac, so why should they require gerrit on github? And besides, if they want to they can just diff it on their own machines with whatever tools they like!

sergiotapia• 10 years ago

Seems like a lot of hoops to jump through, I wonder why they want to use Gerrit. Meanwhile Elixir you open a PR and Jose and the gang address it quickly.

4 open PR's! https://github.com/elixir-lang/elixir/pulls

strictfp4• 10 years ago

Hey, we're getting downvoted! I guess gophers aren't so pragmatic after all, I thought they followed New Jersey style!

Manishearth• 10 years ago

With Servo we use https://reviewable.io/ on github.

We don't enforce its use. If a newcomer makes a pull request, we'll just use the GH review interface. In general simple PRs go through the GH review interface, regardless of who they're from. We break out Reviewable when a PR actually needs it (this is usually the choice of the reviewer).

This means that Servo continues to have a lower bar to contribution, without hampering maintainers on PRs that need more than GHs set of review features.

This has worked out well for us.

StevePerkins• 10 years ago

I've contributed to another Google project (http://vitess.io) that used the the same process. So while Go is a completely different team, it's not exactly unknown within Google. Reviewable is a dramatic improvement over the stock Github tooling.

Go Proposal: Accept GitHub PRs

Related Stories