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

Prepare GitOperationService for moving to Gitlab::Git

See merge request !13773

.gitlab-ci.yml

@@ -125,6 +125,7 @@ stages:
     - export KNAPSACK_GENERATE_REPORT=true
     - export CACHE_CLASSES=true
     - cp ${KNAPSACK_SPINACH_SUITE_REPORT_PATH} ${KNAPSACK_REPORT_PATH}
+    - scripts/gitaly-test-spawn
     - knapsack spinach "-r rerun" || retry '[[ -e tmp/spinach-rerun.txt ]] && bundle exec spinach -r rerun $(cat tmp/spinach-rerun.txt)'
   artifacts:
     expire_in: 31d

app/models/merge_request.rb

@@ -957,13 +957,6 @@ class MergeRequest < ActiveRecord::Base
   private
 
   def write_ref
-    target_project.repository.with_repo_branch_commit(
-      source_project.repository, source_branch) do |commit|
-        if commit
-          target_project.repository.write_ref(ref_path, commit.sha)
-        else
-          raise Rugged::ReferenceError, 'source repository is empty'
-        end
-      end
+    target_project.repository.fetch_source_branch(source_project.repository, source_branch, ref_path)
   end
 end

app/models/repository.rb

@@ -95,19 +95,6 @@ class Repository
     "#<#{self.class.name}:#{@disk_path}>"
   end
 
-  #
-  # Git repository can contains some hidden refs like:
-  #   /refs/notes/*
-  #   /refs/git-as-svn/*
-  #   /refs/pulls/*
-  # This refs by default not visible in project page and not cloned to client side.
-  #
-  # This method return true if repository contains some content visible in project page.
-  #
-  def has_visible_content?
-    branch_count > 0
-  end
-
   def commit(ref = 'HEAD')
     return nil unless exists?
 
@@ -184,7 +171,7 @@ class Repository
 
     return false unless newrev
 
-    GitOperationService.new(user, self).add_branch(branch_name, newrev)
+    GitOperationService.new(user, raw_repository).add_branch(branch_name, newrev)
 
     after_create_branch
     find_branch(branch_name)
@@ -196,7 +183,7 @@ class Repository
 
     return false unless newrev
 
-    GitOperationService.new(user, self).add_tag(tag_name, newrev, options)
+    GitOperationService.new(user, raw_repository).add_tag(tag_name, newrev, options)
 
     find_tag(tag_name)
   end
@@ -205,7 +192,7 @@ class Repository
     before_remove_branch
     branch = find_branch(branch_name)
 
-    GitOperationService.new(user, self).rm_branch(branch)
+    GitOperationService.new(user, raw_repository).rm_branch(branch)
 
     after_remove_branch
     true
@@ -215,7 +202,7 @@ class Repository
     before_remove_tag
     tag = find_tag(tag_name)
 
-    GitOperationService.new(user, self).rm_tag(tag)
+    GitOperationService.new(user, raw_repository).rm_tag(tag)
 
     after_remove_tag
     true
@@ -784,16 +771,30 @@ class Repository
     multi_action(**options)
   end
 
+  def with_branch(user, *args)
+    result = GitOperationService.new(user, raw_repository).with_branch(*args) do |start_commit|
+      yield start_commit
+    end
+
+    newrev, should_run_after_create, should_run_after_create_branch = result
+
+    after_create if should_run_after_create
+    after_create_branch if should_run_after_create_branch
+
+    newrev
+  end
+
   # rubocop:disable Metrics/ParameterLists
   def multi_action(
     user:, branch_name:, message:, actions:,
     author_email: nil, author_name: nil,
     start_branch_name: nil, start_project: project)
 
-    GitOperationService.new(user, self).with_branch(
+    with_branch(
+      user,
       branch_name,
       start_branch_name: start_branch_name,
-      start_project: start_project) do |start_commit|
+      start_repository: start_project.repository.raw_repository) do |start_commit|
 
       index = Gitlab::Git::Index.new(raw_repository)
 
@@ -846,7 +847,8 @@ class Repository
   end
 
   def merge(user, source, merge_request, options = {})
