Allow custom squash commit messages

11 files changed, 177 insertions, 37 deletions

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