Merge branch 'feature/migrate-commit-find-all-to-gitaly' into 'master'

Migrate Gitlab::Git::Commit.find_all to Gitaly

Closes gitaly#396

See merge request !12934

Gemfile

@@ -386,7 +386,7 @@ gem 'vmstat', '~> 2.3.0'
 gem 'sys-filesystem', '~> 1.1.6'
 
 # Gitaly GRPC client
-gem 'gitaly', '~> 0.17.0'
+gem 'gitaly', '~> 0.18.0'
 
 gem 'toml-rb', '~> 0.3.15', require: false
 

Gemfile.lock

@@ -269,7 +269,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly (0.17.0)
+    gitaly (0.18.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (4.7.6)
@@ -970,7 +970,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly (~> 0.17.0)
+  gitaly (~> 0.18.0)
   github-linguist (~> 4.7.0)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.5.1)

lib/gitlab/git/commit.rb

@@ -98,17 +98,13 @@ module Gitlab
         #   Commit.between(repo, '29eda46b', 'master')
         #
         def between(repo, base, head)
-          commits = Gitlab::GitalyClient.migrate(:commits_between) do |is_enabled|
+          Gitlab::GitalyClient.migrate(:commits_between) do |is_enabled|
             if is_enabled
               repo.gitaly_commit_client.between(base, head)
             else
-              repo.commits_between(base, head)
+              repo.commits_between(base, head).map { |c| decorate(c) }
             end
           end
-
-          commits.map do |commit|
-            decorate(commit)
-          end
         rescue Rugged::ReferenceError
           []
         end
@@ -135,6 +131,16 @@ module Gitlab
         #
         # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/326
         def find_all(repo, options = {})
+          Gitlab::GitalyClient.migrate(:find_all_commits) do |is_enabled|
+            if is_enabled
+              find_all_by_gitaly(repo, options)
+            else
+              find_all_by_rugged(repo, options)
+            end
+          end
+        end
+
+        def find_all_by_rugged(repo, options = {})
           actual_options = options.dup
 
           allowed_options = [:ref, :max_count, :skip, :order]
@@ -173,6 +179,10 @@ module Gitlab
           []
         end
 
+        def find_all_by_gitaly(repo, options = {})
+          Gitlab::GitalyClient::CommitService.new(repo).find_all_commits(options)
+        end
+
         def decorate(commit, ref = nil)
           Gitlab::Git::Commit.new(commit, ref)
         end
@@ -214,11 +224,12 @@ module Gitlab
       def initialize(raw_commit, head = nil)
         raise "Nil as raw commit passed" unless raw_commit
 
-        if raw_commit.is_a?(Hash)
+        case raw_commit
+        when Hash
           init_from_hash(raw_commit)
-        elsif raw_commit.is_a?(Rugged::Commit)
+        when Rugged::Commit
           init_from_rugged(raw_commit)
-        elsif raw_commit.is_a?(Gitaly::GitCommit)
+        when Gitlab::GitalyClient::Commit
           init_from_gitaly(raw_commit)
         else
           raise "Invalid raw commit type: #{raw_commit.class}"
@@ -298,7 +309,14 @@ module Gitlab
       end
 
       def parents
-        raw_commit.parents.map { |c| Gitlab::Git::Commit.new(c) }
+        case raw_commit
+        when Rugged::Commit
+          raw_commit.parents.map { |c| Gitlab::Git::Commit.new(c) }
+        when Gitlab::GitalyClient::Commit
+          parent_ids.map { |oid| self.class.find(raw_commit.repository, oid) }.compact
+        else
+          raise NotImplementedError, "commit source doesn't support #parents"
+        end
       end
 
       def stats

lib/gitlab/gitaly_client/commit.rb

+module Gitlab
+  module GitalyClient
+    class Commit
+      attr_reader :repository, :gitaly_commit
+
+      delegate :id, :subject, :body, :author, :committer, :parent_ids, to: :gitaly_commit
+
+      def initialize(repository, gitaly_commit)
+        @repository = repository
+        @gitaly_commit = gitaly_commit
+      end
+    end
+  end
+end

lib/gitlab/gitaly_client/commit_service.rb

@@ -80,6 +80,19 @@ module Gitlab
         consume_commits_response(response)
       end
 
+      def find_all_commits(opts = {})
+        request = Gitaly::FindAllCommitsRequest.new(
+          repository: @gitaly_repo,
+          revision: opts[:ref].to_s,
+          max_count: opts[:max_count].to_i,
+          skip: opts[:skip].to_i
+        )
+        request.order = opts[:order].upcase if opts[:order].present?
+
+        response = GitalyClient.call(@repository.storage, :commit_service, :find_all_commits, request)
+        consume_commits_response(response)
+      end
+
       private
 
       def commit_diff_request_params(commit, options = {})
@@ -94,7 +107,12 @@ module Gitlab
       end
 
       def consume_commits_response(response)
