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?
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?
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.
They won't, but the feature request is added here [0].
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 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!
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).
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.
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'.
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.
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...
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.
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.
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.
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...