4 ms·

Right now, Gerrit and Phabricator give an almost unrivalled code review experience - the ability to queue multiple comments up and submit them at once, mark pre

by asb 10y ago

Right now, Gerrit and Phabricator give an almost unrivalled code review experience - the ability to queue multiple comments up and submit them at once, mark previous things as fixed, keep track of a patch as it evolves along with the comments that were made against it. However the Github/Gitlab PR flow is easier for most occasional contributors. My question to Gitlabbers following this thread: do you have anything in the works for improving code review to better match some of the use cases which are handled so well by the Gerrit/Phabricator approach?

zegerjan 10y ago
Posting multiple comments and submit them at once would be awesome, I can't find an issue for it right now, but Ill create one so you can track it. Merge Request versions is something we plan on shipping this release[1]. Resolving a discussion on a diff has been shipped with 8.11[2]. We'd love to hear your feedback on it, as we know we how good Gerrit and Phabricator are in this regard. If there is another feature you'd like in GitLab too, we would love to hear more on it. [1] https://twitter.com/dzaporozhets/status/773969953569533952 https://twitter.com/dzaporozhets/status/773969953569533952 [2] https://docs.gitlab.com/ce/user/project/merge_requests/merge_request_discussion_resolution.html https://docs.gitlab.com/ce/user/project/merge_requests/merge...
- sytse 10y ago
  Submit at once for comments is in https://gitlab.com/gitlab-org/gitlab-ce/issues/3364 https://gitlab.com/gitlab-org/gitlab-ce/issues/3364
jobvandervoort 10y ago
Absolutely! We want GitLab code review to be the best there is. It's hard to find a good balance between light-weight and powerful code review tools, but I think we're finding the balance by making small iterative improvements. I collect most improvements here [0], I just added your request under 'transaction 'batch' comments'. [0]: https://gitlab.com/gitlab-org/gitlab-ce/issues/19049 https://gitlab.com/gitlab-org/gitlab-ce/issues/19049
lima 10y ago
Phabricator is a really solid GitHub alternative. Some of the largest open source projects are using it and lots of companies. I believe that the GitHub pull request workflow and issue tracker does not scale to large projects and very much prefer Gerrit/Phabricator. Phabricator was created at Facebook for a large development team (mobile apps) and it shows. They have solutions for issues which only appear at scale (code ownership in monorepos, for example - the owners/herald mechanism). It's also very performant, has almost no dependencies and is extremely easy to deploy. GitLab is a great GitHub alternative, but it's also copying its flaws. Great writeup here: (not mine) http://cramer.io/2014/05/03/on-pull-requests http://cramer.io/2014/05/03/on-pull-requests HN discussion: https://news.ycombinator.com/item?id=7697132 https://news.ycombinator.com/item?id=7697132 Good quote from that discussion: > As far as code reviews go, this is pretty spot on. I was part of a a startup that was using GitHub pull requests for code reviews. As the team grew, it became more and more intractable, although not simply because of notifications. Side-by-side diffs and checkpointed diffs (so that you can see what changed since the last round of review and whether/how your comments were addressed) are handled very poorly by GitHub. We ultimately switched to Phabricator, and while there was a little friction as folks got acquainted with the new tool, it made code reviews a much more pleasant process. Recently, I had to go through a full code review back on GH pull requests, and it felt like pulling teeth in comparison. They're fine for interacting with contributors to an open source project, but compared to working with a tool like Phabricator that's built for a code reviewer's workflow (and for teams of engineers working together on a project), they just don't hold a candle, in my opinion.
- jobvandervoort 10y ago
  We have many requests to improve our code review and make it more like Phabricator and Gerrit. We want to strike a balance between light-weight and powerful, but fully intend to give you all the power of a Phabricator or Gerrit in GitLab. We're working on several [0] initiatives towards this goal. If you are interested in seeing these improvements in GitLab, please let us know what is important and what we're not seeing! [0]: https://gitlab.com/gitlab-org/gitlab-ce/issues/19049 https://gitlab.com/gitlab-org/gitlab-ce/issues/19049
  - 10y ago
andruby 10y ago
I miss this functionality when reviewing large Github PR's. A PR can quickly contain thousands of changed (added/removed) lines of code. When reviewing a PR, I want to keep track of which lines I've already approved. I would love to see a way to mark code blocks or entire commits as "reviewed". This is especially handy because PR diff's are shown alphabetically by file path, but the code paths obviously aren't, so it's really hard to keep track of review progress.
- jobvandervoort 10y ago
  In GitLab you can't exactly do as you suggest, but you can collapse a diff, the state of which is kept. You could use that. I'd love for you to add some more flavor here [0], so we can see if we can build something like that. [0]: https://gitlab.com/gitlab-org/gitlab-ce/issues/19049#note_15235662 https://gitlab.com/gitlab-org/gitlab-ce/issues/19049#note_15...
- joneholland 10y ago
  Why are you allowing such large pulls? Everyone who complains about githubs pull request UI always has the same root cause, huge, long lived reviews. Don't fix bad process with more process and tooling. Fix it at the root.
  - wildfire 10y ago
    Because the feature is large? Or the developer acted normally and split thing into multiple, small, independant commits? If the submittor then takes a lot of time between comments and performing changes, is the commentor to blame for a PR being long-lived? Don't assume everything is bad process.
    - joneholland 10y ago
      Why can't it be a series of small PRs then? Or do you not do continuous delivery?
      - 10y ago
sytse 10y ago
Right now Dmitriy is working on merge request versions in GitLab. https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6127 https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6127 He was asking for Gerrit users to talk with him in https://twitter.com/dzaporozhets/status/770217384988770304 https://twitter.com/dzaporozhets/status/770217384988770304 We want to make sure we learn from the best parts of Gerrit and Phabricator and bring them to GitLab. I love Phabricator's ability to comment on specific locations of images https://gitlab.com/gitlab-org/gitlab-ce/issues/2641 https://gitlab.com/gitlab-org/gitlab-ce/issues/2641 and to show the code coverage in the diff https://gitlab.com/gitlab-org/gitlab-ce/issues/4073 https://gitlab.com/gitlab-org/gitlab-ce/issues/4073 I'm not sure if merge request versions will have the ability to batch comments https://gitlab.com/gitlab-org/gitlab-ce/issues/3364 https://gitlab.com/gitlab-org/gitlab-ce/issues/3364 but this is certainly something we're interested in.
- jobvandervoort 10y ago
  > I'm not sure if merge request versions will have the ability to batch comments https://gitlab.com/gitlab-org/gitlab-ce/issues/3364 https://gitlab.com/gitlab-org/gitlab-ce/issues/3364 but this is certainly something we're interested in. They won't, but the feature request is added here [0]. [0]: https://news.ycombinator.com/item?id=12487854 https://news.ycombinator.com/item?id=12487854

by asb 10y ago

zegerjan 10y ago

Posting multiple comments and submit them at once would be awesome, I can't find an issue for it right now, but Ill create one so you can track it. Merge Request versions is something we plan on shipping this release[1]. Resolving a discussion on a diff has been shipped with 8.11[2]. We'd love to hear your feedback on it, as we know we how good Gerrit and Phabricator are in this regard. If there is another feature you'd like in GitLab too, we would love to hear more on it. [1] https://twitter.com/dzaporozhets/status/773969953569533952 https://twitter.com/dzaporozhets/status/773969953569533952 [2] https://docs.gitlab.com/ce/user/project/merge_requests/merge_request_discussion_resolution.html https://docs.gitlab.com/ce/user/project/merge_requests/merge...

sytse 10y ago
Submit at once for comments is in https://gitlab.com/gitlab-org/gitlab-ce/issues/3364 https://gitlab.com/gitlab-org/gitlab-ce/issues/3364

jobvandervoort 10y ago

Absolutely! We want GitLab code review to be the best there is. It's hard to find a good balance between light-weight and powerful code review tools, but I think we're finding the balance by making small iterative improvements. I collect most improvements here [0], I just added your request under 'transaction 'batch' comments'. [0]: https://gitlab.com/gitlab-org/gitlab-ce/issues/19049 https://gitlab.com/gitlab-org/gitlab-ce/issues/19049

lima 10y ago

