13 ms·

Go Proposal: Accept GitHub PRs

122 points by zalmoxes 10y ago

hota_mazi 10y 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).
- eloff 10y 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 10y 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".
  - wyldfire 10y 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?
    - steveklabnik 10y ago
      GitHub's new flow gives you both choices: a "just this one comment" button and a "start a new review" button.
- zalmoxes 10y 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 10y 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.
  - steveklabnik 10y 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.
    - yxhuvud 10y ago
      What I miss at github is a single view that list all the commits in a pull request, in sequence, with both commit messages and diff contents, with possibility to comment both message and diff. Oh, and have the commits show in the actual order the commits will show up in the history. Github has certainly come far this last year, but these are all oh so simple and basic stuff.
- jzelinskie 10y 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.
  - eridius 10y ago
    I use a tool https://reviewable.io/ https://reviewable.io/ which adds this functionality (and some other stuff) on top of GitHub PRs and it's invaluable.
    - helper 10y ago
      +1 for reviewable. In addition to this feature reviewable's diff display is a lot nicer. For example it does a much better job handling whitespace only changes when a block of code gets reindented but otherwise doesn't change.
      - eridius 10y ago
        Oh yeah, collapsing whitespace-only changes is excellent. But even better is how it can detect and collapse changes introduced by rebasing the PR.
        petetnt 10y ago
        You can hide whitespace-only changes in GitHub by appending ?w=1 to the end of the url too.
        eridius 10y ago
        But that makes the whitespace changes literally invisible. Reviewable strikes a good balance, where it will show you whitespace changes that appear in diff hunk contexts, and also explicitly call out when it's skipping a chunk of whitespace-only changes, and you can click on that callout to ask it to show you those whitespace changes anyway.
- shurcooL 10y 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 https://github.com/google/go-github/pull/497
  - avar 10y 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 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 10y 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.
  - deleted 10y ago
    [deleted]
- nishs 10y 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 https://github.com/rietveld-codereview/rietveld [2] https://codereview.chromium.org/ https://codereview.chromium.org/
  - steveklabnik 10y 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.
    - nishs 10y ago
      Apologies for the misleading GitHub portion in my previous comment. I agree, Rust is wonderfully maintained on GitHub. That said, Google might consider it easier to work with Gerrit, Rietveld, Gitiles, etc. because they can work with the source code of these projects directly, than having to write potentially ugly code to work around missing pieces in GitHub's API. I'd love to learn whether a large scale project such as Rust has run into these issues with GitHub.
      - steveklabnik 10y ago
        It's all good! I didn't think you were saying Rust was somehow mis-managed. There's some stuff that could be nicer with GitHub generally, but I don't think having the source would make it any easier to build out this automation; github's API is pretty solid.
- technion 10y 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 10y ago
  Gerrit has superior configuration in best programming language Prolog. Hail Prolog!
- Arzh 10y 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.
yetanother1 10y 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 10y 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 10y ago
    All of those projects also have alternatives that don't require a CLA. The solution is to avoid projects by bureaucratic corporations.
  - yetanother1 10y 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 10y ago
    Postgres doesn't have a CLA.
- shadowfacts 10y 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.
  - yetanother1 10y ago
strictfp 10y 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.
- dom0 10y ago
  Because maintainer timer is, in your world, a worthless thing?
  - strictfp 10y 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!
    - dom0 10y ago
      Gerrit and Github are not "very similar". They are quite different. (Something that would be rather immediately clear to anyone who used either for some duration) > And besides, if they want to they can just diff it on their own machines with whatever tools they like! Oh yes, a very compelling argument when discussing workflow tools is to suggest local diffs. Bravo!
      - strictfp 10y ago
        Well, I happen to have used both. And it all depends on your perspective. I would change my tools in a heartbeat to streamline the process for users or contributors. To me it depends on if you're user focused or technology driven. I think it's kind of akin to switching to git but insist that all merges have been done with perforce merge, because I think it's better in some way. Sure, it might be true, but do you really want to go against the stream to prove your point?
  - deleted 10y ago
sergiotapia 10y 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 https://github.com/elixir-lang/elixir/pulls
- strictfp 10y ago
  Hey, we're getting downvoted! I guess gophers aren't so pragmatic after all, I thought they followed New Jersey style!
