4 ms·
Probably because you got used to Critique at Google ;) I agree though. I think the most important thing in a code review system is inline comments in the diff
by dietrichepp 6y ago
Probably because you got used to Critique at Google ;)
I agree though. I think the most important thing in a code review system is inline comments in the diff itself, and that’s something you get from Gerrit, Phabricator (Differential), etc. It encourages people to discuss the particulars of of a diff. Merge approval can be made contingent on resolving minor issues within a diff. Diffs are also approved on a per-diff basis, and it’s less typical to merge a stack of diffs.
I think the pull request / merge request makes sense with the “trusted lieutenants” development model that the Linux kernel uses, but for other projects you would be more likely to want a work flow where someone submits a single commit/diff and then someone approves it (after comments).
When I review PRs on services like GitHub I very often think, “This should be several different reviews” and the discussion thread in a PR is often not a high-quality discussion. I don’t use GitLab as much but my experience is that it has the same problems. What I would love is to review a stack of commits and approve / make comments on the commits individually.
(For those reading: Mondrian -> Rietveld -> Gettit, and also Mondrian -> Critique. Mondrian and Critique are internal tools at Google. Phabricator originated at Facebook which has a lot of ex-Google engineers on staff.)
- nemetroid 6y agoI don't use Github much, but Gitlab allows for multiple threads in a merge request. These threads may reference diffs/commits, but can also be directed at the merge request in general. Each thread has to be explicitly resolved before merging.
- eatthoselemons 6y agoDo you think that gitlab will ever add inline diff comments? I don't know if it would even be feasible to add to gitlab
- e12e 6y agoMore online than comments on the changes in the Mr? Like: https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/26540/diffs https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/2... (there's some old discussion on there, random example)
- kevincox 6y agoI think the pull request model still makes sense. Of course if you stick to small changes it tends towards the patch model. However thete still some cases where two or three commits at once make sense. Even rarer there are cases where marging a bunch of changes into a "staging" branch before merging to master makes sense. I think this added flexibility is valuable, of course keeping the "patch style" single-commjt review great should probably be the priority.