Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

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?



Right now Dmitriy is working on merge request versions in GitLab. 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

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 and to show the code coverage in the diff 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 but this is certainly something we're interested in.


> I'm not sure if merge request versions will have the ability to batch comments 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


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

HN discussion: 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.


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


I'm not a GitHub user myself, only have experience with GitHub. The article I linked outlined it very well.

Essentially, what I need is code ownership (like Phabricator's "Owner" feature or Chromium's OWNER file: https://www.chromium.org/developers/owners-files) and essentially what's outlined in the article I linked (diffs instead of heavyweight merge requests).


Interesting, I created https://gitlab.com/gitlab-org/gitlab-ce/issues/22141 to discuss.


Phabricator has many great features and we try to learn from it to improve GitLab.

Your quote touches on two things we recently improved:

1. In GitLab 8.11 https://about.gitlab.com/2016/08/22/gitlab-8-11-released/ we added the ability to resolve comments, solving "whether/how your comments were addressed"

2. In the upcoming GitLab 8.12 we've added the ability to show merge request version https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6127, solving "what changed since the last round of review"

Please let us know if there are any other specific things we can solve.


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


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.


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_15...


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.


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.


Why can't it be a series of small PRs then? Or do you not do continuous delivery?


Because then the reviews for the connected PRs ends up being hideous to track.

Either you can:

• Submit the PRs one at a time... which is awful. Why not work on them in parallel?

• Submit your series of PRs all at once, then you have to set the PR base of each one to the following one to make the github diff view actually reasonable for reviews. THEN, when you have a code change based on feedback on the first PR, you have to merge it into all the later dependent PRs. THEN, you have to have to jiggle the PR bases very carefully when you're landing the PRs, otherwise it will merge them all into each other, and you end up with one ginormous commit landing—defeating the point of the small commits. AND unless you're using a squash based workflow, you'll end up with that giant merge mess in the middle of your commit history.


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

[2] https://docs.gitlab.com/ce/user/project/merge_requests/merge...


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




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: