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

(EE) Prepare Repository#merge for migration to Gitaly

See merge request gitlab-org/gitlab-ee!2934

app/models/repository.rb

@@ -173,7 +173,7 @@ class Repository
   end
 
   def add_branch(user, branch_name, ref)
-    branch = raw_repository.add_branch(branch_name, committer: user, target: ref)
+    branch = raw_repository.add_branch(branch_name, user: user, target: ref)
 
     after_create_branch
 
@@ -183,7 +183,7 @@ class Repository
   end
 
   def add_tag(user, tag_name, target, message = nil)
-    raw_repository.add_tag(tag_name, committer: user, target: target, message: message)
+    raw_repository.add_tag(tag_name, user: user, target: target, message: message)
   rescue Gitlab::Git::Repository::InvalidRef
     false
   end
@@ -191,7 +191,7 @@ class Repository
   def rm_branch(user, branch_name)
     before_remove_branch
 
-    raw_repository.rm_branch(branch_name, committer: user)
+    raw_repository.rm_branch(branch_name, user: user)
 
     after_remove_branch
     true
@@ -200,7 +200,7 @@ class Repository
   def rm_tag(user, tag_name)
     before_remove_tag
 
-    raw_repository.rm_tag(tag_name, committer: user)
+    raw_repository.rm_tag(tag_name, user: user)
 
     after_remove_tag
     true
@@ -769,17 +769,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
@@ -863,30 +869,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)
-      merge_request.update(in_progress_merge_commit_sha: commit_id)
-      commit_id
+  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)
+        nil # Return value does not matter.
+      end
     end
-  rescue Gitlab::Git::CommitError # when merge_index.conflicts?
-    false
   end
 
   def revert(
@@ -1231,12 +1220,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

@@ -67,15 +67,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
-      attr_reader :committer, :repository
+      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(committer, new_repository)
-        committer = Gitlab::Git::Committer.from_user(committer) if committer.is_a?(User)
-        @committer = committer
+      def initialize(user, new_repository)
+        if user
+          user = Gitlab::Git::User.from_gitlab(user) unless user.respond_to?(:gl_id)
+          @user = user
+        end
 
         # Refactoring aid
         unless new_repository.is_a?(Gitlab::Git::Repository)
@@ -105,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)
@@ -128,7 +135,7 @@ module Gitlab
 
       def with_hooks(ref, newrev, oldrev)
         Gitlab::Git::HooksService.new.execute(
-          committer,
+          user,
           repository,
           oldrev,
           newrev,

lib/gitlab/git/repository.rb

@@ -610,49 +610,86 @@ module Gitlab
         # TODO: implement this method
       end
 
-      def add_branch(branch_name, committer:, target:)
+      def add_branch(branch_name, user:, target:)
         target_object = Ref.dereference_object(lookup(target))
         raise InvalidRef.new("target not found: #{target}") unless target_object
 
-        OperationService.new(committer, self).add_branch(branch_name, target_object.oid)
+        OperationService.new(user, self).add_branch(branch_name, target_object.oid)
         find_branch(branch_name)
       rescue Rugged::ReferenceError => ex
         raise InvalidRef, ex
       end
 
-      def add_tag(tag_name, committer:, target:, message: nil)
+      def add_tag(tag_name, user:, target:, message: nil)
         target_object = Ref.dereference_object(lookup(target))
         raise InvalidRef.new("target not found: #{target}") unless target_object
 
-        committer = Committer.from_user(committer) if committer.is_a?(User)
+        user = Gitlab::Git::User.from_gitlab(user) unless user.respond_to?(:gl_id)
 
         options = nil # Use nil, not the empty hash. Rugged cares about this.
         if message
           options = {
             message: message,
-            tagger: Gitlab::Git.committer_hash(email: committer.email, name: committer.name)
+            tagger: Gitlab::Git.committer_hash(email: user.email, name: user.name)
           }
         end
 
-        OperationService.new(committer, self).add_tag(tag_name, target_object.oid, options)
+        OperationService.new(user, self).add_tag(tag_name, target_object.oid, options)
 
         find_tag(tag_name)
       rescue Rugged::ReferenceError => ex
         raise InvalidRef, ex
       end
 
-      def rm_branch(branch_name, committer:)
-        OperationService.new(committer, self).rm_branch(find_branch(branch_name))
+      def rm_branch(branch_name, user:)
+        OperationService.new(user, self).rm_branch(find_branch(branch_name))
       end
 
-      def rm_tag(tag_name, committer:)
-        OperationService.new(committer, self).rm_tag(find_tag(tag_name))
+      def rm_tag(tag_name, user:)
+        OperationService.new(user, self).rm_tag(find_tag(tag_name))
       end
 
       def find_tag(name)
         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|

lib/gitlab/git/committer.rb → lib/gitlab/git/user.rb

 module Gitlab
   module Git
-    class Committer
+    class User
       attr_reader :name, :email, :gl_id
 
-      def self.from_user(user)
-        new(user.name, user.email, Gitlab::GlId.gl_id(user))
+      def self.from_gitlab(gitlab_user)
+        new(gitlab_user.name, gitlab_user.email, Gitlab::GlId.gl_id(gitlab_user))
+      end
+
+      def self.from_gitaly(gitaly_user)
+        new(gitaly_user.name, gitaly_user.email, gitaly_user.gl_id)
       end
 
       def initialize(name, email, gl_id)

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/lib/gitlab/git/hooks_service_spec.rb

 require 'spec_helper'
 
 describe Gitlab::Git::HooksService, seed_helper: true do
-  let(:committer) { Gitlab::Git::Committer.new('Jane Doe', 'janedoe@example.com', 'user-456') }
+  let(:user) { Gitlab::Git::User.new('Jane Doe', 'janedoe@example.com', 'user-456') }
   let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, 'project-123') }
   let(:service) { described_class.new }
 
