Merge branch '56014-better-squash-commit-messages' into 'master'

Allow custom squash commit messages

See merge request gitlab-org/gitlab-ce!24518

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 || {};

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?

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

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)

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

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

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:)

app/serializers/merge_request_widget_commit_entity.rb

+# frozen_string_literal: true
+
+class MergeRequestWidgetCommitEntity < Grape::Entity
+  expose :safe_message, as: :message
+  expose :short_id
+  expose :title
+end

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

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

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

changelogs/unreleased/56014-better-squash-commit-messages.yml

+---
+title: Default squash commit message is now selected from the longest commit when
+  squashing merge requests
+merge_request: 24518
+author:
+type: changed

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.
 

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)

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

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)
 

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"] },

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,

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(

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

spec/serializers/merge_request_widget_commit_entity_spec.rb

+# 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

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

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)

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