-    GitOperationService.new(user, self).with_branch(
+    with_branch(
+      user,
       merge_request.target_branch) do |start_commit|
       our_commit = start_commit.sha
       their_commit = source
@@ -873,10 +875,11 @@ class Repository
   def revert(
     user, commit, branch_name,
     start_branch_name: nil, start_project: project)
-    GitOperationService.new(user, self).with_branch(
+    with_branch(
+      user,
       branch_name,
       start_branch_name: start_branch_name,
-      start_project: start_project) do |start_commit|
+      start_repository: start_project.repository.raw_repository) do |start_commit|
 
       revert_tree_id = check_revert_content(commit, start_commit.sha)
       unless revert_tree_id
@@ -896,10 +899,11 @@ class Repository
   def cherry_pick(
     user, commit, branch_name,
     start_branch_name: nil, start_project: project)
-    GitOperationService.new(user, self).with_branch(
+    with_branch(
+      user,
       branch_name,
       start_branch_name: start_branch_name,
-      start_project: start_project) do |start_commit|
+      start_repository: start_project.repository.raw_repository) do |start_commit|
 
       cherry_pick_tree_id = check_cherry_pick_content(commit, start_commit.sha)
       unless cherry_pick_tree_id
@@ -921,7 +925,7 @@ class Repository
   end
 
   def resolve_conflicts(user, branch_name, params)
-    GitOperationService.new(user, self).with_branch(branch_name) do
+    with_branch(user, branch_name) do
       committer = user_to_committer(user)
 
       create_commit(params.merge(author: committer, committer: committer))
@@ -1011,25 +1015,6 @@ class Repository
     run_git(args).first.lines.map(&:strip)
   end
 
-  def with_repo_branch_commit(start_repository, start_branch_name)
-    return yield nil if start_repository.empty_repo?
-
-    if start_repository == self
-      yield commit(start_branch_name)
-    else
-      sha = start_repository.commit(start_branch_name).sha
-
-      if branch_commit = commit(sha)
-        yield branch_commit
-      else
-        with_repo_tmp_commit(
-          start_repository, start_branch_name, sha) do |tmp_commit|
-          yield tmp_commit
-        end
-      end
-    end
-  end
-
   def add_remote(name, url)
     raw_repository.remote_add(name, url)
   rescue Rugged::ConfigError
@@ -1047,14 +1032,12 @@ class Repository
     gitlab_shell.fetch_remote(raw_repository, remote, forced: forced, no_tags: no_tags)
   end
 
-  def fetch_ref(source_path, source_ref, target_ref)
-    args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref})
-    message, status = run_git(args)
-
-    # Make sure ref was created, and raise Rugged::ReferenceError when not
-    raise Rugged::ReferenceError, message if status != 0
+  def fetch_source_branch(source_repository, source_branch, local_ref)
+    raw_repository.fetch_source_branch(source_repository.raw_repository, source_branch, local_ref)
+  end
 
-    target_ref
+  def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:)
+    raw_repository.compare_source_branch(target_branch_name, source_repository.raw_repository, source_branch_name, straight: straight)
   end
 
   def create_ref(ref, ref_path)
@@ -1135,12 +1118,6 @@ class Repository
 
   private
 
-  def run_git(args)
-    circuit_breaker.perform do
-      Gitlab::Popen.popen([Gitlab.config.git.bin_path, *args], path_to_repo)
-    end
-  end
-
   def blob_data_at(sha, path)
     blob = blob_at(sha, path)
     return unless blob
@@ -1236,16 +1213,4 @@ class Repository
       .commits_by_message(query, revision: ref, path: path, limit: limit, offset: offset)
       .map { |c| commit(c) }
   end
-
-  def with_repo_tmp_commit(start_repository, start_branch_name, sha)
-    tmp_ref = fetch_ref(
-      start_repository.path_to_repo,
-      "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}",
-      "refs/tmp/#{SecureRandom.hex}/head"
-    )
-
-    yield commit(sha)
-  ensure
-    delete_refs(tmp_ref) if tmp_ref
-  end
 end

app/services/compare_service.rb

@@ -11,26 +11,8 @@ class CompareService
   end
 
   def execute(target_project, target_branch, straight: false)
-    # If compare with other project we need to fetch ref first
-    target_project.repository.with_repo_branch_commit(
-      start_project.repository,
-      start_branch_name) do |commit|
-      break unless commit
+    raw_compare = target_project.repository.compare_source_branch(target_branch, start_project.repository, start_branch_name, straight: straight)
 