@@ -18,7 +18,7 @@ describe Gitlab::Git::HooksService, seed_helper: true do
         hook = double(trigger: [true, nil])
         expect(Gitlab::Git::Hook).to receive(:new).exactly(3).times.and_return(hook)
 
-        service.execute(committer, repository, @blankrev, @newrev, @ref) { }
+        service.execute(user, repository, @blankrev, @newrev, @ref) { }
       end
     end
 
@@ -28,7 +28,7 @@ describe Gitlab::Git::HooksService, seed_helper: true do
         expect(service).not_to receive(:run_hook).with('post-receive')
 
         expect do
-          service.execute(committer, repository, @blankrev, @newrev, @ref)
+          service.execute(user, repository, @blankrev, @newrev, @ref)
         end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
       end
     end
@@ -40,7 +40,7 @@ describe Gitlab::Git::HooksService, seed_helper: true do
         expect(service).not_to receive(:run_hook).with('post-receive')
 
         expect do
-          service.execute(committer, repository, @blankrev, @newrev, @ref)
+          service.execute(user, repository, @blankrev, @newrev, @ref)
         end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
       end
     end

spec/lib/gitlab/git/committer_spec.rb → spec/lib/gitlab/git/user_spec.rb

 require 'spec_helper'
 
-describe Gitlab::Git::Committer do
+describe Gitlab::Git::User do
   let(:name) { 'Jane Doe' }
   let(:email) { 'janedoe@example.com' }
   let(:gl_id) { 'user-123' }

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

@@ -8,12 +8,9 @@ describe Repository, models: true do
   let(:repository) { project.repository }
   let(:broken_repository) { create(:project, :broken_storage).repository }
   let(:user) { create(:user) }
