Decouple more of Repository from Rugged

app/models/repository.rb

@@ -491,7 +491,7 @@ class Repository
       raw_repository.root_ref
     else
       # When the repo does not exist we raise this error so no data is cached.
-      raise Rugged::ReferenceError
+      raise Gitlab::Git::Repository::NoRepository
     end
   end
   cache_method :root_ref
@@ -525,11 +525,7 @@ class Repository
   def commit_count_for_ref(ref)
     return 0 unless exists?
 
-    begin
     cache.fetch(:"commit_count_#{ref}") { raw_repository.commit_count(ref) }
-    rescue Rugged::ReferenceError
-      0
-    end
   end
 
   delegate :branch_names, to: :raw_repository
@@ -653,26 +649,14 @@ class Repository
   end
 
   def last_commit_for_path(sha, path)
-    raw_repository.gitaly_migrate(:last_commit_for_path) do |is_enabled|
-      if is_enabled
-        last_commit_for_path_by_gitaly(sha, path)
-      else
-        last_commit_for_path_by_rugged(sha, path)
-      end
-    end
+    commit_by(oid: last_commit_id_for_path(sha, path))
   end
 
   def last_commit_id_for_path(sha, path)
     key = path.blank? ? "last_commit_id_for_path:#{sha}" : "last_commit_id_for_path:#{sha}:#{Digest::SHA1.hexdigest(path)}"
 
     cache.fetch(key) do
-      raw_repository.gitaly_migrate(:last_commit_for_path) do |is_enabled|
-        if is_enabled
-          last_commit_for_path_by_gitaly(sha, path).id
-        else
-          last_commit_id_for_path_by_shelling_out(sha, path)
-        end
-      end
+      raw_repository.last_commit_id_for_path(sha, path)
     end
   end
 
@@ -800,16 +784,6 @@ class Repository
     with_cache_hooks { raw.multi_action(user, **options) }
   end
 
-  def can_be_merged?(source_sha, target_branch)
-    raw_repository.gitaly_migrate(:can_be_merged) do |is_enabled|
-      if is_enabled
-        gitaly_can_be_merged?(source_sha, find_branch(target_branch).target)
-      else
-        rugged_can_be_merged?(source_sha, target_branch)
-      end
-    end
-  end
-
   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|
@@ -882,20 +856,12 @@ class Repository
     first_commit_id = commit(first_commit_id).try(:id) || first_commit_id
     second_commit_id = commit(second_commit_id).try(:id) || second_commit_id
     raw_repository.merge_base(first_commit_id, second_commit_id)
-  rescue Rugged::ReferenceError
-    nil
   end
 
   def ancestor?(ancestor_id, descendant_id)
     return false if ancestor_id.nil? || descendant_id.nil?
 
-    Gitlab::GitalyClient.migrate(:is_ancestor) do |is_enabled|
-      if is_enabled
     raw_repository.ancestor?(ancestor_id, descendant_id)
-      else
-        rugged_is_ancestor?(ancestor_id, descendant_id)
-      end
-    end
   end
 
   def fetch_as_mirror(url, forced: false, refmap: :all_refs, remote_name: nil)
@@ -1077,30 +1043,7 @@ class Repository
     Gitlab::Metrics.add_event(event, { path: full_path }.merge(tags))
   end
 