Phabricator is a really solid GitHub alternative. Some of the largest open source projects are using it and lots of companies. I believe that the GitHub pull request workflow and issue tracker does not scale to large projects and very much prefer Gerrit/Phabricator. Phabricator was created at Facebook for a large development team (mobile apps) and it shows. They have solutions for issues which only appear at scale (code ownership in monorepos, for example - the owners/herald mechanism). It's also very performant, has almost no dependencies and is extremely easy to deploy. GitLab is a great GitHub alternative, but it's also copying its flaws. Great writeup here: (not mine) http://cramer.io/2014/05/03/on-pull-requests http://cramer.io/2014/05/03/on-pull-requests HN discussion: https://news.ycombinator.com/item?id=7697132 https://news.ycombinator.com/item?id=7697132 Good quote from that discussion: > As far as code reviews go, this is pretty spot on. I was part of a a startup that was using GitHub pull requests for code reviews. As the team grew, it became more and more intractable, although not simply because of notifications. Side-by-side diffs and checkpointed diffs (so that you can see what changed since the last round of review and whether/how your comments were addressed) are handled very poorly by GitHub. We ultimately switched to Phabricator, and while there was a little friction as folks got acquainted with the new tool, it made code reviews a much more pleasant process. Recently, I had to go through a full code review back on GH pull requests, and it felt like pulling teeth in comparison. They're fine for interacting with contributors to an open source project, but compared to working with a tool like Phabricator that's built for a code reviewer's workflow (and for teams of engineers working together on a project), they just don't hold a candle, in my opinion.

jobvandervoort 10y ago
We have many requests to improve our code review and make it more like Phabricator and Gerrit. We want to strike a balance between light-weight and powerful, but fully intend to give you all the power of a Phabricator or Gerrit in GitLab. We're working on several [0] initiatives towards this goal. If you are interested in seeing these improvements in GitLab, please let us know what is important and what we're not seeing! [0]: https://gitlab.com/gitlab-org/gitlab-ce/issues/19049 https://gitlab.com/gitlab-org/gitlab-ce/issues/19049
- 10y ago

andruby 10y ago

I miss this functionality when reviewing large Github PR's. A PR can quickly contain thousands of changed (added/removed) lines of code. When reviewing a PR, I want to keep track of which lines I've already approved. I would love to see a way to mark code blocks or entire commits as "reviewed". This is especially handy because PR diff's are shown alphabetically by file path, but the code paths obviously aren't, so it's really hard to keep track of review progress.

jobvandervoort 10y ago
In GitLab you can't exactly do as you suggest, but you can collapse a diff, the state of which is kept. You could use that. I'd love for you to add some more flavor here [0], so we can see if we can build something like that. [0]: https://gitlab.com/gitlab-org/gitlab-ce/issues/19049#note_15235662 https://gitlab.com/gitlab-org/gitlab-ce/issues/19049#note_15...
joneholland 10y ago
Why are you allowing such large pulls? Everyone who complains about githubs pull request UI always has the same root cause, huge, long lived reviews. Don't fix bad process with more process and tooling. Fix it at the root.
- wildfire 10y ago
  Because the feature is large? Or the developer acted normally and split thing into multiple, small, independant commits? If the submittor then takes a lot of time between comments and performing changes, is the commentor to blame for a PR being long-lived? Don't assume everything is bad process.
  - joneholland 10y ago
    Why can't it be a series of small PRs then? Or do you not do continuous delivery?
    - 10y ago

sytse 10y ago

Right now Dmitriy is working on merge request versions in GitLab. https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6127 https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6127 He was asking for Gerrit users to talk with him in https://twitter.com/dzaporozhets/status/770217384988770304 https://twitter.com/dzaporozhets/status/770217384988770304 We want to make sure we learn from the best parts of Gerrit and Phabricator and bring them to GitLab. I love Phabricator's ability to comment on specific locations of images https://gitlab.com/gitlab-org/gitlab-ce/issues/2641 https://gitlab.com/gitlab-org/gitlab-ce/issues/2641 and to show the code coverage in the diff https://gitlab.com/gitlab-org/gitlab-ce/issues/4073 https://gitlab.com/gitlab-org/gitlab-ce/issues/4073 I'm not sure if merge request versions will have the ability to batch comments https://gitlab.com/gitlab-org/gitlab-ce/issues/3364 https://gitlab.com/gitlab-org/gitlab-ce/issues/3364 but this is certainly something we're interested in.

jobvandervoort 10y ago
> I'm not sure if merge request versions will have the ability to batch comments https://gitlab.com/gitlab-org/gitlab-ce/issues/3364 https://gitlab.com/gitlab-org/gitlab-ce/issues/3364 but this is certainly something we're interested in. They won't, but the feature request is added here [0]. [0]: https://news.ycombinator.com/item?id=12487854 https://news.ycombinator.com/item?id=12487854