Merge branch 'move-git-operation-service' into 'master'

Move GitOperationService to Gitlab::Git

See merge request !13984

9 files changed, 196 insertions, 192 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 5e66b1104e5..05f2f851162 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -20,7 +20,6 @@ class Repository
 
   delegate :ref_name_for_sha, to: :raw_repository
 
-  CommitError = Class.new(StandardError)
   CreateTreeError = Class.new(StandardError)
 
   # Methods that cache data from the Git repository.
@@ -171,7 +170,7 @@ class Repository
 
     return false unless newrev
 
-    GitOperationService.new(user, raw_repository).add_branch(branch_name, newrev)
+    Gitlab::Git::OperationService.new(user, raw_repository).add_branch(branch_name, newrev)
 
     after_create_branch
     find_branch(branch_name)
@@ -183,7 +182,7 @@ class Repository
 
     return false unless newrev
 
-    GitOperationService.new(user, raw_repository).add_tag(tag_name, newrev, options)
+    Gitlab::Git::OperationService.new(user, raw_repository).add_tag(tag_name, newrev, options)
 
     find_tag(tag_name)
   end
@@ -192,7 +191,7 @@ class Repository
     before_remove_branch
     branch = find_branch(branch_name)
 
-    GitOperationService.new(user, raw_repository).rm_branch(branch)
+    Gitlab::Git::OperationService.new(user, raw_repository).rm_branch(branch)
 
     after_remove_branch
     true
@@ -202,7 +201,7 @@ class Repository
     before_remove_tag
     tag = find_tag(tag_name)
 
-    GitOperationService.new(user, raw_repository).rm_tag(tag)
+    Gitlab::Git::OperationService.new(user, raw_repository).rm_tag(tag)
 
     after_remove_tag
     true
@@ -772,7 +771,7 @@ class Repository
   end
 
   def with_branch(user, *args)
-    result = GitOperationService.new(user, raw_repository).with_branch(*args) do |start_commit|
+    result = Gitlab::Git::OperationService.new(user, raw_repository).with_branch(*args) do |start_commit|
       yield start_commit
     end
 
@@ -868,7 +867,7 @@ class Repository
       merge_request.update(in_progress_merge_commit_sha: commit_id)
       commit_id
     end
-  rescue Repository::CommitError # when merge_index.conflicts?
+  rescue Gitlab::Git::CommitError # when merge_index.conflicts?
     false
   end
 
diff --git a/app/services/commits/create_service.rb b/app/services/commits/create_service.rb
index dbd0b9ef43a..f96f2931508 100644
--- a/app/services/commits/create_service.rb
+++ b/app/services/commits/create_service.rb
@@ -17,7 +17,7 @@ module Commits
       new_commit = create_commit!
 
       success(result: new_commit)
