Refactor Gitlab::Git code related to LFS changes for Gitaly migration

We stop relying on Gitlab::Git::Env for the RevList class, and use
Gitlab::Git::Repository#run_git methods inteaad. The refactor also fixes
another issue, since we now top using "path_to_repo" (which is a
Repository model method).

lib/gitlab/checks/force_push.rb

@@ -15,8 +15,8 @@ module Gitlab
               .ancestor?(oldrev, newrev)
           else
             Gitlab::Git::RevList.new(
-              path_to_repo: project.repository.path_to_repo,
-              oldrev: oldrev, newrev: newrev).missed_ref.present?
+              project.repository.raw, oldrev: oldrev, newrev: newrev
+            ).missed_ref.present?
           end
         end
       end

lib/gitlab/git/lfs_changes.rb

@@ -25,8 +25,7 @@ module Gitlab
       private
 
       def rev_list
-        ::Gitlab::Git::RevList.new(path_to_repo: @repository.path_to_repo,
-                                   newrev: @newrev)
+        Gitlab::Git::RevList.new(@repository, newrev: @newrev)
       end
     end
   end

lib/gitlab/git/popen.rb

@@ -25,7 +25,7 @@ module Gitlab
           stdin.close
 
           if lazy_block
-            return lazy_block.call(stdout.lazy)
+            return [lazy_block.call(stdout.lazy), 0]
           else
             cmd_output << stdout.read
           end

lib/gitlab/git/repository.rb

@@ -1431,6 +1431,26 @@ module Gitlab
         end
       end
 
+      def rev_list(including: [], excluding: [], objects: false, &block)
+        args = ['rev-list']
+
+        args.push(*rev_list_param(including))
+
+        exclude_param = *rev_list_param(excluding)
+        if exclude_param.any?
+          args.push('--not')
+          args.push(*exclude_param)
+        end
+
+        args.push('--objects') if objects
+
+        run_git!(args, lazy_block: block)
+      end
+
+      def missed_ref(oldrev, newrev)
+        run_git!(['rev-list', '--max-count=1', oldrev, "^#{newrev}"])
+      end
+
       private
 
       def local_write_ref(ref_path, ref, old_ref: nil, shell: true)
@@ -1460,7 +1480,7 @@ module Gitlab
         Rails.logger.error "Unable to create #{ref_path} reference for repository #{path}: #{ex}"
       end
 
-      def run_git(args, chdir: path, env: {}, nice: false, &block)
+      def run_git(args, chdir: path, env: {}, nice: false, lazy_block: nil, &block)
         cmd = [Gitlab.config.git.bin_path, *args]
         cmd.unshift("nice") if nice
 
@@ -1470,12 +1490,12 @@ module Gitlab
         end
 
         circuit_breaker.perform do
-          popen(cmd, chdir, env, &block)
+          popen(cmd, chdir, env, lazy_block: lazy_block, &block)
         end
       end
 
-      def run_git!(args, chdir: path, env: {}, nice: false, &block)
-        output, status = run_git(args, chdir: chdir, env: env, nice: nice, &block)
+      def run_git!(args, chdir: path, env: {}, nice: false, lazy_block: nil, &block)
+        output, status = run_git(args, chdir: chdir, env: env, nice: nice, lazy_block: lazy_block, &block)
 
         raise GitError, output unless status.zero?
 
@@ -2336,6 +2356,10 @@ module Gitlab
       rescue Rugged::ReferenceError
         0
       end
+
+      def rev_list_param(spec)
+        spec == :all ? ['--all'] : spec
+      end
     end
   end
 end

lib/gitlab/git/rev_list.rb

@@ -5,17 +5,17 @@ module Gitlab
     class RevList
       include Gitlab::Git::Popen
 
-      attr_reader :oldrev, :newrev, :path_to_repo
+      attr_reader :oldrev, :newrev, :repository
 
-      def initialize(path_to_repo:, newrev:, oldrev: nil)
+      def initialize(repository, newrev:, oldrev: nil)
         @oldrev = oldrev
         @newrev = newrev
-        @path_to_repo = path_to_repo
+        @repository = repository
       end
 
       # This method returns an array of new commit references
       def new_refs
-        execute([*base_args, newrev, '--not', '--all'])
+        repository.rev_list(including: newrev, excluding: :all).split("\n")
       end
 
       # Finds newly added objects
@@ -28,66 +28,39 @@ module Gitlab
       # When given a block it will yield objects as a lazy enumerator so
       # the caller can limit work done instead of processing megabytes of data
       def new_objects(require_path: nil, not_in: nil, &lazy_block)
-        args = [*base_args, newrev, *not_in_refs(not_in), '--objects']
+        opts = {
+          including: newrev,
+          excluding: not_in.nil? ? :all : not_in,
+          require_path: require_path
+        }
 
-        get_objects(args, require_path: require_path, &lazy_block)
+        get_objects(opts, &lazy_block)
       end
 
       def all_objects(require_path: nil, &lazy_block)