-        response.flat_map { |r| r.commits }
+        response.flat_map do |message|
+          message.commits.map do |gitaly_commit|
+            commit = GitalyClient::Commit.new(@repository, gitaly_commit)
+            Gitlab::Git::Commit.new(commit)
+          end
+        end
       end
     end
   end

spec/lib/gitlab/git/commit_spec.rb

@@ -91,7 +91,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
         committer: committer
       )
     end
-    let(:commit) { described_class.new(gitaly_commit) }
+    let(:commit) { described_class.new(Gitlab::GitalyClient::Commit.new(repository, gitaly_commit)) }
 
     it { expect(commit.short_id).to eq(id[0..10]) }
     it { expect(commit.id).to eq(id) }
@@ -290,69 +290,85 @@ describe Gitlab::Git::Commit, seed_helper: true do
     end
 
     describe '.find_all' do
-      it 'should return a return a collection of commits' do
-        commits = described_class.find_all(repository)
+      shared_examples 'finding all commits' do
+        it 'should return a return a collection of commits' do
+          commits = described_class.find_all(repository)
 
-        expect(commits).not_to be_empty
-        expect(commits).to all( be_a_kind_of(Gitlab::Git::Commit) )
-      end
-
-      context 'while applying a sort order based on the `order` option' do
-        it "allows ordering topologically (no parents shown before their children)" do
-          expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_TOPO)
-
-          described_class.find_all(repository, order: :topo)
+          expect(commits).to all( be_a_kind_of(Gitlab::Git::Commit) )
         end
 
-        it "allows ordering by date" do
-          expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_DATE | Rugged::SORT_TOPO)
-
-          described_class.find_all(repository, order: :date)
+        context 'max_count' do
+          subject do
+            commits = Gitlab::Git::Commit.find_all(
+              repository,
+              max_count: 50
+            )
+
+            commits.map(&:id)
+          end
+
+          it 'has 33 elements' do
+            expect(subject.size).to eq(33)
+          end
+
+          it 'includes the expected commits' do
+            expect(subject).to include(
+              SeedRepo::Commit::ID,
+              SeedRepo::Commit::PARENT_ID,
+              SeedRepo::FirstCommit::ID
+            )
+          end
         end
 
-        it "applies no sorting by default" do
-          expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_NONE)
-
-          described_class.find_all(repository)
+        context 'ref + max_count + skip' do
+          subject do
+            commits = Gitlab::Git::Commit.find_all(
+              repository,
+              ref: 'master',
+              max_count: 50,
+              skip: 1
+            )
+
+            commits.map(&:id)
+          end
+
+          it 'has 24 elements' do
+            expect(subject.size).to eq(24)
+          end
+
+          it 'includes the expected commits' do
+            expect(subject).to include(SeedRepo::Commit::ID, SeedRepo::FirstCommit::ID)
+            expect(subject).not_to include(SeedRepo::LastCommit::ID)
+          end
         end
       end
 
-      context 'max_count' do
-        subject do
-          commits = Gitlab::Git::Commit.find_all(
-            repository,
-            max_count: 50
-          )
+      context 'when Gitaly find_all_commits feature is enabled' do
+        it_behaves_like 'finding all commits'
+      end
 
-          commits.map { |c| c.id }
-        end
+      context 'when Gitaly find_all_commits feature is disabled', skip_gitaly_mock: true do
+        it_behaves_like 'finding all commits'
 
-        it 'has 31 elements' do
-          expect(subject.size).to eq(33)
-        end
-        it { is_expected.to include(SeedRepo::Commit::ID) }
-        it { is_expected.to include(SeedRepo::Commit::PARENT_ID) }
-        it { is_expected.to include(SeedRepo::FirstCommit::ID) }
-      end
+        context 'while applying a sort order based on the `order` option' do
+          it "allows ordering topologically (no parents shown before their children)" do
+            expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_TOPO)
 
-      context 'ref + max_count + skip' do
-        subject do
-          commits = Gitlab::Git::Commit.find_all(
-            repository,
-            ref: 'master',
-            max_count: 50,
-            skip: 1
-          )
+            described_class.find_all(repository, order: :topo)
+          end
 
-          commits.map { |c| c.id }
-        end
+          it "allows ordering by date" do
+            expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_DATE | Rugged::SORT_TOPO)
+
+            described_class.find_all(repository, order: :date)
+          end
+
+          it "applies no sorting by default" do
+            expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_NONE)
 
-        it 'has 23 elements' do
-          expect(subject.size).to eq(24)
+            described_class.find_all(repository)
+          end
         end
-        it { is_expected.to include(SeedRepo::Commit::ID) }
-        it { is_expected.to include(SeedRepo::FirstCommit::ID) }
-        it { is_expected.not_to include(SeedRepo::LastCommit::ID) }
       end
     end
   end