-  let(:committer) { Gitlab::Git::Committer.from_user(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
@@ -892,7 +889,7 @@ describe Repository, models: true do
     context 'when pre hooks were successful' do
       it 'runs without errors' do
         expect_any_instance_of(Gitlab::Git::HooksService).to receive(:execute)
-          .with(committer, repository.raw_repository, old_rev, blank_sha, 'refs/heads/feature')
+          .with(git_user, repository.raw_repository, old_rev, blank_sha, 'refs/heads/feature')
 
         expect { repository.rm_branch(user, 'feature') }.not_to raise_error
       end
@@ -938,20 +935,20 @@ describe Repository, models: true do
         service = Gitlab::Git::HooksService.new
         expect(Gitlab::Git::HooksService).to receive(:new).and_return(service)
         expect(service).to receive(:execute)
-          .with(committer, target_repository.raw_repository, old_rev, new_rev, updating_ref)
+          .with(git_user, target_repository.raw_repository, old_rev, new_rev, updating_ref)
           .and_yield(service).and_return(true)
       end
 
       it 'runs without errors' do
         expect do
-          Gitlab::Git::OperationService.new(committer, repository.raw_repository).with_branch('feature') do
+          Gitlab::Git::OperationService.new(git_user, repository.raw_repository).with_branch('feature') do
             new_rev
           end
         end.not_to raise_error
       end
 
       it 'ensures the autocrlf Git option is set to :input' do
-        service = Gitlab::Git::OperationService.new(committer, repository.raw_repository)
+        service = Gitlab::Git::OperationService.new(git_user, repository.raw_repository)
 
         expect(service).to receive(:update_autocrlf_option)
 
@@ -962,7 +959,7 @@ describe Repository, models: true do
         it 'updates the head' do
           expect(repository.find_branch('feature').dereferenced_target.id).to eq(old_rev)
 
-          Gitlab::Git::OperationService.new(committer, repository.raw_repository).with_branch('feature') do
+          Gitlab::Git::OperationService.new(git_user, repository.raw_repository).with_branch('feature') do
             new_rev
           end
 
@@ -980,7 +977,7 @@ describe Repository, models: true do
           expect(target_project.repository.raw_repository).to receive(:fetch_ref)
             .and_call_original
 
-          Gitlab::Git::OperationService.new(committer, target_repository.raw_repository)
+          Gitlab::Git::OperationService.new(git_user, target_repository.raw_repository)
             .with_branch(
               'master',
               start_repository: project.repository.raw_repository,
@@ -996,7 +993,7 @@ describe Repository, models: true do
         it 'does not fetch_ref and just pass the commit' do
           expect(target_repository).not_to receive(:fetch_ref)
 
-          Gitlab::Git::OperationService.new(committer, target_repository.raw_repository)
+          Gitlab::Git::OperationService.new(git_user, target_repository.raw_repository)
             .with_branch('feature', start_repository: project.repository.raw_repository) { new_rev }
         end
       end
@@ -1015,7 +1012,7 @@ describe Repository, models: true do
         end
 
         expect do
-          Gitlab::Git::OperationService.new(committer, target_project.repository.raw_repository)
+          Gitlab::Git::OperationService.new(git_user, target_project.repository.raw_repository)
             .with_branch('feature',
                          start_repository: project.repository.raw_repository,
                          &:itself)
@@ -1037,7 +1034,7 @@ describe Repository, models: true do
         repository.add_branch(user, branch, old_rev)
 
         expect do
-          Gitlab::Git::OperationService.new(committer, repository.raw_repository).with_branch(branch) do
+          Gitlab::Git::OperationService.new(git_user, repository.raw_repository).with_branch(branch) do
             new_rev
           end
         end.not_to raise_error
@@ -1055,7 +1052,7 @@ describe Repository, models: true do
         # Updating 'master' to new_rev would lose the commits on 'master' that
         # are not contained in new_rev. This should not be allowed.
         expect do
-          Gitlab::Git::OperationService.new(committer, repository.raw_repository).with_branch(branch) do
+          Gitlab::Git::OperationService.new(git_user, repository.raw_repository).with_branch(branch) do
             new_rev
           end
         end.to raise_error(Gitlab::Git::CommitError)
@@ -1067,7 +1064,7 @@ describe Repository, models: true do
         allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
 
         expect do
-          Gitlab::Git::OperationService.new(committer, repository.raw_repository).with_branch('feature') do
+          Gitlab::Git::OperationService.new(git_user, repository.raw_repository).with_branch('feature') do
             new_rev
           end
         end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
@@ -1293,10 +1290,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
@@ -1304,19 +1298,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

@@ -164,9 +164,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