-      compare(commit.sha, target_project, target_branch, straight: straight)
-    end
-  end
-
-  private
-
-  def compare(source_sha, target_project, target_branch, straight:)
-    raw_compare = Gitlab::Git::Compare.new(
-      target_project.repository.raw_repository,
-      target_branch,
-      source_sha,
-      straight: straight
-    )
-
-    Compare.new(raw_compare, target_project, straight: straight)
+    Compare.new(raw_compare, target_project, straight: straight) if raw_compare
   end
 end

app/services/git_operation_service.rb

@@ -5,6 +5,11 @@ class GitOperationService
     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
 
@@ -55,10 +60,14 @@ class GitOperationService
   def with_branch(
     branch_name,
     start_branch_name: nil,
-    start_project: repository.project,
+    start_repository: repository,
     &block)
 
-    start_repository = start_project.repository
+    # 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)
@@ -75,6 +84,7 @@ class GitOperationService
 
   private
 
+  # Returns [newrev, should_run_after_create, should_run_after_create_branch]
   def update_branch_with_hooks(branch_name)
     update_autocrlf_option
 
@@ -93,12 +103,7 @@ class GitOperationService
     ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
     update_ref_in_hooks(ref, newrev, oldrev)
 
-    # If repo was empty expire cache
-    repository.after_create if was_empty
-    repository.after_create_branch if
-      was_empty || Gitlab::Git.blank_ref?(oldrev)
-
-    newrev
+    [newrev, was_empty, was_empty || Gitlab::Git.blank_ref?(oldrev)]
   end
 
   def find_oldrev_from_branch(newrev, branch)
@@ -140,7 +145,7 @@ class GitOperationService
     command = %W[#{Gitlab.config.git.bin_path} update-ref --stdin -z]
     _, status = Gitlab::Popen.popen(
       command,
-      repository.path_to_repo) do |stdin|
+      repository.path) do |stdin|
       stdin.write("update #{ref}\x00#{newrev}\x00#{oldrev}\x00")
     end
 
@@ -152,8 +157,8 @@ class GitOperationService
   end
 
   def update_autocrlf_option
-    if repository.raw_repository.autocrlf != :input
-      repository.raw_repository.autocrlf = :input
+    if repository.autocrlf != :input
+      repository.autocrlf = :input
     end
   end
 end

lib/gitlab/conflict/file_collection.rb

@@ -18,7 +18,7 @@ module Gitlab
           new(merge_request, project).tap do |file_collection|
             project
               .repository
-              .with_repo_branch_commit(merge_request.target_project.repository, merge_request.target_branch) do
+              .with_repo_branch_commit(merge_request.target_project.repository.raw_repository, merge_request.target_branch) do
 
               yield file_collection
             end

lib/gitlab/git/repository.rb

@@ -73,6 +73,10 @@ module Gitlab
 
       delegate :exists?, to: :gitaly_repository_client
 
+      def ==(other)
+        path == other.path
+      end
+
       # Default branch in the repository
       def root_ref
         @root_ref ||= gitaly_migrate(:root_ref) do |is_enabled|
@@ -740,6 +744,106 @@ module Gitlab
         end
       end
 
