From 98affa75edde3969e0c119e9f2140710e654abf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Wed, 31 Jan 2018 16:25:38 -0300 Subject: 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 | 4 +- lib/gitlab/git/lfs_changes.rb | 3 +- lib/gitlab/git/popen.rb | 2 +- lib/gitlab/git/repository.rb | 32 +++++++++-- lib/gitlab/git/rev_list.rb | 63 +++++++--------------- spec/lib/gitlab/checks/force_push_spec.rb | 10 ++-- spec/lib/gitlab/git/rev_list_spec.rb | 57 +++++++++----------- .../services/merge_requests/rebase_service_spec.rb | 2 +- 8 files changed, 81 insertions(+), 92 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 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 diff --git a/spec/services/merge_requests/rebase_service_spec.rb b/spec/services/merge_requests/rebase_service_spec.rb index fc1c3d67203..757c31ab692 100644 --- a/spec/services/merge_requests/rebase_service_spec.rb +++ b/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) -- cgit v1.2.1