-  def last_commit_for_path_by_gitaly(sha, path)
-    c = raw_repository.gitaly_commit_client.last_commit_for_path(sha, path)
-    commit_by(oid: c)
-  end
-
-  def last_commit_for_path_by_rugged(sha, path)
-    sha = last_commit_id_for_path_by_shelling_out(sha, path)
-    commit_by(oid: sha)
-  end
-
-  def last_commit_id_for_path_by_shelling_out(sha, path)
-    args = %W(rev-list --max-count=1 #{sha} -- #{path})
-    raw_repository.run_git_with_timeout(args, Gitlab::Git::Popen::FAST_GIT_PROCESS_TIMEOUT).first.strip
-  end
-
   def initialize_raw_repository
     Gitlab::Git::Repository.new(project.repository_storage, disk_path + '.git', Gitlab::GlRepository.gl_repository(project, is_wiki))
   end
-
-  def gitaly_can_be_merged?(their_commit, our_commit)
-    !raw_repository.gitaly_conflicts_client(our_commit, their_commit).conflicts?
-  end
-
-  def rugged_can_be_merged?(their_commit, our_commit)
-    !rugged.merge_commits(our_commit, their_commit).conflicts?
-  end
 end

lib/gitlab/git/diff.rb

@@ -44,7 +44,7 @@ module Gitlab
                             # branch1...branch2) From the git documentation:
                             # "git diff A...B" is equivalent to "git diff
                             # $(git-merge-base A B) B"
-                            repo.merge_base_commit(head, base)
+                            repo.merge_base(head, base)
                           end
 
           options ||= {}

lib/gitlab/git/operation_service.rb

@@ -131,7 +131,10 @@ module Gitlab
 
         oldrev = branch.target
 
-        if oldrev == repository.merge_base(newrev, branch.target)
+        merge_base = repository.merge_base(newrev, branch.target)
+        raise Gitlab::Git::Repository::InvalidRef unless merge_base
+
+        if oldrev == merge_base
           oldrev
         else
           raise Gitlab::Git::CommitError.new('Branch diverged')

lib/gitlab/git/repository.rb

@@ -551,29 +551,34 @@ module Gitlab
       end
 
       # Returns the SHA of the most recent common ancestor of +from+ and +to+
-      def merge_base_commit(from, to)
+      def merge_base(from, to)
         gitaly_migrate(:merge_base) do |is_enabled|
           if is_enabled
             gitaly_repository_client.find_merge_base(from, to)
           else
-            rugged.merge_base(from, to)
+            rugged_merge_base(from, to)
           end
         end
       end
-      alias_method :merge_base, :merge_base_commit
 
       # Gitaly note: JV: check gitlab-ee before removing this method.
       def rugged_is_ancestor?(ancestor_id, descendant_id)
         return false if ancestor_id.nil? || descendant_id.nil?
 
-        merge_base_commit(ancestor_id, descendant_id) == ancestor_id
+        rugged_merge_base(ancestor_id, descendant_id) == ancestor_id
       rescue Rugged::OdbError
         false
       end
 
       # Returns true is +from+ is direct ancestor to +to+, otherwise false
       def ancestor?(from, to)
+        Gitlab::GitalyClient.migrate(:is_ancestor) do |is_enabled|
+          if is_enabled
             gitaly_commit_client.ancestor?(from, to)
+          else
+            rugged_is_ancestor?(from, to)
+          end
+        end
       end
 
       def merged_branch_names(branch_names = [])
@@ -680,11 +685,7 @@ module Gitlab
           if is_enabled
             gitaly_commit_client.commit_count(ref)
           else
-            walker = Rugged::Walker.new(rugged)
-            walker.sorting(Rugged::SORT_TOPO | Rugged::SORT_REVERSE)
-            oid = rugged.rev_parse_oid(ref)
-            walker.push(oid)
-            walker.count
+            rugged_commit_count(ref)
           end
         end
       end
@@ -1130,13 +1131,6 @@ module Gitlab
         target_ref
       end
 
-      # Refactoring aid; allows us to copy code from app/models/repository.rb
-      def run_git_with_timeout(args, timeout, env: {})
-        circuit_breaker.perform do
-          popen_with_timeout([Gitlab.config.git.bin_path, *args], timeout, path, env)
-        end
-      end
-
       # Refactoring aid; allows us to copy code from app/models/repository.rb
       def commit(ref = 'HEAD')
         Gitlab::Git::Commit.find(self, ref)
@@ -1417,6 +1411,26 @@ module Gitlab
         output
       end
 
+      def can_be_merged?(source_sha, target_branch)
+        gitaly_migrate(:can_be_merged) do |is_enabled|
+          if is_enabled
+            gitaly_can_be_merged?(source_sha, find_branch(target_branch).target)
+          else
+            rugged_can_be_merged?(source_sha, target_branch)
+          end
+        end
+      end
+
+      def last_commit_id_for_path(sha, path)
+        gitaly_migrate(:last_commit_for_path) do |is_enabled|
+          if is_enabled
+            last_commit_for_path_by_gitaly(sha, path).id
+          else
+            last_commit_id_for_path_by_shelling_out(sha, path)
+          end
+        end
+      end
+
       private
 
       def shell_write_ref(ref_path, ref, old_ref)
@@ -1460,6 +1474,12 @@ module Gitlab
         output
       end
 
+      def run_git_with_timeout(args, timeout, env: {})
+        circuit_breaker.perform do
+          popen_with_timeout([Gitlab.config.git.bin_path, *args], timeout, path, env)
+        end
+      end
+
       def fresh_worktree?(path)
         File.exist?(path) && !clean_stuck_worktree(path)
       end
@@ -2160,7 +2180,7 @@ module Gitlab
 
           source_sha
         end
-      rescue Rugged::ReferenceError
+      rescue Rugged::ReferenceError, InvalidRef
         raise ArgumentError, 'Invalid merge source'
       end
 
@@ -2257,6 +2277,39 @@ module Gitlab
           .commits_by_message(query, revision: ref, path: path, limit: limit, offset: offset)
           .map { |c| commit(c) }
       end
+
+      def gitaly_can_be_merged?(their_commit, our_commit)
+        !gitaly_conflicts_client(our_commit, their_commit).conflicts?
+      end
+
+      def rugged_can_be_merged?(their_commit, our_commit)
+        !rugged.merge_commits(our_commit, their_commit).conflicts?
+      end
+
+      def last_commit_for_path_by_gitaly(sha, path)
+        gitaly_commit_client.last_commit_for_path(sha, path)
+      end
+
+      def last_commit_id_for_path_by_shelling_out(sha, path)
+        args = %W(rev-list --max-count=1 #{sha} -- #{path})
+        run_git_with_timeout(args, Gitlab::Git::Popen::FAST_GIT_PROCESS_TIMEOUT).first.strip
+      end
+
+      def rugged_merge_base(from, to)
+        rugged.merge_base(from, to)
+      rescue Rugged::ReferenceError
+        nil
+      end
+
+      def rugged_commit_count(ref)
+        walker = Rugged::Walker.new(rugged)
+        walker.sorting(Rugged::SORT_TOPO | Rugged::SORT_REVERSE)
+        oid = rugged.rev_parse_oid(ref)
+        walker.push(oid)
+        walker.count
+      rescue Rugged::ReferenceError
+        0
+      end
     end
   end
 end

scripts/lint-rugged

@@ -21,6 +21,7 @@ ALLOWED = [
 ].freeze
 
 rugged_lines = IO.popen(%w[git grep -i -n rugged -- app config lib], &:read).lines
+rugged_lines = rugged_lines.select { |l| /^[^:]*\.rb:/ =~ l }
 rugged_lines = rugged_lines.reject { |l| l.start_with?(*ALLOWED) }
 rugged_lines = rugged_lines.reject do |line|
   code, _comment = line.split('# ', 2)

spec/lib/gitlab/git/repository_spec.rb

@@ -2,6 +2,7 @@ require "spec_helper"
 
 describe Gitlab::Git::Repository, seed_helper: true do
   include Gitlab::EncodingHelper
+  using RSpec::Parameterized::TableSyntax
 
   shared_examples 'wrapping gRPC errors' do |gitaly_client_class, gitaly_client_method|
     it 'wraps gRPC not found error' do
@@ -442,6 +443,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
     shared_examples 'simple commit counting' do
       it { expect(repository.commit_count("master")).to eq(25) }
       it { expect(repository.commit_count("feature")).to eq(9) }
+      it { expect(repository.commit_count("does-not-exist")).to eq(0) }
     end
 
     context 'when Gitaly commit_count feature is enabled' do
@@ -1032,6 +1034,29 @@ describe Gitlab::Git::Repository, seed_helper: true do
     it { is_expected.to eq(17) }
   end
 
+  describe '#merge_base' do
+    shared_examples '#merge_base' do
+      where(:from, :to, :result) do
+        '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' | '40f4a7a617393735a95a0bb67b08385bc1e7c66d' | '570e7b2abdd848b95f2f578043fc23bd6f6fd24d'
+        '40f4a7a617393735a95a0bb67b08385bc1e7c66d' | '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' | '570e7b2abdd848b95f2f578043fc23bd6f6fd24d'
+        '40f4a7a617393735a95a0bb67b08385bc1e7c66d' | 'foobar' | nil
+        'foobar' | '40f4a7a617393735a95a0bb67b08385bc1e7c66d' | nil
+      end
+
+      with_them do
+        it { expect(repository.merge_base(from, to)).to eq(result) }
+      end
+    end
+
+    context 'with gitaly' do
+      it_behaves_like '#merge_base'
+    end
+
+    context 'without gitaly', :skip_gitaly_mock do
+      it_behaves_like '#merge_base'
+    end
+  end
+
   describe '#count_commits' do
     shared_examples 'extended commit counting' do
       context 'with after timestamp' do