+      def with_repo_branch_commit(start_repository, start_branch_name)
+        raise "expected Gitlab::Git::Repository, got #{start_repository}" unless start_repository.is_a?(Gitlab::Git::Repository)
+
+        return yield nil if start_repository.empty_repo?
+
+        if start_repository == self
+          yield commit(start_branch_name)
+        else
+          sha = start_repository.commit(start_branch_name).sha
+
+          if branch_commit = commit(sha)
+            yield branch_commit
+          else
+            with_repo_tmp_commit(
+              start_repository, start_branch_name, sha) do |tmp_commit|
+              yield tmp_commit
+            end
+          end
+        end
+      end
+
+      def with_repo_tmp_commit(start_repository, start_branch_name, sha)
+        tmp_ref = fetch_ref(
+          start_repository.path,
+          "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}",
+          "refs/tmp/#{SecureRandom.hex}/head"
+        )
+
+        yield commit(sha)
+      ensure
+        delete_refs(tmp_ref) if tmp_ref
+      end
+
+      def fetch_source_branch(source_repository, source_branch, local_ref)
+        with_repo_branch_commit(source_repository, source_branch) do |commit|
+          if commit
+            write_ref(local_ref, commit.sha)
+          else
+            raise Rugged::ReferenceError, 'source repository is empty'
+          end
+        end
+      end
+
+      def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:)
+        with_repo_branch_commit(source_repository, source_branch_name) do |commit|
+          break unless commit
+
+          Gitlab::Git::Compare.new(
+            self,
+            target_branch_name,
+            commit.sha,
+            straight: straight
+          )
+        end
+      end
+
+      def write_ref(ref_path, sha)
+        rugged.references.create(ref_path, sha, force: true)
+      end
+
+      def fetch_ref(source_path, source_ref, target_ref)
+        args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref})
+        message, status = run_git(args)
+
+        # Make sure ref was created, and raise Rugged::ReferenceError when not
+        raise Rugged::ReferenceError, message if status != 0
+
+        target_ref
+      end
+
+      # Refactoring aid; allows us to copy code from app/models/repository.rb
+      def run_git(args)
+        circuit_breaker.perform do
+          popen([Gitlab.config.git.bin_path, *args], path)
+        end
+      end
+
+      # Refactoring aid; allows us to copy code from app/models/repository.rb
+      def commit(ref = 'HEAD')
+        Gitlab::Git::Commit.find(self, ref)
+      end
+
+      # Refactoring aid; allows us to copy code from app/models/repository.rb
+      def empty_repo?
+        !exists? || !has_visible_content?
+      end
+
+      #
+      # Git repository can contains some hidden refs like:
+      #   /refs/notes/*
+      #   /refs/git-as-svn/*
+      #   /refs/pulls/*
+      # This refs by default not visible in project page and not cloned to client side.
+      #
+      # This method return true if repository contains some content visible in project page.
+      #
+      def has_visible_content?
+        branch_count > 0
+      end
+
       def gitaly_repository
         Gitlab::GitalyClient::Util.repository(@storage, @relative_path)
       end

spec/models/repository_spec.rb

@@ -886,7 +886,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, old_rev, blank_sha, 'refs/heads/feature')
+          .with(committer, repository.raw_repository, old_rev, blank_sha, 'refs/heads/feature')
 
         expect { repository.rm_branch(user, 'feature') }.not_to raise_error
       end
@@ -932,20 +932,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, old_rev, new_rev, updating_ref)
+          .with(committer, target_repository.raw_repository, old_rev, new_rev, updating_ref)
           .and_yield(service).and_return(true)
       end
 
       it 'runs without errors' do
         expect do
-          GitOperationService.new(committer, repository).with_branch('feature') do
+          GitOperationService.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)
+        service = GitOperationService.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).with_branch('feature') do
+          GitOperationService.new(committer, repository.raw_repository).with_branch('feature') do
             new_rev
           end
 
@@ -971,13 +971,13 @@ describe Repository, models: true do
         let(:updating_ref) { 'refs/heads/master' }
 
         it 'fetch_ref and create the branch' do
-          expect(target_project.repository).to receive(:fetch_ref)
+          expect(target_project.repository.raw_repository).to receive(:fetch_ref)
             .and_call_original
 
-          GitOperationService.new(committer, target_repository)
+          GitOperationService.new(committer, target_repository.raw_repository)
             .with_branch(
               'master',
-              start_project: project,
+              start_repository: project.repository.raw_repository,
               start_branch_name: 'feature') { new_rev }
 
           expect(target_repository.branch_names).to contain_exactly('master')
@@ -990,8 +990,8 @@ 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)
-            .with_branch('feature', start_project: project) { new_rev }
+          GitOperationService.new(committer, target_repository.raw_repository)
+            .with_branch('feature', start_repository: project.repository.raw_repository) { new_rev }
         end
       end
     end
@@ -1000,7 +1000,7 @@ describe Repository, models: true do
       let(:target_project) { create(:project, :empty_repo) }
 
       before do
-        expect(target_project.repository).to receive(:run_git)
+        expect(target_project.repository.raw_repository).to receive(:run_git)
       end
 
       it 'raises Rugged::ReferenceError' do
