Merge branch '31671-merge-request-message-contains-carriage-returns' into 'master'

removes the possibility of commit messages having carriage returns

Closes #31671

See merge request !11077

app/models/repository.rb

@@ -789,7 +789,7 @@ class Repository
       }
       options.merge!(get_committer_and_author(user, email: author_email, name: author_name))
 
-      Rugged::Commit.create(rugged, options)
+      create_commit(options)
     end
   end
   # rubocop:enable Metrics/ParameterLists
@@ -836,7 +836,7 @@ class Repository
         tree: merge_index.write_tree(rugged),
       )
 
-      commit_id = Rugged::Commit.create(rugged, actual_options)
+      commit_id = create_commit(actual_options)
       merge_request.update(in_progress_merge_commit_sha: commit_id)
       commit_id
     end
@@ -859,12 +859,11 @@ class Repository
 
       committer = user_to_committer(user)
 
-      Rugged::Commit.create(rugged,
+      create_commit(message: commit.revert_message(user),
-        message: commit.revert_message(user),
+                    author: committer,
-        author: committer,
+                    committer: committer,
-        committer: committer,
+                    tree: revert_tree_id,
-        tree: revert_tree_id,
+                    parents: [start_commit.sha])
-        parents: [start_commit.sha])
     end
   end
 
@@ -883,16 +882,15 @@ class Repository
 
       committer = user_to_committer(user)
 
-      Rugged::Commit.create(rugged,
+      create_commit(message: commit.message,
-        message: commit.message,
+                    author: {
-        author: {
+                        email: commit.author_email,
-          email: commit.author_email,
+                        name: commit.author_name,
-          name: commit.author_name,
+                        time: commit.authored_date
-          time: commit.authored_date
+                    },
-        },
+                    committer: committer,
-        committer: committer,
+                    tree: cherry_pick_tree_id,
-        tree: cherry_pick_tree_id,
+                    parents: [start_commit.sha])
-        parents: [start_commit.sha])
     end
   end
 
@@ -900,7 +898,7 @@ class Repository
     GitOperationService.new(user, self).with_branch(branch_name) do
       committer = user_to_committer(user)
 
-      Rugged::Commit.create(rugged, params.merge(author: committer, committer: committer))
+      create_commit(params.merge(author: committer, committer: committer))
     end
   end
 
@@ -1142,6 +1140,12 @@ class Repository
     Gitlab::Metrics.add_event(event, { path: path_with_namespace }.merge(tags))
   end
 
+  def create_commit(params = {})
+    params[:message].delete!("\r")
+
+    Rugged::Commit.create(rugged, params)
+  end
+
   def repository_storage_path
     @project.repository_storage_path
   end

changelogs/unreleased/31671-merge-request-message-contains-carriage-returns.yml

@@ -0,0 +1,4 @@
+---
+title: Remove carriage returns from commit messages
+merge_request: 11077
+author:

lib/gitlab/git/repository.rb

@@ -876,27 +876,6 @@ module Gitlab
         rugged.remotes[remote_name].push(refspecs)
       end
 
-      # Merge the +source_name+ branch into the +target_name+ branch. This is
-      # equivalent to `git merge --no_ff +source_name+`, since a merge commit
-      # is always created.
-      def merge(source_name, target_name, options = {})
-        our_commit = rugged.branches[target_name].target
-        their_commit = rugged.branches[source_name].target
-
-        raise "Invalid merge target" if our_commit.nil?
-        raise "Invalid merge source" if their_commit.nil?
-
-        merge_index = rugged.merge_commits(our_commit, their_commit)
-        return false if merge_index.conflicts?
-
-        actual_options = options.merge(
-          parents: [our_commit, their_commit],
-          tree: merge_index.write_tree(rugged),
-          update_ref: "refs/heads/#{target_name}"
-        )
-        Rugged::Commit.create(rugged, actual_options)
-      end
-
       AUTOCRLF_VALUES = {
         "true" => true,
         "false" => false,

spec/models/repository_spec.rb

@@ -1098,21 +1098,33 @@ describe Repository, models: true do
   end
 
   describe '#merge' do
-    it 'merges the code and return the commit id' do
+    let(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project) }
+
+    let(:commit_options) do
+      author = repository.user_to_committer(user)
+      { message: 'Test \r\n\r\n message', committer: author, author: author }
+    end
+
+    it 'merges the code and returns the commit id' do
       expect(merge_commit).to be_present
       expect(repository.blob_at(merge_commit.id, 'files/ruby/feature.rb')).to be_present
     end
 
     it 'sets the `in_progress_merge_commit_sha` flag for the given merge request' do
-      merge_request = create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project)
+      merge_commit_id = merge(repository, user, merge_request, commit_options)
-
-      merge_commit_id = repository.merge(user,
-                                         merge_request.diff_head_sha,
-                                         merge_request,
-                                         commit_options)
 
       expect(merge_request.in_progress_merge_commit_sha).to eq(merge_commit_id)
     end
+
+    it 'removes carriage returns from commit message' do
+      merge_commit_id = merge(repository, user, merge_request, commit_options)
+
+      expect(repository.commit(merge_commit_id).message).to eq(commit_options[:message].delete("\r"))
+    end
+
+    def merge(repository, user, merge_request, options = {})
+      repository.merge(user, merge_request.diff_head_sha, merge_request, options)
+    end
   end
 
   describe '#revert' do