Hacker News new | past | comments | ask | show | jobs | submit login

I feel they're making it harder to review a merge request with multiple commits. It used to be able to click on a commit and get succinct view of the actual commit with title and message. Now days after clicking on a commit, you have to scroll down and skip a lot of irrelevant information to find the actual commit content - and gitlab by default hides the commmit message! This makes it very cumbersome to review a MR with many commits, and also harder to train new developers to learn the advantages of making good commits.

Being able to review per commit in a merge request was a major reason for my company to choose gitlab back in the day. A merge request consists of a series of commits, each doing a single change (basically the same way linux's patch series works: https://www.kernel.org/doc/html/v4.16/process/submitting-pat...).

I'm hoping gitlab will improve the experience in the future for us that want patch series style merge requests, but I'm worried that gerrit style development is winning where the result of each merge request is supposed to be just a single squashed commit.




Thanks for the feedback gorset. I'm curious to dig a little deeper.

With the irrelevant information, you mean the top part of the merge requests (ie description, CI status, etc)?

And as far as I can see, in the current version, we only hide the body part of the commit message.

I think we can make the experience better by autoscrolling down once you click on a commit in a merge request. I've made an issue [0].

I'm not sure about showing the full commit message. Many would argue that only the first line summary is crucial to show, which is what we do now. But I'm happy to discuss.

[0]: https://gitlab.com/gitlab-org/gitlab-ce/issues/48328


Thanks for the quick follow-up!

This might be a difference in expectations, where we are talking about two styles of merge requests. I believe they can be both supported well by gitlab (in fact, I consider gitlab to be the best review tool out there because it has support for both styles).

## Gerrit style MR

Mostly a merge request with a single commit. All relevant information is placed in MR title/description. If there are more than one commits, they are usually added to review the first commit based on review feedback or problems detected by CI. The commits are often squashed before merge to master, so the individual commit messages are secondary to the MR title/description.

## Patch series style MR

A merge requests with multiple commits, each containing a single logical change. The MR title/description only contains high level information/summary - more detailed description is found in the commits.

If serious problems are found either by CI or reviewer, the developer often amends the commits rather than push new commits on top. The reason why is that each commit should be as complete as possible, which helps reviewing, git bisect and keeping a good git log history.

This style requires more upfront work by the developer, but the advantage is that reviewing becomes easier. Instead of reviewing a large diff with many different changes, the reviewer can look at a single commit at a time. Some types of changes such as refactor, reformat can quickly be verified and reviewed because they are not making any semantics changes. A commit containing a bug fix is much quicker to review if there are no refactoring, reformating or other semantic changes in the same diff.

Good commits should also have good commit messages, which describes what, why and why like /this/. Just as important, this information should be easily available when reviewing the merge request. It should be easy to iterate through all the commits in a merge request, reviewing both the commit message and the commit diff for each commit.

With the latest changes of gitlab, I feel it's being optimized for Gerrit style merge requests, in which case your comment make sense that you're not sure about showing the full commit message. However, for me this is a major step back since we do patch series style merge requests where each commit is often reviewed individually.


Thanks gorset for the great response and the new insight you gave me into this. I now understand what you mean and see how the changes have negatively affect patch-series style MRs.

I created an issue on this and I encourage you to contribute there as well: https://gitlab.com/gitlab-org/gitlab-ce/issues/48334

We're not fans of adding additional configuration, but I can imagine we store e.g. what the last state was that you left commit message in, as to not have to unfold it every time. I'm sure you have further thoughts on improvements that can be made.


The full commit message is just as crucial as the first line. Take for example a recent commit to the OBS project on GitHub: https://github.com/obsproject/obs-studio/commit/26d5560da3b7...

The exact commit is as follows:

  libobs: Add scene item grouping
  
  Allows the ability to group scene items.  Groups 
  internally are sub-scenes, which allows the ability to 
  add unique filters and transforms to each group.
Without the full message, I would not know what this commit brought until the changelog is updated on release. Now I know what to expect and why is was committed. In a merge request, I would like to see the same level of reasoning. For example, if there was a new commit pushed in a MR that I already saw that changed one line, I would prefer to see reasoning behind the change.

This is all personal preference though...


The commit message is the most important fricking part. It explains the why.




Consider applying for YC's Spring batch! Applications are open till Feb 11.

Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: