Hacker News Reader

Comment by mumblemumble | Hacker News Reader

I'd love to hear more about what you dislike about merge request reviews. At work we've been talking about migrating looking for alternatives to GitHub over dissatisfaction with its code review interface, and, of course, GitLab is on the list of ones to check out.

View on HN

Ability to review diff between two versions of PR is a great missing feature in GitHub. When dealing with large changes, GitLab review flow is IMHO easier for reviewer. Gitlab store each push of a MR branch as different version (as hidden refs), which make the feature possible. In GH, you force push a branch, you loose the old code for good, so reviewer has to remember why he did that comment before to see if it is still relevant.

This is such an important feature that has a direct impact on code quality and time savings that I'm not sure why it isn't a priority feature. As a reviewer, having to choose between "review everything again" or "trust the submitter didn't introduce extra changes in the revised pr" is such a terrible choice to have to make.

They added this (reference and comparison to old heads of a PR) at about the start of the year.

Couldn't find the feature today when doing a large review. Do you have to enable it somewhere?

In the top left, there should be a "changes from..." drop down menu that will let you choose which commits you want to view. The default is "changes from all commits".

If you're on GitHub Enterprise, availability will depend on the last time your company has installed updates.

that features has been in bitbucket for ages

That is not the same feature. It requires you to always add your fix in a new commit add opposed to amending the commits and maintain a clear purposed commit list.

I think the more official Git way of doing it would be to collect all the changes in separate commits, and then squash merge them at the end of the code review.

I think this only works when diffing between commits in the same PR, but not when force pushing the same commit over and over again

In my experience, GitLab's MR UI really lacks performance when the diffs pass a certain length. Similarly sized diffs in GitHub don't seem to have this problem, though.

I think of it as additional incentive to maintain smaller patches for ease of review, but sometimes changes are necessarily large due to complexity. It can be obnoxious to be hamstrung like this.

GitHub hides large diffs by default though so maybe that's why

So does Gitlab in my experience.

Honest question: isn't it far better to review the patch locally on your laptop? I always do this, primarily because I like to be able to go through the callers of functions that the patch modifies, look at the callers of functions that have been introduced, etc.

My decades-long complaint with every single code review system I've been forced to use at my workplaces (which no system I've seen fixes so far) is that they don't let you browse code and examine the diff in its native setting, so to speak.

I mostly like their merge request UI but one big missing feature for me as an easy way to view and review commit messages. For complex patches the commit messages can be vital.

One thing I find annoying and occasionally hazardous about Gitlab merge requests is that per-file diffs which exceed a certain length are collapsed by default. This introduces risk since it is very easy for a reviewer to skim right past the collapsed file name, since one's eye tends to be drawn to the more salient red/green diff.

GitHub does this too, and it's awful. Why would anyone want to skip reviewing the biggest changes?

I'm guessing for things like package-lock.json.

Though it can certainly hide much more relevant-to-reviewers changes easily.

I have developed a habit of going through my own pull requests immediately after opening them, and adding a "don't forget this file" comment to all the ones that get collapsed.

'Cuz yeah, I've seen some nasty stuff sail past code review simply because it's so easy to accientally miss those files.

Good one, I'm going to do that too, thanks. It's pretty nuts how broken a UI is that it forces us to do stuff like this though.

I feel like everything about the GitHub PR UI is targeted at reducing the code review job to being a human style checker. Minimal context, no code navigation, abysmal rename following.. it's really hard to review the design of a change.

The reason for the automatic collapse is due to performance. Also it's not uncommon (depending on the language and framework you are using) for the biggest files to be just a mere dependency lock file kind-of update.

Code search, Gitlab doesn't have a code search feature. Paraphrasing The code search functionality can be achieved by adding Elasticsearch. We haven't added elasticsearch yet so cannot comment on the experience of using it.

Sourcegraph might be better: https://about.sourcegraph.com/

It looks like code search is available for self-hosted GitLab instances, and also for paid groups on gitlab.com.

It could be a little while before global code search is available to non-paid users, according to their search roadmap documentation[1].

[1] - https://about.gitlab.com/direction/global-search/

Our current approach is to make code search available for Bronze/starter and above. We appreciate your feedback on enabling it for non-paid. This is the Epic we are working on, feel free to leave feedback here as well. https://gitlab.com/groups/gitlab-org/-/epics/1736

Thanks for the link and engaging with the community. That led me to https://gitlab.com/groups/gitlab-org/-/epics/429 which sounds like a challenge.

Although not at GitLab.com scale, I've worked on a few search-related projects (often using Elasticsearch) in the past and would consider offering some ideas.

Does GitLab have a policy for attribution/licensing of technical design input from external contributors? (this is partly out of curiosity in general -- I don't know for sure whether I'd have anything to add that your team wouldn't already have thought of themselves)

We have made some solid progress on reducing the index size recently. This comment has a nice summary on the preliminary results of what we are working on. https://gitlab.com/gitlab-org/gitlab/-/issues/27918#note_353...

We always welcome the sharing of ideas and feedback. Just tag me on anything I should review. GitLab ID is @JohnMcGuire

We love external contributions. This page should help answer questions about contributing. https://about.gitlab.com/community/contribute/

Great, thank you!

Merge requests seem to merge both branches in to each other. This is not the behavior I would expect. It doesn’t seem to be that way on github but I could be wrong. I see this behavior when to long lived branches are merged.

My git-fu is average level so its possible that I’m missing something obvious here. For me the easy solution is to just merge on the command line.

Personally, I despise how the file header moves around as you scroll over the files.

Reply on news.ycombinator.com