-    rescue ValidationError, ChangeError, Gitlab::Git::Index::IndexError, Repository::CommitError, Gitlab::Git::HooksService::PreReceiveError => ex
+    rescue ValidationError, ChangeError, Gitlab::Git::Index::IndexError, Gitlab::Git::CommitError, Gitlab::Git::HooksService::PreReceiveError => ex
       error(ex.message)
     end
 
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
deleted file mode 100644
index b484e9f6f2b..00000000000
--- a/app/services/git_operation_service.rb
+++ /dev/null
@@ -1,164 +0,0 @@
-class GitOperationService
-  attr_reader :committer, :repository
-
-  def initialize(committer, new_repository)
-    committer = Gitlab::Git::Committer.from_user(committer) if committer.is_a?(User)
-    @committer = committer
-
-    # Refactoring aid
-    unless new_repository.is_a?(Gitlab::Git::Repository)
-      raise "expected a Gitlab::Git::Repository, got #{new_repository}"
-    end
-
-    @repository = new_repository
-  end
-
-  def add_branch(branch_name, newrev)
-    ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
-    oldrev = Gitlab::Git::BLANK_SHA
-
-    update_ref_in_hooks(ref, newrev, oldrev)
-  end
-
-  def rm_branch(branch)
-    ref = Gitlab::Git::BRANCH_REF_PREFIX + branch.name
-    oldrev = branch.target
-    newrev = Gitlab::Git::BLANK_SHA
-
-    update_ref_in_hooks(ref, newrev, oldrev)
-  end
-
-  def add_tag(tag_name, newrev, options = {})
-    ref = Gitlab::Git::TAG_REF_PREFIX + tag_name
-    oldrev = Gitlab::Git::BLANK_SHA
-
-    with_hooks(ref, newrev, oldrev) do |service|
-      # We want to pass the OID of the tag object to the hooks. For an
-      # annotated tag we don't know that OID until after the tag object
-      # (raw_tag) is created in the repository. That is why we have to
-      # update the value after creating the tag object. Only the
-      # "post-receive" hook will receive the correct value in this case.
-      raw_tag = repository.rugged.tags.create(tag_name, newrev, options)
-      service.newrev = raw_tag.target_id
-    end
-  end
-
-  def rm_tag(tag)
-    ref = Gitlab::Git::TAG_REF_PREFIX + tag.name
-    oldrev = tag.target
-    newrev = Gitlab::Git::BLANK_SHA
-
-    update_ref_in_hooks(ref, newrev, oldrev) do
-      repository.rugged.tags.delete(tag_name)
-    end
-  end
-
-  # Whenever `start_branch_name` is passed, if `branch_name` doesn't exist,
-  # it would be created from `start_branch_name`.
-  # If `start_project` is passed, and the branch doesn't exist,
-  # it would try to find the commits from it instead of current repository.
-  def with_branch(
-    branch_name,
-    start_branch_name: nil,
-    start_repository: repository,
-    &block)
-
-    # Refactoring aid
-    unless start_repository.is_a?(Gitlab::Git::Repository)
-      raise "expected a Gitlab::Git::Repository, got #{start_repository}"
-    end
-
-    start_branch_name = nil if start_repository.empty_repo?
-
-    if start_branch_name && !start_repository.branch_exists?(start_branch_name)
-      raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.full_path}"
-    end
-
-    update_branch_with_hooks(branch_name) do
-      repository.with_repo_branch_commit(
-        start_repository,
-        start_branch_name || branch_name,
-        &block)
-    end
-  end
-
-  private
-
-  # Returns [newrev, should_run_after_create, should_run_after_create_branch]
-  def update_branch_with_hooks(branch_name)
-    update_autocrlf_option
-
-    was_empty = repository.empty?
-
-    # Make commit
-    newrev = yield
-
-    unless newrev
-      raise Repository::CommitError.new('Failed to create commit')
-    end
-
-    branch = repository.find_branch(branch_name)
-    oldrev = find_oldrev_from_branch(newrev, branch)
-
-    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)]
-  end
-
-  def find_oldrev_from_branch(newrev, branch)
-    return Gitlab::Git::BLANK_SHA unless branch
-
-    oldrev = branch.target
-
-    if oldrev == repository.rugged.merge_base(newrev, branch.target)
-      oldrev
-    else
-      raise Repository::CommitError.new('Branch diverged')
-    end
-  end
-
-  def update_ref_in_hooks(ref, newrev, oldrev)
-    with_hooks(ref, newrev, oldrev) do
-      update_ref(ref, newrev, oldrev)
-    end
-  end
-
-  def with_hooks(ref, newrev, oldrev)
-    Gitlab::Git::HooksService.new.execute(
-      committer,
-      repository,
-      oldrev,
-      newrev,
-      ref) do |service|
-
-      yield(service)
-    end
-  end
-
-  # Gitaly note: JV: wait with migrating #update_ref until we know how to migrate its call sites.
-  def update_ref(ref, newrev, oldrev)
-    # We use 'git update-ref' because libgit2/rugged currently does not
-    # offer 'compare and swap' ref updates. Without compare-and-swap we can
-    # (and have!) accidentally reset the ref to an earlier state, clobbering
-    # commits. See also https://github.com/libgit2/libgit2/issues/1534.
-    command = %W[#{Gitlab.config.git.bin_path} update-ref --stdin -z]
-    _, status = Gitlab::Popen.popen(
-      command,
-      repository.path) do |stdin|
-      stdin.write("update #{ref}\x00#{newrev}\x00#{oldrev}\x00")
-    end
-
-    unless status.zero?
-      raise Repository::CommitError.new(
-        "Could not update branch #{Gitlab::Git.branch_name(ref)}." \
-        " Please refresh and try again.")
-    end
-  end
-
-  def update_autocrlf_option
-    if repository.autocrlf != :input
-      repository.autocrlf = :input
-    end
-  end
-end
diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb
index b6449f27034..8c9acbc9fbe 100644
--- a/lib/gitlab/git.rb
+++ b/lib/gitlab/git.rb
@@ -5,6 +5,7 @@ module Gitlab
     BRANCH_REF_PREFIX = "refs/heads/".freeze
 
     CommandError = Class.new(StandardError)
+    CommitError = Class.new(StandardError)
 
     class << self
       include Gitlab::EncodingHelper
diff --git a/lib/gitlab/git/operation_service.rb b/lib/gitlab/git/operation_service.rb
new file mode 100644
index 00000000000..9e6fca8c80c
--- /dev/null
+++ b/lib/gitlab/git/operation_service.rb
@@ -0,0 +1,168 @@
+module Gitlab
+  module Git
+    class OperationService
+      attr_reader :committer, :repository
+
+      def initialize(committer, new_repository)
+        committer = Gitlab::Git::Committer.from_user(committer) if committer.is_a?(User)
+        @committer = committer
+
+        # Refactoring aid
+        unless new_repository.is_a?(Gitlab::Git::Repository)
+          raise "expected a Gitlab::Git::Repository, got #{new_repository}"
+        end
+
+        @repository = new_repository
+      end
+
+      def add_branch(branch_name, newrev)
+        ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
+        oldrev = Gitlab::Git::BLANK_SHA
+
+        update_ref_in_hooks(ref, newrev, oldrev)
+      end
+
+      def rm_branch(branch)
+        ref = Gitlab::Git::BRANCH_REF_PREFIX + branch.name
+        oldrev = branch.target
+        newrev = Gitlab::Git::BLANK_SHA
+
+        update_ref_in_hooks(ref, newrev, oldrev)
+      end
+
+      def add_tag(tag_name, newrev, options = {})
+        ref = Gitlab::Git::TAG_REF_PREFIX + tag_name
+        oldrev = Gitlab::Git::BLANK_SHA
+
+        with_hooks(ref, newrev, oldrev) do |service|
+          # We want to pass the OID of the tag object to the hooks. For an
+          # annotated tag we don't know that OID until after the tag object
+          # (raw_tag) is created in the repository. That is why we have to
+          # update the value after creating the tag object. Only the
+          # "post-receive" hook will receive the correct value in this case.
+          raw_tag = repository.rugged.tags.create(tag_name, newrev, options)
+          service.newrev = raw_tag.target_id
+        end
+      end
+
+      def rm_tag(tag)
+        ref = Gitlab::Git::TAG_REF_PREFIX + tag.name
+        oldrev = tag.target
+        newrev = Gitlab::Git::BLANK_SHA
+
+        update_ref_in_hooks(ref, newrev, oldrev) do
+          repository.rugged.tags.delete(tag_name)
+        end
+      end
+
+      # Whenever `start_branch_name` is passed, if `branch_name` doesn't exist,
+      # it would be created from `start_branch_name`.
+      # If `start_project` is passed, and the branch doesn't exist,
+      # it would try to find the commits from it instead of current repository.
+      def with_branch(
+        branch_name,
+        start_branch_name: nil,
+        start_repository: repository,
+        &block)
+
+        # Refactoring aid
+        unless start_repository.is_a?(Gitlab::Git::Repository)
+          raise "expected a Gitlab::Git::Repository, got #{start_repository}"
+        end
+
+        start_branch_name = nil if start_repository.empty_repo?
+
+        if start_branch_name && !start_repository.branch_exists?(start_branch_name)
+          raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.full_path}"
+        end
+
+        update_branch_with_hooks(branch_name) do
+          repository.with_repo_branch_commit(
+            start_repository,
+            start_branch_name || branch_name,
+            &block)
+        end
+      end
+
+      private
+
+      # Returns [newrev, should_run_after_create, should_run_after_create_branch]
+      def update_branch_with_hooks(branch_name)
+        update_autocrlf_option
+
+        was_empty = repository.empty?
+
+        # Make commit
+        newrev = yield
+
+        unless newrev
+          raise Gitlab::Git::CommitError.new('Failed to create commit')
+        end
+
+        branch = repository.find_branch(branch_name)
+        oldrev = find_oldrev_from_branch(newrev, branch)
+
+        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)]
+      end
+
+      def find_oldrev_from_branch(newrev, branch)
+        return Gitlab::Git::BLANK_SHA unless branch
+
+        oldrev = branch.target
+
+        if oldrev == repository.rugged.merge_base(newrev, branch.target)
+          oldrev
+        else
+          raise Gitlab::Git::CommitError.new('Branch diverged')
+        end
+      end
+
+      def update_ref_in_hooks(ref, newrev, oldrev)
+        with_hooks(ref, newrev, oldrev) do
+          update_ref(ref, newrev, oldrev)
+        end
+      end
+
+      def with_hooks(ref, newrev, oldrev)
+        Gitlab::Git::HooksService.new.execute(
+          committer,
+          repository,
+          oldrev,
+          newrev,
+          ref) do |service|
+
+          yield(service)
+        end
+      end
+
+      # Gitaly note: JV: wait with migrating #update_ref until we know how to migrate its call sites.
+      def update_ref(ref, newrev, oldrev)
+        # We use 'git update-ref' because libgit2/rugged currently does not
+        # offer 'compare and swap' ref updates. Without compare-and-swap we can
+        # (and have!) accidentally reset the ref to an earlier state, clobbering
+        # commits. See also https://github.com/libgit2/libgit2/issues/1534.
+        command = %W[#{Gitlab.config.git.bin_path} update-ref --stdin -z]
+        _, status = Gitlab::Popen.popen(
+          command,
+          repository.path) do |stdin|
+          stdin.write("update #{ref}\x00#{newrev}\x00#{oldrev}\x00")
+        end
+
+        unless status.zero?
+          raise Gitlab::Git::CommitError.new(
+            "Could not update branch #{Gitlab::Git.branch_name(ref)}." \
+            " Please refresh and try again.")
+        end
+      end
+
+      def update_autocrlf_option
+        if repository.autocrlf != :input
+          repository.autocrlf = :input
+        end
+      end
+    end
+  end
+end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index e38844d33b4..7065d467ec0 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -938,14 +938,14 @@ describe Repository, models: true do
 
       it 'runs without errors' do
         expect do
-          GitOperationService.new(committer, repository.raw_repository).with_branch('feature') do
+          Gitlab::Git::OperationService.new(committer, 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 = GitOperationService.new(committer, repository.raw_repository)
+        service = Gitlab::Git::OperationService.new(committer, repository.raw_repository)
 
         expect(service).to receive(:update_autocrlf_option)
 
@@ -956,7 +956,7 @@ describe Repository, models: true do
         it 'updates the head' do
           expect(repository.find_branch('feature').dereferenced_target.id).to eq(old_rev)
 
-          GitOperationService.new(committer, repository.raw_repository).with_branch('feature') do
+          Gitlab::Git::OperationService.new(committer, repository.raw_repository).with_branch('feature') do
             new_rev
           end
 
@@ -974,7 +974,7 @@ describe Repository, models: true do
           expect(target_project.repository.raw_repository).to receive(:fetch_ref)
             .and_call_original
 
-          GitOperationService.new(committer, target_repository.raw_repository)
+          Gitlab::Git::OperationService.new(committer, target_repository.raw_repository)
             .with_branch(
               'master',
               start_repository: project.repository.raw_repository,
@@ -990,7 +990,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)
 
-          GitOperationService.new(committer, target_repository.raw_repository)
+          Gitlab::Git::OperationService.new(committer, target_repository.raw_repository)
             .with_branch('feature', start_repository: project.repository.raw_repository) { new_rev }
         end
       end
@@ -1009,7 +1009,7 @@ describe Repository, models: true do
         end
 
         expect do
-          GitOperationService.new(committer, target_project.repository.raw_repository)
+          Gitlab::Git::OperationService.new(committer, target_project.repository.raw_repository)
             .with_branch('feature',
                          start_repository: project.repository.raw_repository,
                          &:itself)
@@ -1031,7 +1031,7 @@ describe Repository, models: true do
         repository.add_branch(user, branch, old_rev)
 
         expect do
-          GitOperationService.new(committer, repository.raw_repository).with_branch(branch) do
+          Gitlab::Git::OperationService.new(committer, repository.raw_repository).with_branch(branch) do
             new_rev
           end
         end.not_to raise_error
@@ -1049,10 +1049,10 @@ 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
-          GitOperationService.new(committer, repository.raw_repository).with_branch(branch) do
+          Gitlab::Git::OperationService.new(committer, repository.raw_repository).with_branch(branch) do
             new_rev
           end
-        end.to raise_error(Repository::CommitError)
+        end.to raise_error(Gitlab::Git::CommitError)
       end
     end
 
@@ -1061,7 +1061,7 @@ describe Repository, models: true do
         allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
 
         expect do
-          GitOperationService.new(committer, repository.raw_repository).with_branch('feature') do
+          Gitlab::Git::OperationService.new(committer, repository.raw_repository).with_branch('feature') do
             new_rev
           end
         end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
@@ -1160,7 +1160,7 @@ describe Repository, models: true do
       end
 
       it 'sets autocrlf to :input' do
-        GitOperationService.new(nil, repository.raw_repository).send(:update_autocrlf_option)
+        Gitlab::Git::OperationService.new(nil, repository.raw_repository).send(:update_autocrlf_option)
 
         expect(repository.raw_repository.autocrlf).to eq(:input)
       end
@@ -1175,7 +1175,7 @@ describe Repository, models: true do
         expect(repository.raw_repository).not_to receive(:autocrlf=)
           .with(:input)
 
-        GitOperationService.new(nil, repository.raw_repository).send(:update_autocrlf_option)
+        Gitlab::Git::OperationService.new(nil, repository.raw_repository).send(:update_autocrlf_option)
       end
     end
   end
@@ -1761,15 +1761,15 @@ describe Repository, models: true do
 
   describe '#update_ref' do
     it 'can create a ref' do
-      GitOperationService.new(nil, repository.raw_repository).send(:update_ref, 'refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+      Gitlab::Git::OperationService.new(nil, repository.raw_repository).send(:update_ref, 'refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
 
       expect(repository.find_branch('foobar')).not_to be_nil
     end
 
     it 'raises CommitError when the ref update fails' do
       expect do
-        GitOperationService.new(nil, repository.raw_repository).send(:update_ref, 'refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
-      end.to raise_error(Repository::CommitError)
+        Gitlab::Git::OperationService.new(nil, repository.raw_repository).send(:update_ref, 'refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+      end.to raise_error(Gitlab::Git::CommitError)
     end
   end
 
diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb
index 971eaf837cb..114019441a3 100644
--- a/spec/requests/api/files_spec.rb
+++ b/spec/requests/api/files_spec.rb
@@ -224,7 +224,7 @@ describe API::Files do
 
     it "returns a 400 if editor fails to create file" do
       allow_any_instance_of(Repository).to receive(:create_file)
-        .and_raise(Repository::CommitError, 'Cannot create file')
+        .and_raise(Gitlab::Git::CommitError, 'Cannot create file')
 
       post api(route("any%2Etxt"), user), valid_params
 
@@ -339,7 +339,7 @@ describe API::Files do
     end
 
     it "returns a 400 if fails to delete file" do
-      allow_any_instance_of(Repository).to receive(:delete_file).and_raise(Repository::CommitError, 'Cannot delete file')
+      allow_any_instance_of(Repository).to receive(:delete_file).and_raise(Gitlab::Git::CommitError, 'Cannot delete file')
 
       delete api(route(file_path), user), valid_params
 
diff --git a/spec/requests/api/v3/files_spec.rb b/spec/requests/api/v3/files_spec.rb
index 4ffa5d1784e..dc7f0eefd16 100644
--- a/spec/requests/api/v3/files_spec.rb
+++ b/spec/requests/api/v3/files_spec.rb
@@ -127,7 +127,7 @@ describe API::V3::Files do
 
     it "returns a 400 if editor fails to create file" do
       allow_any_instance_of(Repository).to receive(:create_file)
-        .and_raise(Repository::CommitError, 'Cannot create file')
+        .and_raise(Gitlab::Git::CommitError, 'Cannot create file')
 
       post v3_api("/projects/#{project.id}/repository/files", user), valid_params
 
@@ -228,7 +228,7 @@ describe API::V3::Files do
     end
 
     it "returns a 400 if fails to delete file" do
-      allow_any_instance_of(Repository).to receive(:delete_file).and_raise(Repository::CommitError, 'Cannot delete file')
+      allow_any_instance_of(Repository).to receive(:delete_file).and_raise(Gitlab::Git::CommitError, 'Cannot delete file')
 
       delete v3_api("/projects/#{project.id}/repository/files", user), valid_params
 
diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb
index b4099457478..c4979792194 100644
--- a/spec/workers/git_garbage_collect_worker_spec.rb
+++ b/spec/workers/git_garbage_collect_worker_spec.rb
@@ -143,7 +143,7 @@ describe GitGarbageCollectWorker do
       tree: old_commit.tree,
       parents: [old_commit]
     )
-    GitOperationService.new(nil, project.repository.raw_repository).send(
+    Gitlab::Git::OperationService.new(nil, project.repository.raw_repository).send(
       :update_ref,
       "refs/heads/#{SecureRandom.hex(6)}",
       new_commit_sha,