Fix specs that use invalid arguments to Gitlab::Git::Commit.between

Unlike the CommitsBetween RPC, the ListCommits RPC will issue an error
if an unknown or invalid revision is given as an argument. This should
generally be fine in production, where (e.g.) attempting to view
changes on a branch that has just been deleted can justifiably error
out.

In the test suite, we use stub revisions a lot, and where these get to
Gitlab::Git::Commit.between, they cause an error in previously-passing
specs. We don't rely on the result of `between` in any of these specs,
so we just need to fix the tests not to trigger the behaviour.

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

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