Implement review comments from @dbalexandre.

- Don't define "allowed environment variables" in two places.
- Dispatch to different arities of `Popen.open` without an if/else block.
- Use `described_class` instead of explicitly stating the class name within a
- spec.
- Remove `git_environment_variables_validator_spec` and keep the validation inline.

1 files changed, 12 insertions, 10 deletions

diff --git a/lib/gitlab/git/rev_list.rb b/lib/gitlab/git/rev_list.rb
index d8c78d806ea..ecd038e04df 100644
--- a/lib/gitlab/git/rev_list.rb
+++ b/lib/gitlab/git/rev_list.rb
@@ -3,12 +3,10 @@
 module Gitlab
   module Git
     class RevList
-      include ActiveModel::Validations
-
-      validates :env, git_environment_variables: true
-
       attr_reader :project, :env
 
+      ALLOWED_VARIABLES = %w(GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES).freeze
+
       def initialize(oldrev, newrev, project:, env: nil)
         @project = project
         @env = env.presence || {}
@@ -21,17 +19,21 @@ module Gitlab
       end
 
       def execute
-        if self.valid?
-          Gitlab::Popen.popen(@args, nil, @env.slice(*allowed_environment_variables))
-        else
-          Gitlab::Popen.popen(@args)
+        Gitlab::Popen.popen(@args, nil, parse_environment_variables)
+      end
+
+      def valid?
+        env.slice(*ALLOWED_VARIABLES).all? do |(name, value)|
+          value =~ /^#{project.repository.path_to_repo}/
         end
       end
 
       private
 
-      def allowed_environment_variables
-        %w(GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_OBJECT_DIRECTORY)
+      def parse_environment_variables
+        return {} unless valid?
+
+        env.slice(*ALLOWED_VARIABLES)
       end
     end
   end