Retrieve Git-specific env in Gitlab::Git::RevList and add a new #new_refs method

Signed-off-by: Rémy Coutable <remy@rymai.me>

lib/gitlab/checks/force_push.rb

@@ -8,13 +8,9 @@ module Gitlab
         if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
           false
         else
-          missed_ref, exit_status = Gitlab::Git::RevList.new(oldrev, newrev, project: project, env: env).execute
-
-          if exit_status == 0
-            missed_ref.present?
-          else
-            raise "Got a non-zero exit code while calling out to `git rev-list` in the force-push check."
-          end
+          Gitlab::Git::RevList.new(
+            path_to_repo: project.repository.path_to_repo,
+            oldrev: oldrev, newrev: newrev).missed_ref.present?
         end
       end
     end

lib/gitlab/git/rev_list.rb

 module Gitlab
   module Git
     class RevList
-      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 || {}
-        @args = [Gitlab.config.git.bin_path,
-                 "--git-dir=#{project.repository.path_to_repo}",
-                 "rev-list",
-                 "--max-count=1",
-                 oldrev,
-                 "^#{newrev}"]
+      attr_reader :oldrev, :newrev, :path_to_repo
+
+      def initialize(path_to_repo:, newrev:, oldrev: nil)
+        @oldrev = oldrev
+        @newrev = newrev
+        @path_to_repo = path_to_repo
       end
-      def execute
-        Gitlab::Popen.popen(@args, nil, parse_environment_variables)
+      # This method returns an array of new references
+      def new_refs
+        execute([*base_args, newrev, '--not', '--all'])
       end
-      def valid?
-        environment_variables.all? do |(name, value)|
-          value.to_s.start_with?(project.repository.path_to_repo)
-        end
+      # This methods returns an array of missed references
+      def missed_ref
+        execute([*base_args, '--max-count=1', oldrev, "^#{newrev}"])
       end
       private
-      def parse_environment_variables
-        return {} unless valid?
-        environment_variables
+      def execute(args)
+        output, status = Gitlab::Popen.popen(args, nil, Gitlab::Git::Env.all.stringify_keys)
+
+        unless status.zero?
+          raise "Got a non-zero exit code while calling out `#{args.join(' ')}`."
+        end
+        output.split("\n")
       end
-      def environment_variables
-        @environment_variables ||= env.slice(*ALLOWED_VARIABLES).compact
+      def base_args
+        [
+          Gitlab.config.git.bin_path,
+          "--git-dir=#{path_to_repo}",
+          'rev-list'
+        ]
       end
     end
   end

spec/lib/gitlab/git/rev_list_spec.rb

@@ -3,58 +3,54 @@ require 'spec_helper'
 describe Gitlab::Git::RevList, lib: true do
   let(:project) { create(:project, :repository) }
-  context "validations" do
-    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 = { var => "/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 = { 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
-        end
-
-        it "ignores nil values" do
-          env = { var => nil }
-          rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
-
-          expect(rev_list).to be_valid
-        end
-      end
-    end
+  before do
+    expect(Gitlab::Git::Env).to receive(:all).and_return({
+      GIT_OBJECT_DIRECTORY: 'foo',
+      GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar'
+    })
   end
-  context "#execute" do
-    let(:env) { { "GIT_OBJECT_DIRECTORY" => project.repository.path_to_repo } }
-    let(:rev_list) { Gitlab::Git::RevList.new('oldrev', 'newrev', project: project, env: env) }
-
-    it "calls out to `popen` without environment variables if the record is invalid" do
-      allow(rev_list).to receive(:valid?).and_return(false)
-
-      expect(Open3).to receive(:popen3).with(hash_excluding(env), any_args)
-
-      rev_list.execute
+  context "#new_refs" do
+    let(:rev_list) { Gitlab::Git::RevList.new(newrev: 'newrev', path_to_repo: project.repository.path_to_repo) }
+
+    it 'calls out to `popen`' do
+      expect(Gitlab::Popen).to receive(:popen).with([
+        Gitlab.config.git.bin_path,
+        "--git-dir=#{project.repository.path_to_repo}",
+        'rev-list',
+        'newrev',
+        '--not',
+        '--all'
+      ],
+      nil,
+      {
+        'GIT_OBJECT_DIRECTORY' => 'foo',
+        'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar'
+      }).and_return(["sha1\nsha2", 0])
+
+      expect(rev_list.new_refs).to eq(%w[sha1 sha2])
     end
-    it "calls out to `popen` with environment variables if the record is valid" do
-      allow(rev_list).to receive(:valid?).and_return(true)
-
-      expect(Open3).to receive(:popen3).with(hash_including(env), any_args)
-
-      rev_list.execute
+  end
+  context "#missed_ref" do
+    let(:rev_list) { Gitlab::Git::RevList.new(oldrev: 'oldrev', newrev: 'newrev', path_to_repo: project.repository.path_to_repo) }
+
+    it 'calls out to `popen`' do
+      expect(Gitlab::Popen).to receive(:popen).with([
+        Gitlab.config.git.bin_path,
+        "--git-dir=#{project.repository.path_to_repo}",
+        'rev-list',
+        '--max-count=1',
+        'oldrev',
+        '^newrev'
+      ],
+      nil,
+      {
+        'GIT_OBJECT_DIRECTORY' => 'foo',
+        'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar'
+      }).and_return(["sha1\nsha2", 0])
+
+      expect(rev_list.missed_ref).to eq(%w[sha1 sha2])
     end
   end
 end