Merge branch 'operation-service-merge' into 'master'

Prepare Repository#merge for migration to Gitaly

Closes gitaly#559

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

app/models/repository.rb

@@ -768,17 +768,23 @@ class Repository
     multi_action(**options)
   end
 
-  def with_branch(user, *args)
-    result = Gitlab::Git::OperationService.new(user, raw_repository).with_branch(*args) do |start_commit|
-      yield start_commit
-    end
+  def with_cache_hooks
+    result = yield
 
-    newrev, should_run_after_create, should_run_after_create_branch = result
+    return unless result
 
-    after_create if should_run_after_create
-    after_create_branch if should_run_after_create_branch
+    after_create if result.repo_created?
+    after_create_branch if result.branch_created?
 
-    newrev
+    result.newrev
+  end
+
+  def with_branch(user, *args)
+    with_cache_hooks do
+      Gitlab::Git::OperationService.new(user, raw_repository).with_branch(*args) do |start_commit|
+        yield start_commit
+      end
+    end
   end
 
   # rubocop:disable Metrics/ParameterLists
@@ -843,30 +849,13 @@ class Repository
     end
   end
 
-  def merge(user, source, merge_request, options = {})
-    with_branch(
-      user,
-      merge_request.target_branch) do |start_commit|
-      our_commit = start_commit.sha
-      their_commit = source
-
-      raise 'Invalid merge target' unless our_commit
-      raise 'Invalid merge source' unless their_commit
-
-      merge_index = rugged.merge_commits(our_commit, their_commit)
-      break if merge_index.conflicts?
-
-      actual_options = options.merge(
-        parents: [our_commit, their_commit],
-        tree: merge_index.write_tree(rugged)
-      )
-
-      commit_id = create_commit(actual_options)
+  def merge(user, source_sha, merge_request, message)
+    with_cache_hooks do
+      raw_repository.merge(user, source_sha, merge_request.target_branch, message) do |commit_id|
         merge_request.update(in_progress_merge_commit_sha: commit_id)
-      commit_id
+        nil # Return value does not matter.
+      end
     end
-  rescue Gitlab::Git::CommitError # when merge_index.conflicts?
-    false
   end
 
   def revert(
@@ -1157,12 +1146,6 @@ class Repository
     Gitlab::Metrics.add_event(event, { path: full_path }.merge(tags))
   end
 
-  def create_commit(params = {})
-    params[:message].delete!("\r")
-
-    Rugged::Commit.create(rugged, params)
-  end
-
   def last_commit_for_path_by_gitaly(sha, path)
     c = raw_repository.gitaly_commit_client.last_commit_for_path(sha, path)
     commit(c)

app/services/merge_requests/merge_service.rb

@@ -38,15 +38,9 @@ module MergeRequests
     private
 
     def commit
-      committer = repository.user_to_committer(current_user)
+      message = params[:commit_message] || merge_request.merge_commit_message
 
-      options = {
-        message: params[:commit_message] || merge_request.merge_commit_message,
-        author: committer,
-        committer: committer
-      }
-
-      commit_id = repository.merge(current_user, source, merge_request, options)
+      commit_id = repository.merge(current_user, source, merge_request, message)
 
       raise MergeError, 'Conflicts detected during merge' unless commit_id
 

lib/gitlab/git/operation_service.rb

 module Gitlab
   module Git
     class OperationService
+      WithBranchResult = Struct.new(:newrev, :repo_created, :branch_created) do
+        alias_method :repo_created?, :repo_created
+        alias_method :branch_created?, :branch_created
+      end
+
       attr_reader :user, :repository
 
       def initialize(user, new_repository)
@@ -107,7 +112,7 @@ module Gitlab
         ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
         update_ref_in_hooks(ref, newrev, oldrev)
 
-        [newrev, was_empty, was_empty || Gitlab::Git.blank_ref?(oldrev)]
+        WithBranchResult.new(newrev, was_empty, was_empty || Gitlab::Git.blank_ref?(oldrev))
       end
 
       def find_oldrev_from_branch(newrev, branch)

lib/gitlab/git/repository.rb

@@ -653,6 +653,43 @@ module Gitlab
         tags.find { |tag| tag.name == name }
       end
 
+      def merge(user, source_sha, target_branch, message)
+        committer = Gitlab::Git.committer_hash(email: user.email, name: user.name)
+
+        OperationService.new(user, self).with_branch(target_branch) do |start_commit|
+          our_commit = start_commit.sha
+          their_commit = source_sha
+
+          raise 'Invalid merge target' unless our_commit
+          raise 'Invalid merge source' unless their_commit
+
+          merge_index = rugged.merge_commits(our_commit, their_commit)
+          break if merge_index.conflicts?
+
+          options = {
+            parents: [our_commit, their_commit],
+            tree: merge_index.write_tree(rugged),
+            message: message,
+            author: committer,
+            committer: committer
+          }
+
+          commit_id = create_commit(options)
+
+          yield commit_id
+
+          commit_id
+        end
+      rescue Gitlab::Git::CommitError # when merge_index.conflicts?
+        nil
+      end
+
+      def create_commit(params = {})
+        params[:message].delete!("\r")
+
+        Rugged::Commit.create(rugged, params)
+      end
+
       # Delete the specified branch from the repository
       def delete_branch(branch_name)
         gitaly_migrate(:delete_branch) do |is_enabled|

spec/lib/gitlab/diff/position_tracer_spec.rb

@@ -1761,17 +1761,9 @@ describe Gitlab::Diff::PositionTracer do
       let(:merge_commit) do
         update_file_again_commit
 
-        committer = repository.user_to_committer(current_user)
-
-        options = {
-          message: "Merge branches",
-          author: committer,
-          committer: committer
-        }
-
         merge_request = create(:merge_request, source_branch: second_create_file_commit.sha, target_branch: branch_name, source_project: project)
 
-        repository.merge(current_user, merge_request.diff_head_sha, merge_request, options)
+        repository.merge(current_user, merge_request.diff_head_sha, merge_request, "Merge branches")
 
         project.commit(branch_name)
       end

spec/models/commit_spec.rb

@@ -207,11 +207,6 @@ eos
     context 'of a merge commit' do
       let(:repository) { project.repository }
 
-      let(:commit_options) do
-        author = repository.user_to_committer(user)
-        { message: 'Test message', committer: author, author: author }
-      end
-
       let(:merge_request) do
         create(:merge_request,
                source_branch: 'video',
@@ -224,7 +219,7 @@ eos
         merge_commit_id = repository.merge(user,
                                            merge_request.diff_head_sha,
                                            merge_request,
-                                           commit_options)
+                                           'Test message')
 
         repository.commit(merge_commit_id)
       end

spec/models/repository_spec.rb

@@ -10,10 +10,7 @@ describe Repository, models: true do
   let(:user) { create(:user) }
   let(:git_user) { Gitlab::Git::User.from_gitlab(user) }
 
-  let(:commit_options) do
-    author = repository.user_to_committer(user)
-    { message: 'Test message', committer: author, author: author }
-  end
+  let(:message) { 'Test message' }
 
   let(:merge_commit) do
     merge_request = create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project)
@@ -21,7 +18,7 @@ describe Repository, models: true do
     merge_commit_id = repository.merge(user,
                                        merge_request.diff_head_sha,
                                        merge_request,
-                                       commit_options)
+                                       message)
 
     repository.commit(merge_commit_id)
   end
@@ -1287,10 +1284,7 @@ describe Repository, models: true do
   describe '#merge' 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
+    let(:message) { 'Test \r\n\r\n message' }
 
     it 'merges the code and returns the commit id' do
       expect(merge_commit).to be_present
@@ -1298,19 +1292,19 @@ describe Repository, models: true do
     end
 
     it 'sets the `in_progress_merge_commit_sha` flag for the given merge request' do
-      merge_commit_id = merge(repository, user, merge_request, commit_options)
+      merge_commit_id = merge(repository, user, merge_request, message)
 
       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)
+      merge_commit_id = merge(repository, user, merge_request, message)
 
-      expect(repository.commit(merge_commit_id).message).to eq(commit_options[:message].delete("\r"))
+      expect(repository.commit(merge_commit_id).message).to eq(message.delete("\r"))
     end
 
-    def merge(repository, user, merge_request, options = {})
-      repository.merge(user, merge_request.diff_head_sha, merge_request, options)
+    def merge(repository, user, merge_request, message)
+      repository.merge(user, merge_request.diff_head_sha, merge_request, message)
     end
   end
 

spec/services/merge_requests/refresh_service_spec.rb

@@ -150,9 +150,7 @@ describe MergeRequests::RefreshService do
     context 'manual merge of source branch' do
       before do
         # Merge master -> feature branch
-        author = { email: 'test@gitlab.com', time: Time.now, name: "Me" }
-        commit_options = { message: 'Test message', committer: author, author: author }
-        @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, commit_options)
+        @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, 'Test message')
         commit = @project.repository.commit('feature')
         service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature')
         reload_mrs