Merge branch 'zj-rev-list-removal-ee' into 'master'

Remove RevList class

Closes gitaly#1233

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

app/models/repository.rb

@@ -161,12 +161,9 @@ class Repository
 
   # Returns a list of commits that are not present in any reference
   def new_commits(newrev)
-    # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/1233
-    refs = Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-      ::Gitlab::Git::RevList.new(raw, newrev: newrev).new_refs
-    end
+    commits = raw.new_commits(newrev)
 
-    refs.map { |sha| commit(sha.strip) }
+    ::Commit.decorate(commits, project)
   end
 
   # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/384

ee/spec/lib/gitlab/checks/change_access_spec.rb

@@ -406,7 +406,7 @@ describe Gitlab::Checks::ChangeAccess do
 
       context 'for an ff merge request' do
         # the signed-commits branch fast-forwards onto master
-        let(:newrev) { '2d1096e3' }
+        let(:newrev) { "2d1096e3a0ecf1d2baf6dee036cc80775d4940ba" }
 
         it 'does not raise errors for a fast forward' do
           expect(change_access).not_to receive(:committer_check)
@@ -441,6 +441,7 @@ describe Gitlab::Checks::ChangeAccess do
         it 'does not raise errors for a merge commit' do
           expect(change_access).to receive(:committer_check).once
                                      .and_call_original
+
           expect { subject.exec }.not_to raise_error
         end
       end

lib/gitlab/git/repository.rb

@@ -381,6 +381,21 @@ module Gitlab
         end
       end
 
+      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/1233
+      def new_commits(newrev)
+        gitaly_migrate(:new_commits) do |is_enabled|
+          if is_enabled
+            gitaly_ref_client.list_new_commits(newrev)
+          else
+            refs = Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+              rev_list(including: newrev, excluding: :all).split("\n").map(&:strip)
+            end
+
+            Gitlab::Git::Commit.batch_by_oid(self, refs)
+          end
+        end
+      end
+
       def count_commits(options)
         options = process_count_commits_options(options.dup)
 

lib/gitlab/git/rev_list.rb

-module Gitlab
-  module Git
-    class RevList
-      include Gitlab::Git::Popen
-
-      attr_reader :oldrev, :newrev, :repository
-
-      def initialize(repository, newrev:, oldrev: nil)
-        @oldrev = oldrev
-        @newrev = newrev
-        @repository = repository
-      end
-
-      # This method returns an array of new commit references
-      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/1233
-      #
-      def new_refs
-        Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-          repository.rev_list(including: newrev, excluding: :all).split("\n")
-        end
-      end
-
-      private
-
-      def execute(args)
-        repository.rev_list(args).split("\n")
-      end
-
-      def get_objects(including: [], excluding: [], options: [], require_path: nil)
-        opts = { including: including, excluding: excluding, options: options, objects: true }
-
-        repository.rev_list(opts) do |lazy_output|
-          objects = objects_from_output(lazy_output, require_path: require_path)
-
-          yield(objects)
-        end
-      end
-
-      def objects_from_output(object_output, require_path: nil)
-        object_output.map do |output_line|
-          sha, path = output_line.split(' ', 2)
-
-          next if require_path && path.to_s.empty?
-
-          sha
-        end.reject(&:nil?)
-      end
-    end
-  end
-end

lib/gitlab/gitaly_client/ref_service.rb

@@ -56,6 +56,25 @@ module Gitlab
         encode!(response.name.dup)
       end
 
+      def list_new_commits(newrev)
+        request = Gitaly::ListNewCommitsRequest.new(
+          repository: @gitaly_repo,
+          commit_id: newrev
+        )
+
+        response = GitalyClient
+          .call(@storage, :ref_service, :list_new_commits, request, timeout: GitalyClient.medium_timeout)
+
+        commits = []
+        response.each do |msg|
+          msg.commits.each do |c|
+            commits << Gitlab::Git::Commit.new(@repository, c)
+          end
+        end
+
+        commits
+      end
+
       def count_tag_names
         tag_names.count
       end

spec/lib/gitlab/checks/change_access_spec.rb

@@ -191,7 +191,7 @@ describe Gitlab::Checks::ChangeAccess do
           let(:changes) { { oldrev: oldrev, ref: ref } }
 
           it 'skips integrity check' do
-            expect_any_instance_of(Gitlab::Git::RevList).not_to receive(:new_objects)
+            expect(project.repository).not_to receive(:new_objects)
 
             subject.exec
           end

spec/lib/gitlab/git/rev_list_spec.rb

-require 'spec_helper'
-
-describe Gitlab::Git::RevList do
-  let(:repository) { create(:project, :repository).repository.raw }
-  let(:rev_list) { described_class.new(repository, newrev: 'newrev') }
-
-  def args_for_popen(args_list)
-    [Gitlab.config.git.bin_path, 'rev-list', *args_list]
-  end
-
-  def stub_popen_rev_list(*additional_args, with_lazy_block: true, output:)
-    repo_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access { repository.path }
-
-    params = [
-      args_for_popen(additional_args),
-      repo_path,
-      {},
-      hash_including(lazy_block: with_lazy_block ? anything : nil)
-    ]
-
-    expect(repository).to receive(:popen).with(*params) do |*_, lazy_block:|
-      output = lazy_block.call(output.lines.lazy.map(&:chomp)) if with_lazy_block
-
-      [output, 0]
-    end
-  end
-
-  context "#new_refs" do
-    it 'calls out to `popen`' do
-      stub_popen_rev_list('newrev', '--not', '--all', with_lazy_block: false, output: "sha1\nsha2")
-
-      expect(rev_list.new_refs).to eq(%w[sha1 sha2])
-    end
-  end
-end

spec/models/repository_spec.rb

@@ -296,24 +296,40 @@ describe Repository do
   end
 
   describe '#new_commits' do
-    let(:new_refs) do
-      double(:git_rev_list, new_refs: %w[
-        c1acaa58bbcbc3eafe538cb8274ba387047b69f8
-        5937ac0a7beb003549fc5fd26fc247adbce4a52e
-      ])
+    shared_examples 'finding unreferenced commits' do
+      set(:project) { create(:project, :repository) }
+      let(:repository) { project.repository }
+
+      subject { repository.new_commits(rev) }
+
+      context 'when there are no new commits' do
+        let(:rev) { repository.commit.id }
+
+        it 'returns an empty array' do
+          expect(subject).to eq([])
+        end
       end
 
-    it 'delegates to Gitlab::Git::RevList' do
-      expect(Gitlab::Git::RevList).to receive(:new).with(
-        repository.raw,
-        newrev: 'aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj').and_return(new_refs)
+      context 'when new commits are found' do
+        let(:branch) { 'orphaned-branch' }
+        let!(:rev) { repository.commit(branch).id }
 
-      commits = repository.new_commits('aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj')
+        it 'returns the commits' do
+          repository.delete_branch(branch)
 
-      expect(commits).to eq([
-        repository.commit('c1acaa58bbcbc3eafe538cb8274ba387047b69f8'),
-        repository.commit('5937ac0a7beb003549fc5fd26fc247adbce4a52e')
-      ])
+          expect(subject).not_to be_empty
+          expect(subject).to all( be_a(::Commit) )
+          expect(subject.size).to eq(1)
+        end
+      end
+    end
+
+    context 'when Gitaly handles the request' do
+      it_behaves_like 'finding unreferenced commits'
+    end
+
+    context 'when Gitaly is disabled', :disable_gitaly do
+      it_behaves_like 'finding unreferenced commits'
     end
   end