Merge branch 'gitaly-lfs-client-prep' into 'master'

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

Closes gitaly#980

See merge request gitlab-org/gitlab-ce!16835

2 files changed, 30 insertions, 37 deletions

diff --git a/spec/lib/gitlab/checks/force_push_spec.rb b/spec/lib/gitlab/checks/force_push_spec.rb
index 633e319f46d..a65012d2314 100644
--- a/spec/lib/gitlab/checks/force_push_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/git/rev_list_spec.rb b/spec/lib/gitlab/git/rev_list_spec.rb
index 90fbef9d248..4e0ee206219 100644
--- a/spec/lib/gitlab/git/rev_list_spec.rb
+++ b/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
-    ]
-  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])
+    [Gitlab.config.git.bin_path, 'rev-list', *args_list]
   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