Merge branch 'pks-list-commits' into 'master'

Switch over commit RPCs to use ListCommits

See merge request gitlab-org/gitlab!65468

Gemfile

@@ -472,7 +472,7 @@ end
 gem 'spamcheck', '~> 0.1.0'
 
 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 14.1.0.pre.rc2'
+gem 'gitaly', '~> 14.1.0.pre.rc3'
 
 # KAS GRPC protocol definitions
 gem 'kas-grpc', '~> 0.0.2'

Gemfile.lock

@@ -460,7 +460,7 @@ GEM
       rails (>= 3.2.0)
     git (1.7.0)
       rchardet (~> 1.8)
-    gitaly (14.1.0.pre.rc2)
+    gitaly (14.1.0.pre.rc3)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab (4.16.1)
@@ -1482,7 +1482,7 @@ DEPENDENCIES
   gettext (~> 3.3)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 14.1.0.pre.rc2)
+  gitaly (~> 14.1.0.pre.rc3)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-dangerfiles (~> 2.2.2)

config/feature_flags/development/list_commits.yml

+---
+name: list_commits
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65468
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/335208
+milestone: '14.1'
+type: development
+group: group::gitaly
+default_enabled: false

lib/gitlab/git/repository.rb

@@ -354,9 +354,13 @@ module Gitlab
         end
       end
 
-      def new_commits(newrev)
+      def new_commits(newrevs)
         wrapped_gitaly_errors do
-          gitaly_ref_client.list_new_commits(newrev)
+          if Feature.enabled?(:list_commits)
+            gitaly_commit_client.list_commits(Array.wrap(newrevs) + %w[--not --all])
+          else
+            Array.wrap(newrevs).flat_map { |newrev| gitaly_ref_client.list_new_commits(newrev) }
+          end
         end
       end
 

lib/gitlab/gitaly_client/commit_service.rb

@@ -248,6 +248,16 @@ module Gitlab
         consume_commits_response(response)
       end
 
+      def list_commits(revisions)
+        request = Gitaly::ListCommitsRequest.new(
+          repository: @gitaly_repo,
+          revisions: Array.wrap(revisions)
+        )
+
+        response = GitalyClient.call(@repository.storage, :commit_service, :list_commits, request, timeout: GitalyClient.medium_timeout)
+        consume_commits_response(response)
+      end
+
       def list_commits_by_oid(oids)
         return [] if oids.empty?
 

spec/lib/gitlab/git/repository_spec.rb

@@ -934,6 +934,63 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
     end
   end
 
+  describe '#new_commits' do
+    let(:repository) { mutable_repository }
+    let(:new_commit) do
+      author = { name: 'Test User', email: 'mail@example.com', time: Time.now }
+
+      Rugged::Commit.create(repository_rugged,
+                            author: author,
+                            committer: author,
+                            message: "Message",
+                            parents: [],
+                            tree: "4b825dc642cb6eb9a060e54bf8d69288fbee4904")
+    end
+
+    let(:expected_commits) { 1 }
+    let(:revisions) { [new_commit] }
+
+    shared_examples 'an enumeration of new commits' do
+      it 'enumerates commits' do
+        commits = repository.new_commits(revisions).to_a
+
+        expect(commits.size).to eq(expected_commits)
+        commits.each do |commit|
+          expect(commit.id).to eq(new_commit)
+          expect(commit.message).to eq("Message")
+        end
+      end
+    end
+
+    context 'with list_commits disabled' do
+      before do
+        stub_feature_flags(list_commits: false)
+
+        expect_next_instance_of(Gitlab::GitalyClient::RefService) do |service|
+          expect(service)
+            .to receive(:list_new_commits)
+            .with(new_commit)
+            .and_call_original
+        end
+      end
+
+      it_behaves_like 'an enumeration of new commits'
+    end
+
+    context 'with list_commits enabled' do
+      before do
+        expect_next_instance_of(Gitlab::GitalyClient::CommitService) do |service|
+          expect(service)
+            .to receive(:list_commits)
+            .with([new_commit, '--not', '--all'])
+            .and_call_original
+        end
+      end
+
+      it_behaves_like 'an enumeration of new commits'
+    end
+  end
+
   describe '#count_commits_between' do
     subject { repository.count_commits_between('feature', 'master') }
 

spec/lib/gitlab/gitaly_client/commit_service_spec.rb

@@ -287,6 +287,39 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
     end
   end
 
+  describe '#list_commits' do
+    shared_examples 'a ListCommits request' do
+      before do
+        ::Gitlab::GitalyClient.clear_stubs!
+      end
+
+      it 'sends a list_commits message' do
+        expect_next_instance_of(Gitaly::CommitService::Stub) do |service|
+          expect(service)
+            .to receive(:list_commits)
+            .with(gitaly_request_with_params(expected_params), kind_of(Hash))
+            .and_return([])
+        end
+
+        client.list_commits(revisions)
+      end
+    end
+
+    context 'with a single revision' do
+      let(:revisions) { 'master' }
+      let(:expected_params) { %w[master] }
+
+      it_behaves_like 'a ListCommits request'
+    end
+
+    context 'with multiple revisions' do
+      let(:revisions) { %w[master --not --all] }
+      let(:expected_params) { %w[master --not --all] }
+
+      it_behaves_like 'a ListCommits request'
+    end
+  end
+
   describe '#commit_stats' do
     let(:request) do
       Gitaly::CommitStatsRequest.new(