diff options
| -rw-r--r-- | lib/gitlab/checks/force_push.rb | 10 | ||||
| -rw-r--r-- | lib/gitlab/git/rev_list.rb | 49 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/rev_list_spec.rb | 92 |
3 files changed, 72 insertions, 79 deletions
diff --git a/lib/gitlab/checks/force_push.rb b/lib/gitlab/checks/force_push.rb index de0c9049ebf..7284849aa18 100644 --- a/lib/gitlab/checks/force_push.rb +++ b/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 diff --git a/lib/gitlab/git/rev_list.rb b/lib/gitlab/git/rev_list.rb index 79dd0cf7df2..a16b0ed76f4 100644 --- a/lib/gitlab/git/rev_list.rb +++ b/lib/gitlab/git/rev_list.rb @@ -1,41 +1,42 @@ 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? + 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 - environment_variables + 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 diff --git a/spec/lib/gitlab/git/rev_list_spec.rb b/spec/lib/gitlab/git/rev_list_spec.rb index d48629a296d..78894ba9409 100644 --- a/spec/lib/gitlab/git/rev_list_spec.rb +++ b/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 + 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 + 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 |