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).

5 files changed, 50 insertions, 54 deletions

diff --git a/lib/gitlab/checks/force_push.rb b/lib/gitlab/checks/force_push.rb
index dc5d285ea65..c9c3050cfc2 100644
--- a/lib/gitlab/checks/force_push.rb
+++ b/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
diff --git a/lib/gitlab/git/lfs_changes.rb b/lib/gitlab/git/lfs_changes.rb
index 732dd5d998a..48434047fce 100644
--- a/lib/gitlab/git/lfs_changes.rb
+++ b/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
diff --git a/lib/gitlab/git/popen.rb b/lib/gitlab/git/popen.rb
index e0bd2bbe47b..c1767046ff0 100644
--- a/lib/gitlab/git/popen.rb
+++ b/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
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 7127f7858ee..635c94d9dd2 100644
--- a/lib/gitlab/git/repository.rb
+++ b/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
diff --git a/lib/gitlab/git/rev_list.rb b/lib/gitlab/git/rev_list.rb
index f8b2e7e0e21..38c3a55f96f 100644
--- a/lib/gitlab/git/rev_list.rb
+++ b/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")
-      end
-
-      def lazy_execute(args, &lazy_block)
-        popen(args, nil, Gitlab::Git::Env.to_env_hash, lazy_block: lazy_block)
-      end
-
-      def base_args
-        [
-          Gitlab.config.git.bin_path,
-          "--git-dir=#{path_to_repo}",
-          'rev-list'
-        ]
+        repository.rev_list(args).split("\n")
       end
 
-      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)
+      def get_objects(including: [], excluding: [], require_path: nil)
+        opts = { including: including, excluding: excluding, objects: true }
 
-            yield(objects)
-          end
-        else
-          object_output = execute(args)
+        repository.rev_list(opts) do |lazy_output|
+          objects = objects_from_output(lazy_output, require_path: require_path)
 
-          objects_from_output(object_output, require_path: require_path)
+          yield(objects)
         end
       end