-        args = [*base_args, '--all', '--objects']
-
-        get_objects(args, require_path: require_path, &lazy_block)
+        get_objects(including: :all, require_path: require_path, &lazy_block)
       end
 
       # This methods returns an array of missed references
       #
       # Should become obsolete after https://gitlab.com/gitlab-org/gitaly/issues/348.
       def missed_ref
-        execute([*base_args, '--max-count=1', oldrev, "^#{newrev}"])
+        repository.missed_ref(oldrev, newrev).split("\n")
       end
 
       private
 
-      def not_in_refs(references)
-        return ['--not', '--all'] unless references
-        return [] if references.empty?
-
-        references.prepend('--not')
-      end
-
       def execute(args)
-        output, status = popen(args, nil, Gitlab::Git::Env.to_env_hash)
-
-        unless status.zero?
-          raise "Got a non-zero exit code while calling out `#{args.join(' ')}`: #{output}"
-        end
-
-        output.split("\n")
+        repository.rev_list(args).split("\n")
       end
 
-      def lazy_execute(args, &lazy_block)
-        popen(args, nil, Gitlab::Git::Env.to_env_hash, lazy_block: lazy_block)
-      end
+      def get_objects(including: [], excluding: [], require_path: nil)
+        opts = { including: including, excluding: excluding, objects: true }
 
-      def base_args
-        [
-          Gitlab.config.git.bin_path,
-          "--git-dir=#{path_to_repo}",
-          'rev-list'
-        ]
-      end
+        repository.rev_list(opts) do |lazy_output|
+          objects = objects_from_output(lazy_output, require_path: require_path)
 
-      def get_objects(args, require_path: nil)
-        if block_given?
-          lazy_execute(args) do |lazy_output|
-            objects = objects_from_output(lazy_output, require_path: require_path)
-
-            yield(objects)
-          end
-        else
-          object_output = execute(args)
-
-          objects_from_output(object_output, require_path: require_path)
+          yield(objects)
         end
       end
 

spec/lib/gitlab/checks/force_push_spec.rb

@@ -2,18 +2,20 @@ require 'spec_helper'
 
 describe Gitlab::Checks::ForcePush do
   let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository.raw }
 
   context "exit code checking", :skip_gitaly_mock do
     it "does not raise a runtime error if the `popen` call to git returns a zero exit code" do
-      allow_any_instance_of(Gitlab::Git::RevList).to receive(:popen).and_return(['normal output', 0])
+      allow(repository).to receive(:popen).and_return(['normal output', 0])
 
       expect { described_class.force_push?(project, 'oldrev', 'newrev') }.not_to raise_error
     end
 
-    it "raises a runtime error if the `popen` call to git returns a non-zero exit code" do
-      allow_any_instance_of(Gitlab::Git::RevList).to receive(:popen).and_return(['error', 1])
+    it "raises a GitError error if the `popen` call to git returns a non-zero exit code" do
+      allow(repository).to receive(:popen).and_return(['error', 1])
 
-      expect { described_class.force_push?(project, 'oldrev', 'newrev') }.to raise_error(RuntimeError)
+      expect { described_class.force_push?(project, 'oldrev', 'newrev') }
+        .to raise_error(Gitlab::Git::Repository::GitError)
     end
   end
 end

spec/lib/gitlab/git/rev_list_spec.rb

@@ -1,51 +1,42 @@
 require 'spec_helper'
 
 describe Gitlab::Git::RevList do
-  let(:project) { create(:project, :repository) }
-  let(:rev_list) { described_class.new(newrev: 'newrev', path_to_repo: project.repository.path_to_repo) }
+  let(:repository) { create(:project, :repository).repository.raw }
+  let(:rev_list) { described_class.new(repository, newrev: 'newrev') }
   let(:env_hash) do
     {
       'GIT_OBJECT_DIRECTORY' => 'foo',
       'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar'
     }
   end
+  let(:command_env) { { 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'foo:bar' } }
 
   before do
-    allow(Gitlab::Git::Env).to receive(:all).and_return(env_hash.symbolize_keys)
+    allow(Gitlab::Git::Env).to receive(:all).and_return(env_hash)
   end
 
   def args_for_popen(args_list)
-    [
-      Gitlab.config.git.bin_path,
-      "--git-dir=#{project.repository.path_to_repo}",
-      'rev-list',
-      *args_list
-    ]
+    [Gitlab.config.git.bin_path, 'rev-list', *args_list]
   end
 
-  def stub_popen_rev_list(*additional_args, output:)
-    args = args_for_popen(additional_args)
-
-    expect(rev_list).to receive(:popen).with(args, nil, env_hash)
-                                       .and_return([output, 0])
-  end
-
-  def stub_lazy_popen_rev_list(*additional_args, output:)
+  def stub_popen_rev_list(*additional_args, with_lazy_block: true, output:)
     params = [
       args_for_popen(additional_args),
-      nil,
-      env_hash,
-      hash_including(lazy_block: anything)
+      repository.path,
+      command_env,
+      hash_including(lazy_block: with_lazy_block ? anything : nil)
     ]
 
