summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js4
-rw-r--r--app/controllers/projects/merge_requests_controller.rb2
-rw-r--r--app/graphql/types/merge_request_type.rb3
-rw-r--r--app/models/commit.rb2
-rw-r--r--app/models/commit_collection.rb9
-rw-r--r--app/models/merge_request.rb10
-rw-r--r--app/models/repository.rb4
-rw-r--r--app/serializers/merge_request_widget_commit_entity.rb7
-rw-r--r--app/serializers/merge_request_widget_entity.rb20
-rw-r--r--app/services/merge_requests/merge_service.rb31
-rw-r--r--app/services/merge_requests/squash_service.rb33
-rw-r--r--changelogs/unreleased/56014-better-squash-commit-messages.yml6
-rw-r--r--doc/user/project/merge_requests/squash_and_merge.md16
-rw-r--r--spec/controllers/projects/merge_requests_controller_spec.rb17
-rw-r--r--spec/factories/commits.rb10
-rw-r--r--spec/features/merge_requests/user_squashes_merge_request_spec.rb2
-rw-r--r--spec/fixtures/api/schemas/entities/merge_request_widget.json6
-rw-r--r--spec/javascripts/vue_mr_widget/mock_data.js4
-rw-r--r--spec/models/commit_collection_spec.rb11
-rw-r--r--spec/models/merge_request_spec.rb48
-rw-r--r--spec/serializers/merge_request_widget_commit_entity_spec.rb21
-rw-r--r--spec/serializers/merge_request_widget_entity_spec.rb22
-rw-r--r--spec/services/merge_requests/merge_service_spec.rb2
-rw-r--r--spec/services/merge_requests/squash_service_spec.rb71
24 files changed, 283 insertions, 78 deletions
diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
index ab194e84ab4..36cac230d9d 100644
--- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
+++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
@@ -32,10 +32,10 @@ export default class MergeRequestStore {
this.sourceBranchProtected = data.source_branch_protected;
this.conflictsDocsPath = data.conflicts_docs_path;
this.mergeStatus = data.merge_status;
- this.commitMessage = data.merge_commit_message;
+ this.commitMessage = data.default_merge_commit_message;
this.shortMergeCommitSha = data.short_merge_commit_sha;
this.mergeCommitSha = data.merge_commit_sha;
- this.commitMessageWithDescription = data.merge_commit_message_with_description;
+ this.commitMessageWithDescription = data.default_merge_commit_message_with_description;
this.commitsCount = data.commits_count;
this.divergedCommitsCount = data.diverged_commits_count;
this.pipeline = data.pipeline || {};
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 7c4dc95529a..5cf7fa3422d 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -240,7 +240,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end
def merge_params_attributes
- [:should_remove_source_branch, :commit_message, :squash]
+ [:should_remove_source_branch, :commit_message, :squash_commit_message, :squash]
end
def merge_when_pipeline_succeeds_active?
diff --git a/app/graphql/types/merge_request_type.rb b/app/graphql/types/merge_request_type.rb
index fb740b6fb1c..47b915b451e 100644
--- a/app/graphql/types/merge_request_type.rb
+++ b/app/graphql/types/merge_request_type.rb
@@ -39,7 +39,8 @@ module Types
field :rebase_commit_sha, GraphQL::STRING_TYPE, null: true
field :rebase_in_progress, GraphQL::BOOLEAN_TYPE, method: :rebase_in_progress?, null: false
field :diff_head_sha, GraphQL::STRING_TYPE, null: true
- field :merge_commit_message, GraphQL::STRING_TYPE, null: true
+ field :merge_commit_message, GraphQL::STRING_TYPE, method: :default_merge_commit_message, null: true, deprecation_reason: "Renamed to defaultMergeCommitMessage"
+ field :default_merge_commit_message, GraphQL::STRING_TYPE, null: true
field :merge_ongoing, GraphQL::BOOLEAN_TYPE, method: :merge_ongoing?, null: false
field :source_branch_exists, GraphQL::BOOLEAN_TYPE, method: :source_branch_exists?, null: false
field :mergeable_discussions_state, GraphQL::BOOLEAN_TYPE, null: true
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 982e13e2845..f412d252e5c 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -379,7 +379,7 @@ class Commit
end
def merge_commit?
- parents.size > 1
+ parent_ids.size > 1
end
def merged_merge_request(current_user)
diff --git a/app/models/commit_collection.rb b/app/models/commit_collection.rb
index 885f61beb05..42ec5b5e664 100644
--- a/app/models/commit_collection.rb
+++ b/app/models/commit_collection.rb
@@ -3,6 +3,7 @@
# A collection of Commit instances for a specific project and Git reference.
class CommitCollection
include Enumerable
+ include Gitlab::Utils::StrongMemoize
attr_reader :project, :ref, :commits
@@ -20,11 +21,17 @@ class CommitCollection
end
def committers
- emails = commits.reject(&:merge_commit?).map(&:committer_email).uniq
+ emails = without_merge_commits.map(&:committer_email).uniq
User.by_any_email(emails)
end
+ def without_merge_commits
+ strong_memoize(:without_merge_commits) do
+ commits.reject(&:merge_commit?)
+ end
+ end
+
# Sets the pipeline status for every commit.
#
# Setting this status ahead of time removes the need for running a query for
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 84cb8e1c50b..2035bffd829 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -939,7 +939,7 @@ class MergeRequest < ActiveRecord::Base
self.target_project.repository.branch_exists?(self.target_branch)
end
- def merge_commit_message(include_description: false)
+ def default_merge_commit_message(include_description: false)
closes_issues_references = visible_closing_issues_for.map do |issue|
issue.to_reference(target_project)
end
@@ -959,6 +959,13 @@ class MergeRequest < ActiveRecord::Base
message.join("\n\n")
end
+ # Returns the oldest multi-line commit message, or the MR title if none found
+ def default_squash_commit_message
+ strong_memoize(:default_squash_commit_message) do
+ commits.without_merge_commits.reverse.find(&:description?)&.safe_message || title
+ end
+ end
+
def reset_merge_when_pipeline_succeeds
return unless merge_when_pipeline_succeeds?
@@ -967,6 +974,7 @@ class MergeRequest < ActiveRecord::Base
if merge_params
merge_params.delete('should_remove_source_branch')
merge_params.delete('commit_message')
+ merge_params.delete('squash_commit_message')
end
self.save
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 9ae13fbaa80..bfd2608bed4 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1030,12 +1030,12 @@ class Repository
remote_branch: merge_request.target_branch)
end
- def squash(user, merge_request)
+ def squash(user, merge_request, message)
raw.squash(user, merge_request.id, branch: merge_request.target_branch,
start_sha: merge_request.diff_start_sha,
end_sha: merge_request.diff_head_sha,
author: merge_request.author,
- message: merge_request.title)
+ message: message)
end
def update_submodule(user, submodule, commit_sha, message:, branch:)
diff --git a/app/serializers/merge_request_widget_commit_entity.rb b/app/serializers/merge_request_widget_commit_entity.rb
new file mode 100644
index 00000000000..50a5c44a6ad
--- /dev/null
+++ b/app/serializers/merge_request_widget_commit_entity.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class MergeRequestWidgetCommitEntity < Grape::Entity
+ expose :safe_message, as: :message
+ expose :short_id
+ expose :title
+end
diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb
index f42abf06e1e..2142ceb6122 100644
--- a/app/serializers/merge_request_widget_entity.rb
+++ b/app/serializers/merge_request_widget_entity.rb
@@ -56,10 +56,23 @@ class MergeRequestWidgetEntity < IssuableEntity
merge_request.diff_head_sha.presence
end
- expose :merge_commit_message
expose :actual_head_pipeline, with: PipelineDetailsEntity, as: :pipeline, if: -> (mr, _) { presenter(mr).can_read_pipeline? }
+
expose :merge_pipeline, with: PipelineDetailsEntity, if: ->(mr, _) { mr.merged? && can?(request.current_user, :read_pipeline, mr.target_project)}
+ expose :default_squash_commit_message
+ expose :default_merge_commit_message
+
+ expose :default_merge_commit_message_with_description do |merge_request|
+ merge_request.default_merge_commit_message(include_description: true)
+ end
+
+ expose :commits_without_merge_commits, using: MergeRequestWidgetCommitEntity do |merge_request|
+ merge_request.commits.without_merge_commits
+ end
+
+ expose :commits_count
+
# Booleans
expose :merge_ongoing?, as: :merge_ongoing
expose :work_in_progress?, as: :work_in_progress
@@ -77,7 +90,6 @@ class MergeRequestWidgetEntity < IssuableEntity
end
expose :branch_missing?, as: :branch_missing
- expose :commits_count
expose :cannot_be_merged?, as: :has_conflicts
expose :can_be_merged?, as: :can_be_merged
expose :mergeable?, as: :mergeable
@@ -205,10 +217,6 @@ class MergeRequestWidgetEntity < IssuableEntity
ci_environments_status_project_merge_request_path(merge_request.project, merge_request)
end
- expose :merge_commit_message_with_description do |merge_request|
- merge_request.merge_commit_message(include_description: true)
- end
-
expose :diverged_commits_count do |merge_request|
if merge_request.open? && merge_request.diverged_from_target_branch?
merge_request.diverged_commits_count
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 70a67baa01c..449997bcf07 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -8,6 +8,8 @@ module MergeRequests
# Executed when you do merge via GitLab UI
#
class MergeService < MergeRequests::BaseService
+ include Gitlab::Utils::StrongMemoize
+
MergeError = Class.new(StandardError)
attr_reader :merge_request, :source
@@ -37,15 +39,10 @@ module MergeRequests
end
def source
- return merge_request.diff_head_sha unless merge_request.squash
-
- squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute(merge_request)
-
- case squash_result[:status]
- when :success
- squash_result[:squash_sha]
- when :error
- raise ::MergeRequests::MergeService::MergeError, squash_result[:message]
+ if merge_request.squash
+ squash_sha!
+ else
+ merge_request.diff_head_sha
end
end
@@ -82,8 +79,22 @@ module MergeRequests
merge_request.update!(merge_commit_sha: commit_id)
end
+ def squash_sha!
+ strong_memoize(:squash_sha) do
+ params[:merge_request] = merge_request
+ squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute
+
+ case squash_result[:status]
+ when :success
+ squash_result[:squash_sha]
+ when :error
+ raise ::MergeRequests::MergeService::MergeError, squash_result[:message]
+ end
+ end
+ end
+
def try_merge
- message = params[:commit_message] || merge_request.merge_commit_message
+ message = params[:commit_message] || merge_request.default_merge_commit_message
repository.merge(current_user, source, merge_request, message)
rescue Gitlab::Git::PreReceiveError => e
diff --git a/app/services/merge_requests/squash_service.rb b/app/services/merge_requests/squash_service.rb
index a439a380255..9d1a5d5e6d4 100644
--- a/app/services/merge_requests/squash_service.rb
+++ b/app/services/merge_requests/squash_service.rb
@@ -2,15 +2,10 @@
module MergeRequests
class SquashService < MergeRequests::WorkingCopyBaseService
- def execute(merge_request)
- @merge_request = merge_request
- @repository = target_project.repository
-
- squash || error('Failed to squash. Should be done manually.')
- end
-
- def squash
- if merge_request.commits_count < 2
+ def execute
+ # If performing a squash would result in no change, then
+ # immediately return a success message without performing a squash
+ if merge_request.commits_count < 2 && message.nil?
return success(squash_sha: merge_request.diff_head_sha)
end
@@ -18,7 +13,13 @@ module MergeRequests
return error('Squash task canceled: another squash is already in progress.')
end
- squash_sha = repository.squash(current_user, merge_request)
+ squash! || error('Failed to squash. Should be done manually.')
+ end
+
+ private
+
+ def squash!
+ squash_sha = repository.squash(current_user, merge_request, message || merge_request.default_squash_commit_message)
success(squash_sha: squash_sha)
rescue => e
@@ -26,5 +27,17 @@ module MergeRequests
log_error(e.message)
false
end
+
+ def repository
+ target_project.repository
+ end
+
+ def merge_request
+ params[:merge_request]
+ end
+
+ def message
+ params[:squash_commit_message].presence
+ end
end
end
diff --git a/changelogs/unreleased/56014-better-squash-commit-messages.yml b/changelogs/unreleased/56014-better-squash-commit-messages.yml
new file mode 100644
index 00000000000..b08d584ac0a
--- /dev/null
+++ b/changelogs/unreleased/56014-better-squash-commit-messages.yml
@@ -0,0 +1,6 @@
+---
+title: Default squash commit message is now selected from the longest commit when
+ squashing merge requests
+merge_request: 24518
+author:
+type: changed
diff --git a/doc/user/project/merge_requests/squash_and_merge.md b/doc/user/project/merge_requests/squash_and_merge.md
index 1b57331dbe7..34cba867e2c 100644
--- a/doc/user/project/merge_requests/squash_and_merge.md
+++ b/doc/user/project/merge_requests/squash_and_merge.md
@@ -18,10 +18,14 @@ Into a single commit on merge:
![A squashed commit followed by a merge commit][squashed-commit]
-The squashed commit's commit message is the merge request title. And note that
-the squashed commit is still followed by a merge commit, as the merge
-method for this example repository uses a merge commit. Squashing also works
-with the fast-forward merge strategy, see
+The squashed commit's commit message will be either:
+
+- Taken from the first multi-line commit message in the merge.
+- The merge request's title if no multi-line commit message is found.
+
+Note that the squashed commit is still followed by a merge commit,
+as the merge method for this example repository uses a merge commit.
+Squashing also works with the fast-forward merge strategy, see
[squashing and fast-forward merge](#squash-and-fast-forward-merge) for more
details.
@@ -34,7 +38,7 @@ you'd rather not include them in your target branch.
With squash and merge, when the merge request is ready to be merged,
all you have to do is enable squashing before you press merge to join
-the commits include in the merge request into a single commit.
+the commits in the merge request into a single commit.
This way, the history of your base branch remains clean with
meaningful commit messages and is simpler to [revert] if necessary.
@@ -56,7 +60,7 @@ This can then be overridden at the time of accepting the merge request:
The squashed commit has the following metadata:
-- Message: the title of the merge request.
+- Message: the message of the squash commit.
- Author: the author of the merge request.
- Committer: the user who initiated the squash.
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index ca5ff9b1e3b..79f97aa4170 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -387,6 +387,23 @@ describe Projects::MergeRequestsController do
end
end
+ context 'when a squash commit message is passed' do
+ let(:message) { 'My custom squash commit message' }
+
+ it 'passes the same message to SquashService' do
+ params = { squash: '1', squash_commit_message: message }
+
+ expect_next_instance_of(MergeRequests::SquashService, project, user, params.merge(merge_request: merge_request)) do |squash_service|
+ expect(squash_service).to receive(:execute).and_return({
+ status: :success,
+ squash_sha: SecureRandom.hex(20)
+ })
+ end
+
+ merge_with_sha(params)
+ end
+ end
+
context 'when the pipeline succeeds is passed' do
let!(:head_pipeline) do
create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request)
diff --git a/spec/factories/commits.rb b/spec/factories/commits.rb
index 818f7b046f6..2bcc4b6cf52 100644
--- a/spec/factories/commits.rb
+++ b/spec/factories/commits.rb
@@ -16,14 +16,24 @@ FactoryBot.define do
commit
end
+
project
+ skip_create # Commits cannot be persisted
+
initialize_with do
new(git_commit, project)
end
after(:build) do |commit, evaluator|
allow(commit).to receive(:author).and_return(evaluator.author || build_stubbed(:author))
+ allow(commit).to receive(:parent_ids).and_return([])
+ end
+
+ trait :merge_commit do
+ after(:build) do |commit|
+ allow(commit).to receive(:parent_ids).and_return(Array.new(2) { SecureRandom.hex(20) })
+ end
end
trait :without_author do
diff --git a/spec/features/merge_requests/user_squashes_merge_request_spec.rb b/spec/features/merge_requests/user_squashes_merge_request_spec.rb
index 47f9f10815c..bf9c55cf22c 100644
--- a/spec/features/merge_requests/user_squashes_merge_request_spec.rb
+++ b/spec/features/merge_requests/user_squashes_merge_request_spec.rb
@@ -14,7 +14,7 @@ describe 'User squashes a merge request', :js do
latest_master_commits = project.repository.commits_between(original_head.sha, 'master').map(&:raw)
squash_commit = an_object_having_attributes(sha: a_string_matching(/\h{40}/),
- message: "Csv\n",
+ message: a_string_starting_with(project.merge_requests.first.default_squash_commit_message),
author_name: user.name,
committer_name: user.name)
diff --git a/spec/fixtures/api/schemas/entities/merge_request_widget.json b/spec/fixtures/api/schemas/entities/merge_request_widget.json
index 1bd39a46830..67c209f3fc3 100644
--- a/spec/fixtures/api/schemas/entities/merge_request_widget.json
+++ b/spec/fixtures/api/schemas/entities/merge_request_widget.json
@@ -44,7 +44,7 @@
"merge_user": { "type": ["object", "null"] },
"diff_head_sha": { "type": ["string", "null"] },
"diff_head_commit_short_id": { "type": ["string", "null"] },
- "merge_commit_message": { "type": ["string", "null"] },
+ "default_merge_commit_message": { "type": ["string", "null"] },
"pipeline": { "type": ["object", "null"] },
"merge_pipeline": { "type": ["object", "null"] },
"work_in_progress": { "type": "boolean" },
@@ -102,7 +102,9 @@
"new_blob_path": { "type": ["string", "null"] },
"merge_check_path": { "type": "string" },
"ci_environments_status_path": { "type": "string" },
- "merge_commit_message_with_description": { "type": "string" },
+ "default_merge_commit_message_with_description": { "type": "string" },
+ "default_squash_commit_message": { "type": "string" },
+ "commits_without_merge_commits": { "type": "array" },
"diverged_commits_count": { "type": "integer" },
"commit_change_content_path": { "type": "string" },
"merge_commit_path": { "type": ["string", "null"] },
diff --git a/spec/javascripts/vue_mr_widget/mock_data.js b/spec/javascripts/vue_mr_widget/mock_data.js
index 072e98fc0e8..75b197fb2ba 100644
--- a/spec/javascripts/vue_mr_widget/mock_data.js
+++ b/spec/javascripts/vue_mr_widget/mock_data.js
@@ -58,7 +58,7 @@ export default {
merge_user: null,
diff_head_sha: '104096c51715e12e7ae41f9333e9fa35b73f385d',
diff_head_commit_short_id: '104096c5',
- merge_commit_message:
+ default_merge_commit_message:
"Merge branch 'daaaa' into 'master'\n\nUpdate README.md\n\nSee merge request !22",
pipeline: {
id: 172,
@@ -213,7 +213,7 @@ export default {
merge_check_path: '/root/acets-app/merge_requests/22/merge_check',
ci_environments_status_url: '/root/acets-app/merge_requests/22/ci_environments_status',
project_archived: false,
- merge_commit_message_with_description:
+ default_merge_commit_message_with_description:
"Merge branch 'daaaa' into 'master'\n\nUpdate README.md\n\nSee merge request !22",
diverged_commits_count: 0,
only_allow_merge_if_pipeline_succeeds: false,
diff --git a/spec/models/commit_collection_spec.rb b/spec/models/commit_collection_spec.rb
index 005005b236b..12e59b35428 100644
--- a/spec/models/commit_collection_spec.rb
+++ b/spec/models/commit_collection_spec.rb
@@ -35,6 +35,17 @@ describe CommitCollection do
end
end
+ describe '#without_merge_commits' do
+ it 'returns all commits except merge commits' do
+ collection = described_class.new(project, [
+ build(:commit),
+ build(:commit, :merge_commit)
+ ])
+
+ expect(collection.without_merge_commits.size).to eq(1)
+ end
+ end
+
describe '#with_pipeline_status' do
it 'sets the pipeline status for every commit so no additional queries are necessary' do
create(
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index b62f973ad1e..afa87b8a62d 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -82,6 +82,38 @@ describe MergeRequest do
end
end
+ describe '#default_squash_commit_message' do
+ let(:project) { subject.project }
+
+ def commit_collection(commit_hashes)
+ raw_commits = commit_hashes.map { |raw| Commit.from_hash(raw, project) }
+
+ CommitCollection.new(project, raw_commits)
+ end
+
+ it 'returns the oldest multiline commit message' do
+ commits = commit_collection([
+ { message: 'Singleline', parent_ids: [] },
+ { message: "Second multiline\nCommit message", parent_ids: [] },
+ { message: "First multiline\nCommit message", parent_ids: [] }
+ ])
+
+ expect(subject).to receive(:commits).and_return(commits)
+
+ expect(subject.default_squash_commit_message).to eq("First multiline\nCommit message")
+ end
+
+ it 'returns the merge request title if there are no multiline commits' do
+ commits = commit_collection([
+ { message: 'Singleline', parent_ids: [] }
+ ])
+
+ expect(subject).to receive(:commits).and_return(commits)
+
+ expect(subject.default_squash_commit_message).to eq(subject.title)
+ end
+ end
+
describe 'modules' do
subject { described_class }
@@ -920,18 +952,18 @@ describe MergeRequest do
end
end
- describe '#merge_commit_message' do
+ describe '#default_merge_commit_message' do
it 'includes merge information as the title' do
request = build(:merge_request, source_branch: 'source', target_branch: 'target')
- expect(request.merge_commit_message)
+ expect(request.default_merge_commit_message)
.to match("Merge branch 'source' into 'target'\n\n")
end
it 'includes its title in the body' do
request = build(:merge_request, title: 'Remove all technical debt')
- expect(request.merge_commit_message)
+ expect(request.default_merge_commit_message)
.to match("Remove all technical debt\n\n")
end
@@ -943,34 +975,34 @@ describe MergeRequest do
allow(subject.project).to receive(:default_branch).and_return(subject.target_branch)
subject.cache_merge_request_closes_issues!
- expect(subject.merge_commit_message)
+ expect(subject.default_merge_commit_message)
.to match("Closes #{issue.to_reference}")
end
it 'includes its reference in the body' do
request = build_stubbed(:merge_request)
- expect(request.merge_commit_message)
+ expect(request.default_merge_commit_message)
.to match("See merge request #{request.to_reference(full: true)}")
end
it 'excludes multiple linebreak runs when description is blank' do
request = build(:merge_request, title: 'Title', description: nil)
- expect(request.merge_commit_message).not_to match("Title\n\n\n\n")
+ expect(request.default_merge_commit_message).not_to match("Title\n\n\n\n")
end
it 'includes its description in the body' do
request = build(:merge_request, description: 'By removing all code')
- expect(request.merge_commit_message(include_description: true))
+ expect(request.default_merge_commit_message(include_description: true))
.to match("By removing all code\n\n")
end
it 'does not includes its description in the body' do
request = build(:merge_request, description: 'By removing all code')
- expect(request.merge_commit_message)
+ expect(request.default_merge_commit_message)
.not_to match("By removing all code\n\n")
end
end
diff --git a/spec/serializers/merge_request_widget_commit_entity_spec.rb b/spec/serializers/merge_request_widget_commit_entity_spec.rb
new file mode 100644
index 00000000000..ce83978c49a
--- /dev/null
+++ b/spec/serializers/merge_request_widget_commit_entity_spec.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequestWidgetCommitEntity do
+ let(:project) { create(:project, :repository) }
+ let(:commit) { project.commit }
+ let(:request) { double('request') }
+
+ let(:entity) do
+ described_class.new(commit, request: request)
+ end
+
+ context 'as json' do
+ subject { entity.as_json }
+
+ it { expect(subject[:message]).to eq(commit.safe_message) }
+ it { expect(subject[:short_id]).to eq(commit.short_id) }
+ it { expect(subject[:title]).to eq(commit.title) }
+ end
+end
diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb
index 376698a16df..4dbd79f2fc0 100644
--- a/spec/serializers/merge_request_widget_entity_spec.rb
+++ b/spec/serializers/merge_request_widget_entity_spec.rb
@@ -188,9 +188,14 @@ describe MergeRequestWidgetEntity do
.to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}.diff")
end
- it 'has merge_commit_message_with_description' do
- expect(subject[:merge_commit_message_with_description])
- .to eq(resource.merge_commit_message(include_description: true))
+ it 'has default_merge_commit_message_with_description' do
+ expect(subject[:default_merge_commit_message_with_description])
+ .to eq(resource.default_merge_commit_message(include_description: true))
+ end
+
+ it 'has default_squash_commit_message' do
+ expect(subject[:default_squash_commit_message])
+ .to eq(resource.default_squash_commit_message)
end
describe 'new_blob_path' do
@@ -272,4 +277,15 @@ describe MergeRequestWidgetEntity do
expect(entity[:rebase_path]).to be_nil
end
end
+
+ describe 'commits_without_merge_commits' do
+ it 'should not include merge commits' do
+ # Mock all but the first 5 commits to be merge commits
+ resource.commits.each_with_index do |commit, i|
+ expect(commit).to receive(:merge_commit?).at_least(:once).and_return(i > 4)
+ end
+
+ expect(subject[:commits_without_merge_commits].size).to eq(5)
+ end
+ end
end
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index 5d96b5ce27c..04a62aa454d 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -258,7 +258,7 @@ describe MergeRequests::MergeService do
it 'logs and saves error if there is an error when squashing' do
error_message = 'Failed to squash. Should be done manually'
- allow_any_instance_of(MergeRequests::SquashService).to receive(:squash).and_return(nil)
+ allow_any_instance_of(MergeRequests::SquashService).to receive(:squash!).and_return(nil)
merge_request.update(squash: true)
service.execute(merge_request)
diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb
index 53bce15735c..2713652873e 100644
--- a/spec/services/merge_requests/squash_service_spec.rb
+++ b/spec/services/merge_requests/squash_service_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
describe MergeRequests::SquashService do
include GitHelpers
- let(:service) { described_class.new(project, user, {}) }
+ let(:service) { described_class.new(project, user, { merge_request: merge_request }) }
let(:user) { project.owner }
let(:project) { create(:project, :repository) }
let(:repository) { project.repository.raw }
@@ -31,32 +31,49 @@ describe MergeRequests::SquashService do
shared_examples 'the squash succeeds' do
it 'returns the squashed commit SHA' do
- result = service.execute(merge_request)
+ result = service.execute
expect(result).to match(status: :success, squash_sha: a_string_matching(/\h{40}/))
expect(result[:squash_sha]).not_to eq(merge_request.diff_head_sha)
end
it 'cleans up the temporary directory' do
- service.execute(merge_request)
+ service.execute
expect(File.exist?(squash_dir_path)).to be(false)
end
it 'does not keep the branch push event' do
- expect { service.execute(merge_request) }.not_to change { Event.count }
+ expect { service.execute }.not_to change { Event.count }
+ end
+
+ context 'when there is a single commit in the merge request' do
+ before do
+ expect(merge_request).to receive(:commits_count).at_least(:once).and_return(1)
+ end
+
+ it 'will skip performing the squash, as the outcome would be the same' do
+ expect(merge_request.target_project.repository).not_to receive(:squash)
+
+ service.execute
+ end
+
+ it 'will still perform the squash when a custom squash commit message has been provided' do
+ service = described_class.new(project, user, { merge_request: merge_request, squash_commit_message: 'A custom commit message' })
+
+ expect(merge_request.target_project.repository).to receive(:squash).and_return('sha')
+
+ service.execute
+ end
end
context 'the squashed commit' do
- let(:squash_sha) { service.execute(merge_request)[:squash_sha] }
+ let(:squash_sha) { service.execute[:squash_sha] }
let(:squash_commit) { project.repository.commit(squash_sha) }
- it 'copies the author info and message from the merge request' do
+ it 'copies the author info from the merge request' do
expect(squash_commit.author_name).to eq(merge_request.author.name)
expect(squash_commit.author_email).to eq(merge_request.author.email)
-
- # Commit messages have a trailing newline, but titles don't.
- expect(squash_commit.message.chomp).to eq(merge_request.title)
end
it 'sets the current user as the committer' do
@@ -72,21 +89,37 @@ describe MergeRequests::SquashService do
expect(squash_diff.patch.length).to eq(mr_diff.patch.length)
expect(squash_commit.sha).not_to eq(merge_request.diff_head_sha)
end
+
+ it 'has a default squash commit message if no message was provided' do
+ expect(squash_commit.message.chomp).to eq(merge_request.default_squash_commit_message.chomp)
+ end
+
+ context 'if a message was provided' do
+ let(:service) { described_class.new(project, user, { merge_request: merge_request, squash_commit_message: message }) }
+ let(:message) { 'My custom message' }
+ let(:squash_sha) { service.execute[:squash_sha] }
+
+ it 'has the same message as the message provided' do
+ expect(squash_commit.message.chomp).to eq(message)
+ end
+ end
end
end
describe '#execute' do
context 'when there is only one commit in the merge request' do
+ let(:merge_request) { merge_request_with_one_commit }
+
it 'returns that commit SHA' do
- result = service.execute(merge_request_with_one_commit)
+ result = service.execute
- expect(result).to match(status: :success, squash_sha: merge_request_with_one_commit.diff_head_sha)
+ expect(result).to match(status: :success, squash_sha: merge_request.diff_head_sha)
end
it 'does not perform any git actions' do
expect(repository).not_to receive(:popen)
- service.execute(merge_request_with_one_commit)
+ service.execute
end
end
@@ -116,12 +149,11 @@ describe MergeRequests::SquashService do
expect(service).to receive(:log_error).with(log_error)
expect(service).to receive(:log_error).with(error)
- service.execute(merge_request)
+ service.execute
end
it 'returns an error' do
- expect(service.execute(merge_request)).to match(status: :error,
- message: a_string_including('squash'))
+ expect(service.execute).to match(status: :error, message: a_string_including('squash'))
end
end
end
@@ -131,23 +163,22 @@ describe MergeRequests::SquashService do
let(:error) { 'A test error' }
before do
- allow(merge_request).to receive(:commits_count).and_raise(error)
+ allow(merge_request.target_project.repository).to receive(:squash).and_raise(error)
end
it 'logs the MR reference and exception' do
expect(service).to receive(:log_error).with(a_string_including("#{project.full_path}#{merge_request.to_reference}"))
expect(service).to receive(:log_error).with(error)
- service.execute(merge_request)
+ service.execute
end
it 'returns an error' do
- expect(service.execute(merge_request)).to match(status: :error,
- message: a_string_including('squash'))
+ expect(service.execute).to match(status: :error, message: a_string_including('squash'))
end
it 'cleans up the temporary directory' do
- service.execute(merge_request)
+ service.execute
expect(File.exist?(squash_dir_path)).to be(false)
end