Hacker News Reader

Comment by teen | Hacker News Reader

Gitlab has been pretty great so far, but the code review tooling is so poor I think I need to look elsewhere. The pull request pages take forever to load, they can't handle pull requests with over 800 lines well at all. It will collapse files with over 100-200 lines of changes for seemingly no reason (hiding the comments within them). You can't ignore whitespace in diffs, and the comments disappear from the diff if they push new commits, you need to look at them in the overview again.

View on HN

Hi, Engineering Manager at GitLab here, Source Code team.

Thanks for the comment.

Indeed that is something we've had in our minds for a long time. Over the past months/years, we have been shipping out incremental improvements in the way we handle and render large MR diffs.

Recently we made the loading happen in batches which resulted in a significant improvement in how fast we start showing the diffs. https://gitlab.com/groups/gitlab-org/-/epics/1816

We plan to continue improving the performance of the MR page, lowering memory footprint this Q2 as well as providing other ways to enhance the experience.

Some examples:

* Commit navigation, to help commit by commit review: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28596

* File by file mode of reviewing MRs, including the concept of "unread diffs": https://gitlab.com/groups/gitlab-org/-/epics/516

We're constantly looking to improve and learn more about the struggles of our users. If you have further thoughts, please drop a comment in one of the issues and epics above and tag me (@andr3).

If some of your problems are not covered by these, feel free to open an issue (https://gitlab.com/gitlab-org/gitlab) and tag me as well.

Again, thanks for the feedback.

> It will collapse files with over 100-200 lines of changes for seemingly no reason (hiding the comments within them)

OH MY GOD this so much. This one specific anti-feature has no doubt caused countless bugs to be missed during code review. I can quote the issue thread by memory at this point i've revisited it so many times the last year(s?)

For goodness sake, if the trade off is between 'seeing all of the changes during the review' vs 'having the page load quickly', the only people who choose 'quickly' are those who haven't been burned by human error missing a collapsed diff. Gitlab made the wrong trade off here in good/fast/cheap triangle.

Here's a little script I use occasionally, it forces the browser to download all hidden files, and to display them all:

  document.querySelectorAll('.js-file-title').forEach(t => t.click());
  document.querySelectorAll('.diff-content').forEach(d => d.style.display="block");
  document.querySelectorAll('.nothing-here-block').forEach(d => d.style.display="none");

In my experience, if you are reviewing 800+ line PRs you should be in an IDE. I believe the average human being can not review an 800 line diff reliably.

Such large PRs would also probably benefit from live discussions over video (or in the good old days -- in person).

Interesting, I’ve only went through the effort of checking out a pull request for review a couple times. What’s the workflow like? Any tips for reducing friction? I avoid checking out PRs because it’s much higher friction and because I trust CI to catch most bugs so the crappy code forge interfaces are good enough.

Personally, I would kill for a one click to step review interface in IntelliJ that synced comments back to the review.

I use gitlens extension on Visual studio.Pretty great so far.

I've reviewed 4000 line MRs without problems using other tools. It is possible.

I can relate. We use GitLab at our company.

I still think the merge request UI is great. Its main problems are its slowness and the fact that files with a long diff are collapsed. As the full merge request is quite slow, I will usually open the diff of each commit one by one, then put review comments as a "per commit" basis.

Thanks for the comment.

I've left a few replies in this thread already with more detail of work we have done, is currently in flight and work that is planned to keep improving the overall situation.

As to the "per commit" review, we're working on something to make this navigation easier and easier and shipping it very soon. https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28596

As to the collapsed, unfortunately if we loaded all diff lines expanded, the large changesets would make the browser unusable. We're working to lower the memory footprint and have better ways to deal with these large MRs.

One example I can leave you with that we're trying to figure out a way to make this useful as an additional option, is this: * File by file mode of reviewing MRs, including the concept of "unread diffs": https://gitlab.com/groups/gitlab-org/-/epics/516

We're aware of several of the pain points (we use GitLab to build GitLab ;) ), but if you have further feedback, please leave a comment in the issues and epics I've left here (or create new ones) and feel free to tag me @andr3.

Thanks!

I don’t understand. This describes basically every code review tool I’ve ever used. If there’s something aside from Gitlab that I should try that still integrates, I’d love to know.

For huge diffs, there’s not much that beats `git checkout <branch> && git diff HEAD^`.

For leaving comments, pretty much every system I’ve used has issues. At work we use phabricator; the default settings are atrocious but they can be tuned to something workable.

I sometimes wonder how hard it would be to get diff reviews to be a first (or even second) class citizen of the git ecosystem. That way you can finally get the bliss of leaving review comments in the IDE. They already have “notes” which are vaguely similar, maybe it’s something they’re considering?

> [git ecosystem] have “notes” which are vaguely similar, maybe it’s something they’re considering?

If you're referring to git-notes, they are ... kinda special. They are objects in their own namespace that reference the repository objects the notes themselves are attached to.

Sure enough, you can attach a note to any git-object. And you can view them either directly or from the git-log view. But the discoverability is lacking, and the UI of doing pretty much anything with git-notes is absolutely atrocious. Because they are in their own namespace, you don't automatically get them when you do a pull or fetch, which can get pretty confusing.

For the curious ones, the magic invocation needed to pull in updates to notes from the remote repo is:

    git fetch origin +refs/notes/*:refs/notes/*

Notes are not versioned, and you certainly don't get nice N-way merges between local and remote content. Semantics are different from regular git, and stuff can get lost.

So from where I'm looking at things, git-notes are not suitable for plumbing in a code review flow.

You're right, I was referring to git-notes and they aren't suitable as plumbing for code review :)

I was meaning to point out that we've developed plumbing for objects with "weaker" guarantees than source code, which can still be attached to refs. To me, this is indicative that the maintainers are willing to accept patches on things that are outside the "core mission" of versioning source code, and might mean that they're open to the idea of accepting a well-architected code review feature.

Although, you're also right, in that there's still a lot of plumbing to make sure that authors can only edit their own comments, and that sort of thing. The more I think about it, there's a lot of work to be done and it isn't easy.

But, if you made a proposal, I think git-notes is precedent for them accepting your contributions. :)

I always use IntelliJ for code reviews, works great.

I mean load a pull request from Github (takes <1s) Load one from Gitlab and it takes 10+ sec. Maybe I'm spoiled from using Google's tools previously.

Yeah, this is definitely one of GitLab's weakest points at the moment and seriously impacts day-to-day work here as well. Simple features like ticking off files you've already reviewed are missing and even mid-sized MRs are a pain to review even on a beefy machine.

This feature already exists, I use it all the time.

In GitLab? Maybe we're using an outdated GitLab version here, but I can't find it. And it doesn't seem to be mentioned in the offical Reviewing documentation: https://docs.gitlab.com/ce/user/project/merge_requests/revie...

If you mean the "collapsing" feature, this is really not equivalent, since the state does not persist between multiple review sessions.

I don't have it either. I wish we could "check" things off. I also wish I had more ability to do settings "by group", rather than by project.

Community Advocate at GitLab here, it'd be great if you can add this feedback to this issue: https://gitlab.com/gitlab-org/gitlab/-/issues/24629 Thanks!

Hi! I'm an Engineering Manager at GitLab in Source Code team.

This is on our horizon. We have an Epic to track an overall revamp and adding an option to review file-by-file which will include developing something like what you just described.

Epic: https://gitlab.com/groups/gitlab-org/-/epics/516

Issue for the checking of file seen (including the concept of "unread diffs"): https://gitlab.com/gitlab-org/gitlab/-/issues/24629

If you could please add your thoughts in that issue, we could use that feedback while we develop the feature.

Thanks a bunch for caring and voicing your concerns. It helps us get better.

Thanks a lot for taking the time to answer! I hope my criticism didn't come off too strong, overall I love working with GitLab, the parent just struck a chord with one of my pain points. Glad to see you're working on it!

This sounds as it it would be a great improvement over Bitbucket. :-)

Reply on news.ycombinator.com