Commit 7ceba86d authored by Stan Hu's avatar Stan Hu Committed by Michael Kozono

Skip squashing with only one commit in a merge request

We noticed in https://gitlab.com/gitlab-org/gitaly/issues/2395 that the
backend was unnecessarily attempting to squash merge requests with one
commit. This happened because `SquashService` skips the squash if two
conditions are met:

1. The number of commits < 2
2. The `squash_commit_message` is null

If a `squash_commit_message` is passed, this indicates that the user
wants the squashed commit to have a different commit message.

By default, the frontend always sends `squash_commit_message`,
regardless of whether the squash button is available in the merge
request. With this change, the frontend will only send this parameter if
the button is shown (i.e. when the commit count is >= 2).

Alternatively, we could also change the backend to skip squashing if
only the first condition is met.

Closes https://gitlab.com/gitlab-org/gitlab/issues/198971
parent e928c92a
...@@ -146,9 +146,15 @@ export default { ...@@ -146,9 +146,15 @@ export default {
auto_merge_strategy: useAutoMerge ? this.mr.preferredAutoMergeStrategy : undefined, auto_merge_strategy: useAutoMerge ? this.mr.preferredAutoMergeStrategy : undefined,
should_remove_source_branch: this.removeSourceBranch === true, should_remove_source_branch: this.removeSourceBranch === true,
squash: this.squashBeforeMerge, squash: this.squashBeforeMerge,
squash_commit_message: this.squashCommitMessage,
}; };
// If users can't alter the squash message (e.g. for 1-commit merge requests),
// we shouldn't send the commit message because that would make the backend
// do unnecessary work.
if (this.shouldShowSquashBeforeMerge) {
options.squash_commit_message = this.squashCommitMessage;
}
this.isMakingRequest = true; this.isMakingRequest = true;
this.service this.service
.merge(options) .merge(options)
......
---
title: Skip squashing with only one commit in a merge request
merge_request: 23744
author:
type: fixed
...@@ -6,6 +6,7 @@ describe 'User squashes a merge request', :js do ...@@ -6,6 +6,7 @@ describe 'User squashes a merge request', :js do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:source_branch) { 'csv' } let(:source_branch) { 'csv' }
let(:protected_source_branch) { false }
let!(:original_head) { project.repository.commit('master') } let!(:original_head) { project.repository.commit('master') }
...@@ -40,7 +41,7 @@ describe 'User squashes a merge request', :js do ...@@ -40,7 +41,7 @@ describe 'User squashes a merge request', :js do
def accept_mr def accept_mr
expect(page).to have_button('Merge') expect(page).to have_button('Merge')
uncheck 'Delete source branch' uncheck 'Delete source branch' unless protected_source_branch
click_on 'Merge' click_on 'Merge'
end end
...@@ -56,14 +57,34 @@ describe 'User squashes a merge request', :js do ...@@ -56,14 +57,34 @@ describe 'User squashes a merge request', :js do
end end
context 'when the MR has only one commit' do context 'when the MR has only one commit' do
let(:source_branch) { 'master' }
let(:target_branch) { 'branch-merged' }
let(:protected_source_branch) { true }
let(:source_sha) { project.commit(source_branch).sha }
let(:target_sha) { project.commit(target_branch).sha }
before do before do
merge_request = create(:merge_request, source_project: project, target_project: project, source_branch: 'master', target_branch: 'branch-merged') merge_request = create(:merge_request, source_project: project, target_project: project, source_branch: source_branch, target_branch: target_branch, squash: true)
visit project_merge_request_path(project, merge_request) visit project_merge_request_path(project, merge_request)
end end
it 'does not show the squash checkbox' do it 'accepts the merge request without issuing a squash request', :sidekiq_inline do
expect_next_instance_of(Gitlab::GitalyClient::OperationService) do |instance|
expect(instance).not_to receive(:user_squash)
end
expect(project.repository.ancestor?(source_branch, target_branch)).to be_falsey
expect(page).not_to have_field('squash') expect(page).not_to have_field('squash')
accept_mr
expect(page).to have_content('Merged')
latest_target_commits = project.repository.commits_between(source_sha, target_sha).map(&:raw)
expect(latest_target_commits.count).to eq(1)
expect(project.repository.ancestor?(source_branch, target_branch)).to be_truthy
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment