Merge branch '195667-replace-commits-between-with-list-commits' into 'master'

Use the ListCommits RPC for Gitlab::Git::Commit.between

See merge request gitlab-org/gitlab!67591

config/feature_flags/development/between_uses_list_commits.yml

+---
+name: between_uses_list_commits
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/67591
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/337960
+milestone: '14.2'
+type: development
+group: group::source code
+default_enabled: false

ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe 'Two merge requests on a merge train' do
   include RepoHelpers
 
   let(:project) { create(:project, :repository) }
+  let(:key) { create(:key, user: project.owner) }
   let_it_be(:maintainer_1) { create(:user) }
   let_it_be(:maintainer_2) { create(:user) }
 
@@ -270,10 +271,15 @@ RSpec.describe 'Two merge requests on a merge train' do
     end
 
     def push_commit_to_master
-      create_file_in_repo(project, 'master', 'master', 'test.txt', 'This is test')
-      changes = Base64.encode64("123456 789012 refs/heads/master")
-      key_id = create(:key, user: project.owner).shell_id
-      PostReceive.new.perform("project-#{project.id}", key_id, changes)
+      branch = project.default_branch_or_main
+      oldrev = project.repository.commit(branch).sha
+
+      create_file_in_repo(project, branch, branch, 'test.txt', 'This is a test')
+
+      newrev = project.repository.commit(branch).sha
+      changes = Base64.encode64("#{oldrev} #{newrev} refs/heads/#{branch}")
+
+      PostReceive.new.perform("project-#{project.id}", key.shell_id, changes)
     end
   end
 end

lib/gitlab/git/commit.rb

@@ -111,8 +111,18 @@ module Gitlab
         #   Commit.between(repo, '29eda46b', 'master')
         #
         def between(repo, base, head)
+          # In either of these cases, we are guaranteed to return no commits, so
+          # shortcut the RPC call
+          return [] if Gitlab::Git.blank_ref?(base) || Gitlab::Git.blank_ref?(head)
+
           wrapped_gitaly_errors do
-            repo.gitaly_commit_client.between(base, head)
+            if Feature.enabled?(:between_uses_list_commits, default_enabled: :yaml)
+              revisions = [head, "^#{base}"] # base..head
+
+              repo.gitaly_commit_client.list_commits(revisions, reverse: true)
+            else
+              repo.gitaly_commit_client.between(base, head)
+            end
           end
         end
 

lib/gitlab/gitaly_client/commit_service.rb

@@ -255,10 +255,11 @@ module Gitlab
         consume_commits_response(response)
       end
 
-      def list_commits(revisions)
+      def list_commits(revisions, reverse: false)
         request = Gitaly::ListCommitsRequest.new(
           repository: @gitaly_repo,
-          revisions: Array.wrap(revisions)
+          revisions: Array.wrap(revisions),
+          reverse: reverse
         )
 
         response = GitalyClient.call(@repository.storage, :commit_service, :list_commits, request, timeout: GitalyClient.medium_timeout)

spec/lib/gitlab/git/commit_spec.rb

@@ -369,11 +369,15 @@ RSpec.describe Gitlab::Git::Commit, :seed_helper do
         commits.map { |c| c.id }
       end
 
-      it 'has 1 element' do
-        expect(subject.size).to eq(1)
+      it { is_expected.to contain_exactly(SeedRepo::Commit::ID) }
+
+      context 'between_uses_list_commits FF disabled' do
+        before do
+          stub_feature_flags(between_uses_list_commits: false)
+        end
+
+        it { is_expected.to contain_exactly(SeedRepo::Commit::ID) }
       end
-      it { is_expected.to include(SeedRepo::Commit::ID) }
-      it { is_expected.not_to include(SeedRepo::FirstCommit::ID) }
     end
 
     describe '.shas_with_signatures' do

spec/services/git/branch_push_service_spec.rb

@@ -151,9 +151,9 @@ RSpec.describe Git::BranchPushService, services: true do
     it "when pushing a new branch for the first time" do
       expect(UpdateMergeRequestsWorker)
         .to receive(:perform_async)
-        .with(project.id, user.id, blankrev, 'newrev', ref)
+        .with(project.id, user.id, blankrev, newrev, ref)
 
-      execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref)
+      execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref)
     end
   end
 
@@ -162,13 +162,13 @@ RSpec.describe Git::BranchPushService, services: true do
       it "calls the copy attributes method for the first push to the default branch" do
         expect(project.repository).to receive(:copy_gitattributes).with('master')
 
-        execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref)
+        execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref)
       end
 
       it "calls the copy attributes method for changes to the default branch" do
         expect(project.repository).to receive(:copy_gitattributes).with(ref)
 
-        execute_service(project, user, oldrev: 'oldrev', newrev: 'newrev', ref: ref)
+        execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
       end
     end
 
@@ -195,7 +195,7 @@ RSpec.describe Git::BranchPushService, services: true do
       it "when pushing a branch for the first time" do
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref)
+        execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref)
         expect(project.protected_branches).not_to be_empty
         expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER])
         expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER])
@@ -206,7 +206,7 @@ RSpec.describe Git::BranchPushService, services: true do
 
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref)
+        execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref)
         expect(project.protected_branches).to be_empty
       end
 
@@ -216,7 +216,7 @@ RSpec.describe Git::BranchPushService, services: true do
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
 
-        execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref)
+        execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref)
 
         expect(project.protected_branches).not_to be_empty
         expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER])
@@ -231,7 +231,7 @@ RSpec.describe Git::BranchPushService, services: true do
         expect(project.default_branch).to eq("master")
         expect(ProtectedBranches::CreateService).not_to receive(:new)
 
-        execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref)
+        execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref)
 
         expect(project.protected_branches).not_to be_empty
         expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::NO_ACCESS])
@@ -243,7 +243,7 @@ RSpec.describe Git::BranchPushService, services: true do
 
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref)
+        execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref)
         expect(project.protected_branches).not_to be_empty
         expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER])
         expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER])
@@ -251,7 +251,7 @@ RSpec.describe Git::BranchPushService, services: true do
 
       it "when pushing new commits to existing branch" do
         expect(project).to receive(:execute_hooks)
-        execute_service(project, user, oldrev: 'oldrev', newrev: 'newrev', ref: ref)
+        execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
       end
     end
   end

spec/services/git/process_ref_changes_service_spec.rb

@@ -116,6 +116,8 @@ RSpec.describe Git::ProcessRefChangesService do
           if changes_method == :tag_changes
             allow_any_instance_of(Repository).to receive(:tag_exists?) { true }
           end
+
+          allow(Gitlab::Git::Commit).to receive(:between) { [] }
         end
 
         context 'when git_push_create_all_pipelines is disabled' do
@@ -150,6 +152,8 @@ RSpec.describe Git::ProcessRefChangesService do
       context 'with invalid .gitlab-ci.yml' do
         before do
           stub_ci_pipeline_yaml_file(nil)
+
+          allow(Gitlab::Git::Commit).to receive(:between) { [] }
         end
 
         it 'does not create a pipeline' do
@@ -190,6 +194,8 @@ RSpec.describe Git::ProcessRefChangesService do
         allow(MergeRequests::PushedBranchesService).to receive(:new).and_return(
           double(execute: %w(create1 create2)), double(execute: %w(changed1)), double(execute: %w(removed2))
         )
+
+        allow(Gitlab::Git::Commit).to receive(:between).and_return([])
       end
 
       it 'schedules job for existing merge requests' do

spec/workers/post_receive_spec.rb

@@ -5,7 +5,13 @@ require 'spec_helper'
 RSpec.describe PostReceive do
   include AfterNextHelpers
 
-  let(:changes) { "123456 789012 refs/heads/tést\n654321 210987 refs/tags/tag" }
+  let(:changes) do
+    <<~EOF
+      #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/heads/tést
+      #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/tags/tag
+    EOF
+  end
+
   let(:wrongly_encoded_changes) { changes.encode("ISO-8859-1").force_encoding("UTF-8") }
   let(:base64_changes) { Base64.encode64(wrongly_encoded_changes) }
   let(:gl_repository) { "project-#{project.id}" }
@@ -64,7 +70,6 @@ RSpec.describe PostReceive do
   describe '#process_project_changes' do
     context 'with an empty project' do
       let(:empty_project) { create(:project, :empty_repo) }
-      let(:changes) { "123456 789012 refs/heads/tést1\n" }
 
       before do
         allow_next(Gitlab::GitPostReceive).to receive(:identify).and_return(empty_project.owner)
@@ -146,8 +151,8 @@ RSpec.describe PostReceive do
       context 'branches' do
         let(:changes) do
           <<~EOF
-            123456 789012 refs/heads/tést1
-            123456 789012 refs/heads/tést2
+            #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/heads/tést1
+            #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/heads/tést2
           EOF
         end
 
@@ -182,9 +187,9 @@ RSpec.describe PostReceive do
         context 'with a default branch' do
           let(:changes) do
             <<~EOF
-              123456 789012 refs/heads/tést1
-              123456 789012 refs/heads/tést2
-              678912 123455 refs/heads/#{project.default_branch}
+              #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/heads/tést1
+              #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/heads/tést2
+              #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/heads/#{project.default_branch}
             EOF
           end
 
@@ -200,9 +205,9 @@ RSpec.describe PostReceive do
       context 'tags' do
         let(:changes) do
           <<~EOF
-            654321 210987 refs/tags/tag1
-            654322 210986 refs/tags/tag2
-            654323 210985 refs/tags/tag3
+            #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/tags/tag1
+            #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/tags/tag2
+            #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/tags/tag3
           EOF
         end
 
@@ -241,7 +246,7 @@ RSpec.describe PostReceive do
       end
 
       context 'merge-requests' do
-        let(:changes) { "123456 789012 refs/merge-requests/123" }
+        let(:changes) { "#{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/merge-requests/123" }
 
         it "does not call any of the services" do
           expect(Git::ProcessRefChangesService).not_to receive(:new)
@@ -253,7 +258,6 @@ RSpec.describe PostReceive do
       end
 
       context 'after project changes hooks' do
-        let(:changes) { '123456 789012 refs/heads/tést' }
         let(:fake_hook_data) { { event_name: 'repository_update' } }
 
         before do
@@ -305,12 +309,12 @@ RSpec.describe PostReceive do
 
     context 'master' do
       let(:default_branch) { 'master' }
-      let(:oldrev) { '012345' }
-      let(:newrev) { '6789ab' }
+      let(:oldrev) { SeedRepo::Commit::PARENT_ID }
+      let(:newrev) { SeedRepo::Commit::ID }
       let(:changes) do
         <<~EOF
             #{oldrev} #{newrev} refs/heads/#{default_branch}
-            123456 789012 refs/heads/tést2
+            #{oldrev} #{newrev} refs/heads/tést2
         EOF
       end
 
@@ -326,8 +330,8 @@ RSpec.describe PostReceive do
     context 'branches' do
       let(:changes) do
         <<~EOF
-            123456 789012 refs/heads/tést1
-            123456 789012 refs/heads/tést2
+            #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/heads/tést1
+            #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/heads/tést2
         EOF
       end
 
@@ -406,8 +410,8 @@ RSpec.describe PostReceive do
         context 'branches' do
           let(:changes) do
             <<~EOF
-                123456 789012 refs/heads/tést1
-                123456 789012 refs/heads/tést2
+                #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/heads/tést1
+                #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/heads/tést2
             EOF
           end
 
@@ -434,9 +438,9 @@ RSpec.describe PostReceive do
         context 'tags' do
           let(:changes) do
             <<~EOF
-              654321 210987 refs/tags/tag1
-              654322 210986 refs/tags/tag2
-              654323 210985 refs/tags/tag3
+              #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/tags/tag1
+              #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/tags/tag2
+              #{SeedRepo::Commit::PARENT_ID} #{SeedRepo::Commit::ID} refs/tags/tag3
             EOF
           end