Manishearth 10y ago
With Servo we use https://reviewable.io/ 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 10y ago
  I've contributed to another Google project (http://vitess.io 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.

122 points by zalmoxes 10y 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).

eloff 10y 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 10y 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".
- wyldfire 10y 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?
  - steveklabnik 10y ago
    GitHub's new flow gives you both choices: a "just this one comment" button and a "start a new review" button.
zalmoxes 10y 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 10y 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.
- steveklabnik 10y 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.
  - yxhuvud 10y ago
    What I miss at github is a single view that list all the commits in a pull request, in sequence, with both commit messages and diff contents, with possibility to comment both message and diff. Oh, and have the commits show in the actual order the commits will show up in the history. Github has certainly come far this last year, but these are all oh so simple and basic stuff.
jzelinskie 10y 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.
- eridius 10y ago
  I use a tool https://reviewable.io/ https://reviewable.io/ which adds this functionality (and some other stuff) on top of GitHub PRs and it's invaluable.
  - helper 10y ago
    +1 for reviewable. In addition to this feature reviewable's diff display is a lot nicer. For example it does a much better job handling whitespace only changes when a block of code gets reindented but otherwise doesn't change.
    - eridius 10y ago
      Oh yeah, collapsing whitespace-only changes is excellent. But even better is how it can detect and collapse changes introduced by rebasing the PR.
      - petetnt 10y ago
        You can hide whitespace-only changes in GitHub by appending ?w=1 to the end of the url too.
        eridius 10y ago
        But that makes the whitespace changes literally invisible. Reviewable strikes a good balance, where it will show you whitespace changes that appear in diff hunk contexts, and also explicitly call out when it's skipping a chunk of whitespace-only changes, and you can click on that callout to ask it to show you those whitespace changes anyway.
shurcooL 10y 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 https://github.com/google/go-github/pull/497
- avar 10y 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 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 10y 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.
- deleted 10y ago
  [deleted]
nishs 10y 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 https://github.com/rietveld-codereview/rietveld [2] https://codereview.chromium.org/ https://codereview.chromium.org/
- steveklabnik 10y 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.
  - nishs 10y ago
    Apologies for the misleading GitHub portion in my previous comment. I agree, Rust is wonderfully maintained on GitHub. That said, Google might consider it easier to work with Gerrit, Rietveld, Gitiles, etc. because they can work with the source code of these projects directly, than having to write potentially ugly code to work around missing pieces in GitHub's API. I'd love to learn whether a large scale project such as Rust has run into these issues with GitHub.
    - steveklabnik 10y ago
      It's all good! I didn't think you were saying Rust was somehow mis-managed. There's some stuff that could be nicer with GitHub generally, but I don't think having the source would make it any easier to build out this automation; github's API is pretty solid.
technion 10y 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 10y ago
Gerrit has superior configuration in best programming language Prolog. Hail Prolog!
Arzh 10y 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.

yetanother1 10y 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 10y 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 10y ago
  All of those projects also have alternatives that don't require a CLA. The solution is to avoid projects by bureaucratic corporations.
- yetanother1 10y 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 10y ago
  Postgres doesn't have a CLA.
shadowfacts 10y 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.
- yetanother1 10y ago

strictfp 10y 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.

dom0 10y ago
Because maintainer timer is, in your world, a worthless thing?
- strictfp 10y 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!
  - dom0 10y ago
    Gerrit and Github are not "very similar". They are quite different. (Something that would be rather immediately clear to anyone who used either for some duration) > And besides, if they want to they can just diff it on their own machines with whatever tools they like! Oh yes, a very compelling argument when discussing workflow tools is to suggest local diffs. Bravo!
    - strictfp 10y ago
      Well, I happen to have used both. And it all depends on your perspective. I would change my tools in a heartbeat to streamline the process for users or contributors. To me it depends on if you're user focused or technology driven. I think it's kind of akin to switching to git but insist that all merges have been done with perforce merge, because I think it's better in some way. Sure, it might be true, but do you really want to go against the stream to prove your point?
- deleted 10y ago

sergiotapia 10y 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 https://github.com/elixir-lang/elixir/pulls

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

Manishearth 10y ago

With Servo we use https://reviewable.io/ 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 10y ago
I've contributed to another Google project (http://vitess.io 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.