Implement final review comments from @rymai.25301-git-2.11-force-push-bug

- `raise "string"` raises a `RuntimeError` - no need to be explicit
- Remove top-level comment in the `RevList` class
- Use `%w()` instead of `%w[]`
- Extract an `environment_variables` method to cache `env.slice(*ALLOWED_VARIABLES)`
- Use `start_with?` for env variable validation instead of regex match
- Validation specs for each allowed environment variable were identical. Build them dynamically.
- Minor change to `popen3` expectation.

3 files changed, 30 insertions, 51 deletions

diff --git a/lib/gitlab/checks/force_push.rb b/lib/gitlab/checks/force_push.rb
index e1c967a1f89..de0c9049ebf 100644
--- a/lib/gitlab/checks/force_push.rb
+++ b/lib/gitlab/checks/force_push.rb
@@ -13,7 +13,7 @@ module Gitlab
           if exit_status == 0
             missed_ref.present?
           else
-            raise RuntimeError, "Got a non-zero exit code while calling out to `git rev-list` in the force-push check."
+            raise "Got a non-zero exit code while calling out to `git rev-list` in the force-push check."
           end
         end
       end
diff --git a/lib/gitlab/git/rev_list.rb b/lib/gitlab/git/rev_list.rb
index ecd038e04df..25e9d619697 100644
--- a/lib/gitlab/git/rev_list.rb
+++ b/lib/gitlab/git/rev_list.rb
@@ -1,11 +1,9 @@
-# Call out to the `git rev-list` command
-
 module Gitlab
   module Git
     class RevList
       attr_reader :project, :env
 
-      ALLOWED_VARIABLES = %w(GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES).freeze
+      ALLOWED_VARIABLES = %w[GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES].freeze
 
       def initialize(oldrev, newrev, project:, env: nil)
         @project = project
@@ -23,8 +21,8 @@ module Gitlab
       end
 
       def valid?
-        env.slice(*ALLOWED_VARIABLES).all? do |(name, value)|
-          value =~ /^#{project.repository.path_to_repo}/
+        environment_variables.all? do |(name, value)|
+          value.start_with?(project.repository.path_to_repo)
         end
       end
 
@@ -33,7 +31,11 @@ module Gitlab
       def parse_environment_variables
         return {} unless valid?
 
-        env.slice(*ALLOWED_VARIABLES)
+        environment_variables
+      end
+
+      def environment_variables
+        @environment_variables ||= env.slice(*ALLOWED_VARIABLES)
       end
     end
   end
diff --git a/spec/lib/gitlab/git/rev_list_spec.rb b/spec/lib/gitlab/git/rev_list_spec.rb
index f76aca29107..444639acbaa 100644
--- a/spec/lib/gitlab/git/rev_list_spec.rb
+++ b/spec/lib/gitlab/git/rev_list_spec.rb
@@ -4,49 +4,28 @@ describe Gitlab::Git::RevList, lib: true do
   let(:project) { create(:project) }
 
   context "validations" do
-    context "GIT_OBJECT_DIRECTORY" do
-      it "accepts values starting with the project repo path" do
-        env = { "GIT_OBJECT_DIRECTORY" => "#{project.repository.path_to_repo}/objects" }
-        rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
+    described_class::ALLOWED_VARIABLES.each do |var|
+      context var do
+        it "accepts values starting with the project repo path" do
+          env = { var => "#{project.repository.path_to_repo}/objects" }
+          rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
 
-        expect(rev_list).to be_valid
-      end
-
-      it "rejects values starting not with the project repo path" do
-        env = { "GIT_OBJECT_DIRECTORY" => "/some/other/path" }
-        rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
-
-        expect(rev_list).not_to be_valid
-      end
-
-      it "rejects values containing the project repo path but not starting with it" do
-        env = { "GIT_OBJECT_DIRECTORY" => "/some/other/path/#{project.repository.path_to_repo}" }
-        rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
-
-        expect(rev_list).not_to be_valid
-      end
-    end
+          expect(rev_list).to be_valid
+        end
 
-    context "GIT_ALTERNATE_OBJECT_DIRECTORIES" do
-      it "accepts values starting with the project repo path" do
-        env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => project.repository.path_to_repo }
-        rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
+        it "rejects values starting not with the project repo path" do
+          env = { var => "/some/other/path" }
+          rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
 
-        expect(rev_list).to be_valid
-      end
-
-      it "rejects values starting not with the project repo path" do
-        env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/some/other/path" }
-        rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
-
-        expect(rev_list).not_to be_valid
-      end
+          expect(rev_list).not_to be_valid
+        end
 
-      it "rejects values containing the project repo path but not starting with it" do
-        env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/some/other/path/#{project.repository.path_to_repo}" }
-        rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
+        it "rejects values containing the project repo path but not starting with it" do
+          env = { var => "/some/other/path/#{project.repository.path_to_repo}" }
+          rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
 
-        expect(rev_list).not_to be_valid
+          expect(rev_list).not_to be_valid
+        end
       end
     end
   end
@@ -57,20 +36,18 @@ describe Gitlab::Git::RevList, lib: true do
 
     it "calls out to `popen` without environment variables if the record is invalid" do
       allow(rev_list).to receive(:valid?).and_return(false)
-      allow(Open3).to receive(:popen3)
 
-      rev_list.execute
+      expect(Open3).to receive(:popen3).with(hash_excluding(env), any_args)
 
-      expect(Open3).to have_received(:popen3).with(hash_excluding(env), any_args)
+      rev_list.execute
     end
 
     it "calls out to `popen` with environment variables if the record is valid" do
       allow(rev_list).to receive(:valid?).and_return(true)
-      allow(Open3).to receive(:popen3)
 
-      rev_list.execute
+      expect(Open3).to receive(:popen3).with(hash_including(env), any_args)
 
-      expect(Open3).to have_received(:popen3).with(hash_including(env), any_args)
+      rev_list.execute
     end
   end
 end