@@ -1009,9 +1009,9 @@ describe Repository, models: true do
         end
 
         expect do
-          GitOperationService.new(committer, target_project.repository)
+          GitOperationService.new(committer, target_project.repository.raw_repository)
             .with_branch('feature',
-                         start_project: project,
+                         start_repository: project.repository.raw_repository,
                          &:itself)
         end.to raise_reference_error
       end
@@ -1031,7 +1031,7 @@ describe Repository, models: true do
         repository.add_branch(user, branch, old_rev)
 
         expect do
-          GitOperationService.new(committer, repository).with_branch(branch) do
+          GitOperationService.new(committer, repository.raw_repository).with_branch(branch) do
             new_rev
           end
         end.not_to raise_error
@@ -1049,7 +1049,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
-          GitOperationService.new(committer, repository).with_branch(branch) do
+          GitOperationService.new(committer, repository.raw_repository).with_branch(branch) do
             new_rev
           end
         end.to raise_error(Repository::CommitError)
@@ -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).with_branch('feature') do
+          GitOperationService.new(committer, repository.raw_repository).with_branch('feature') do
             new_rev
           end
         end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
@@ -1079,10 +1079,9 @@ describe Repository, models: true do
         expect(repository).not_to receive(:expire_emptiness_caches)
         expect(repository).to     receive(:expire_branches_cache)
 
-        GitOperationService.new(committer, repository)
-          .with_branch('new-feature') do
-            new_rev
-          end
+        repository.with_branch(user, 'new-feature') do
+          new_rev
+        end
       end
     end
 
@@ -1139,7 +1138,7 @@ describe Repository, models: true do
 
     describe 'when there are no branches' do
       before do
-        allow(repository).to receive(:branch_count).and_return(0)
+        allow(repository.raw_repository).to receive(:branch_count).and_return(0)
       end
 
       it { is_expected.to eq(false) }
@@ -1147,7 +1146,7 @@ describe Repository, models: true do
 
     describe 'when there are branches' do
       it 'returns true' do
-        expect(repository).to receive(:branch_count).and_return(3)
+        expect(repository.raw_repository).to receive(:branch_count).and_return(3)
 
         expect(subject).to eq(true)
       end
@@ -1161,7 +1160,7 @@ describe Repository, models: true do
       end
 
       it 'sets autocrlf to :input' do
-        GitOperationService.new(nil, repository).send(:update_autocrlf_option)
+        GitOperationService.new(nil, repository.raw_repository).send(:update_autocrlf_option)
 
         expect(repository.raw_repository.autocrlf).to eq(:input)
       end
@@ -1176,7 +1175,7 @@ describe Repository, models: true do
         expect(repository.raw_repository).not_to receive(:autocrlf=)
           .with(:input)
 
-        GitOperationService.new(nil, repository).send(:update_autocrlf_option)
+        GitOperationService.new(nil, repository.raw_repository).send(:update_autocrlf_option)
       end
     end
   end
@@ -1762,14 +1761,14 @@ describe Repository, models: true do
 
   describe '#update_ref' do
     it 'can create a ref' do
-      GitOperationService.new(nil, repository).send(:update_ref, 'refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+      GitOperationService.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).send(:update_ref, 'refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+        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)
     end
   end

spec/workers/git_garbage_collect_worker_spec.rb

@@ -23,8 +23,8 @@ describe GitGarbageCollectWorker do
 
         expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original
         expect_any_instance_of(Repository).to receive(:branch_names).and_call_original
-        expect_any_instance_of(Repository).to receive(:branch_count).and_call_original
-        expect_any_instance_of(Repository).to receive(:has_visible_content?).and_call_original
+        expect_any_instance_of(Gitlab::Git::Repository).to receive(:branch_count).and_call_original
+        expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original
 
         subject.perform(project.id)
       end
@@ -143,7 +143,7 @@ describe GitGarbageCollectWorker do
       tree: old_commit.tree,
       parents: [old_commit]
     )
-    GitOperationService.new(nil, project.repository).send(
+    GitOperationService.new(nil, project.repository.raw_repository).send(
       :update_ref,
       "refs/heads/#{SecureRandom.hex(6)}",
       new_commit_sha,