-    expect(rev_list).to receive(:popen).with(*params) do |*_, lazy_block:|
-      lazy_block.call(output.lines.lazy.map(&:chomp))
+    expect(repository).to receive(:popen).with(*params) do |*_, lazy_block:|
+      output = lazy_block.call(output.lines.lazy.map(&:chomp)) if with_lazy_block
+
+      [output, 0]
     end
   end
 
   context "#new_refs" do
     it 'calls out to `popen`' do
-      stub_popen_rev_list('newrev', '--not', '--all', output: "sha1\nsha2")
+      stub_popen_rev_list('newrev', '--not', '--all', with_lazy_block: false, output: "sha1\nsha2")
 
       expect(rev_list.new_refs).to eq(%w[sha1 sha2])
     end
@@ -55,18 +46,18 @@ describe Gitlab::Git::RevList do
     it 'fetches list of newly pushed objects using rev-list' do
       stub_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2")
 
-      expect(rev_list.new_objects).to eq(%w[sha1 sha2])
+      expect { |b| rev_list.new_objects(&b) }.to yield_with_args(%w[sha1 sha2])
     end
 
     it 'can skip pathless objects' do
       stub_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2 path/to/file")
 
-      expect(rev_list.new_objects(require_path: true)).to eq(%w[sha2])
+      expect { |b| rev_list.new_objects(require_path: true, &b) }.to yield_with_args(%w[sha2])
     end
 
     it 'can handle non utf-8 paths' do
       non_utf_char = [0x89].pack("c*").force_encoding("UTF-8")
-      stub_lazy_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha2 πå†h/†ø/ƒîlé#{non_utf_char}\nsha1")
+      stub_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha2 πå†h/†ø/ƒîlé#{non_utf_char}\nsha1")
 
       rev_list.new_objects(require_path: true) do |object_ids|
         expect(object_ids.force).to eq(%w[sha2])
@@ -74,7 +65,7 @@ describe Gitlab::Git::RevList do
     end
 
     it 'can yield a lazy enumerator' do
-      stub_lazy_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2")
+      stub_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2")
 
       rev_list.new_objects do |object_ids|
         expect(object_ids).to be_a Enumerator::Lazy
@@ -82,7 +73,7 @@ describe Gitlab::Git::RevList do
     end
 
     it 'returns the result of the block when given' do
-      stub_lazy_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2")
+      stub_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2")
 
       objects = rev_list.new_objects do |object_ids|
         object_ids.first
@@ -94,13 +85,13 @@ describe Gitlab::Git::RevList do
     it 'can accept list of references to exclude' do
       stub_popen_rev_list('newrev', '--not', 'master', '--objects', output: "sha1\nsha2")
 
-      expect(rev_list.new_objects(not_in: ['master'])).to eq(%w[sha1 sha2])
+      expect { |b| rev_list.new_objects(not_in: ['master'], &b) }.to yield_with_args(%w[sha1 sha2])
     end
 
     it 'handles empty list of references to exclude as listing all known objects' do
       stub_popen_rev_list('newrev', '--objects', output: "sha1\nsha2")
 
-      expect(rev_list.new_objects(not_in: [])).to eq(%w[sha1 sha2])
+      expect { |b| rev_list.new_objects(not_in: [], &b) }.to yield_with_args(%w[sha1 sha2])
     end
   end
 
@@ -108,15 +99,15 @@ describe Gitlab::Git::RevList do
     it 'fetches list of all pushed objects using rev-list' do
       stub_popen_rev_list('--all', '--objects', output: "sha1\nsha2")
 
-      expect(rev_list.all_objects).to eq(%w[sha1 sha2])
+      expect { |b| rev_list.all_objects(&b) }.to yield_with_args(%w[sha1 sha2])
     end
   end
 
   context "#missed_ref" do
-    let(:rev_list) { described_class.new(oldrev: 'oldrev', newrev: 'newrev', path_to_repo: project.repository.path_to_repo) }
+    let(:rev_list) { described_class.new(repository, oldrev: 'oldrev', newrev: 'newrev') }
 
     it 'calls out to `popen`' do
-      stub_popen_rev_list('--max-count=1', 'oldrev', '^newrev', output: "sha1\nsha2")
+      stub_popen_rev_list('--max-count=1', 'oldrev', '^newrev', with_lazy_block: false, output: "sha1\nsha2")
 
       expect(rev_list.missed_ref).to eq(%w[sha1 sha2])
     end

spec/services/merge_requests/rebase_service_spec.rb

@@ -108,7 +108,7 @@ describe MergeRequests::RebaseService do
       context 'git commands', :disable_gitaly do
         it 'sets GL_REPOSITORY env variable when calling git commands' do
           expect(repository).to receive(:popen).exactly(3)
-            .with(anything, anything, hash_including('GL_REPOSITORY'))
+            .with(anything, anything, hash_including('GL_REPOSITORY'), anything)
             .and_return(['', 0])
 
           service.execute(merge_request)