5 ms·

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 dis

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.

jeltz 6y ago
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.
tardyp 6y ago
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.
- noahdesu 6y ago
  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.
- tobylane 6y ago
  They added this (reference and comparison to old heads of a PR) at about the start of the year.
  - tardyp 6y ago
    Couldn't find the feature today when doing a large review. Do you have to enable it somewhere?
    - mumblemumble 6y ago
      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.
      - 411111111111111 6y ago
        that features has been in bitbucket for ages
      - tardyp 6y ago
        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.
        mumblemumble 6y ago
        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.
  - tiziano88 6y ago
    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
nAwYz 6y ago
Large changes make the diff view incredible sluggish for me. They also seem to miss an option to mark single files as "Done with review"
_ix 6y ago
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.
- pluc 6y ago
  GitHub hides large diffs by default though so maybe that's why
  - berkut 6y ago
    So does Gitlab in my experience.
- mangamadaiyan 6y ago
  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.
gargasm777 6y ago
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.
- manigandham 6y ago
  Sourcegraph might be better: https://about.sourcegraph.com/ https://about.sourcegraph.com/
- jka 6y ago
  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/ https://about.gitlab.com/direction/global-search/
  - jmcguire-gitlab 6y ago
    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 https://gitlab.com/groups/gitlab-org/-/epics/1736
    - jka 6y ago
      Thanks for the link and engaging with the community. That led me to https://gitlab.com/groups/gitlab-org/-/epics/429 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)
daviddaviddavid 6y ago
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.
- skrebbel 6y ago
  GitHub does this too, and it's awful. Why would anyone want to skip reviewing the biggest changes?
  - nkantar 6y ago
    I'm guessing for things like package-lock.json. Though it can certainly hide much more relevant-to-reviewers changes easily.
  - mumblemumble 6y ago
    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.
    - skrebbel 6y ago
      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.
  - brodock 6y ago
    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.
mixmastamyk 6y ago
Personally, I despise how the file header moves around as you scroll over the files.
zests 6y ago
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.

jeltz 6y ago

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.

tardyp 6y ago

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.

noahdesu 6y ago
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.
tobylane 6y ago
They added this (reference and comparison to old heads of a PR) at about the start of the year.
- tardyp 6y ago
  Couldn't find the feature today when doing a large review. Do you have to enable it somewhere?
  - mumblemumble 6y ago
    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.
    - 411111111111111 6y ago
      that features has been in bitbucket for ages
    - tardyp 6y ago
      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.
      - mumblemumble 6y ago
        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.
- tiziano88 6y ago
  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

nAwYz 6y ago

Large changes make the diff view incredible sluggish for me. They also seem to miss an option to mark single files as "Done with review"

_ix 6y ago

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.

pluc 6y ago
GitHub hides large diffs by default though so maybe that's why
- berkut 6y ago
  So does Gitlab in my experience.
mangamadaiyan 6y ago
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.

gargasm777 6y ago

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.

manigandham 6y ago
Sourcegraph might be better: https://about.sourcegraph.com/ https://about.sourcegraph.com/
jka 6y ago
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/ https://about.gitlab.com/direction/global-search/
- jmcguire-gitlab 6y ago
  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 https://gitlab.com/groups/gitlab-org/-/epics/1736
  - jka 6y ago
    Thanks for the link and engaging with the community. That led me to https://gitlab.com/groups/gitlab-org/-/epics/429 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)

daviddaviddavid 6y ago

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.

skrebbel 6y ago
GitHub does this too, and it's awful. Why would anyone want to skip reviewing the biggest changes?
- nkantar 6y ago
  I'm guessing for things like package-lock.json. Though it can certainly hide much more relevant-to-reviewers changes easily.
- mumblemumble 6y ago
  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.
  - skrebbel 6y ago
    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.
- brodock 6y ago
  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.

mixmastamyk 6y ago

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

zests 6y ago

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.