From 3e1442766f3e2327e1e620b3b11623b09c35142b Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 15 Dec 2016 07:53:46 +0530 Subject: Implement review comments from @dbalexandre. - Don't define "allowed environment variables" in two places. - Dispatch to different arities of `Popen.open` without an if/else block. - Use `described_class` instead of explicitly stating the class name within a - spec. - Remove `git_environment_variables_validator_spec` and keep the validation inline. --- .../git_environment_variables_validator.rb | 13 ----- lib/gitlab/git/rev_list.rb | 22 ++++---- spec/lib/gitlab/git/rev_list_spec.rb | 50 +++++++++++++++-- .../git_environment_variables_validator_spec.rb | 64 ---------------------- 4 files changed, 57 insertions(+), 92 deletions(-) delete mode 100644 app/validators/git_environment_variables_validator.rb delete mode 100644 spec/validators/git_environment_variables_validator_spec.rb diff --git a/app/validators/git_environment_variables_validator.rb b/app/validators/git_environment_variables_validator.rb deleted file mode 100644 index 92041e0a773..00000000000 --- a/app/validators/git_environment_variables_validator.rb +++ /dev/null @@ -1,13 +0,0 @@ -class GitEnvironmentVariablesValidator < ActiveModel::EachValidator - def validate_each(record, attribute, env) - variables_to_validate = %w(GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES) - - variables_to_validate.each do |variable_name| - variable_value = env[variable_name] - - if variable_value.present? && !(variable_value =~ /^#{record.project.repository.path_to_repo}/) - record.errors.add(attribute, "The #{variable_name} variable must start with the project repo path") - end - end - end -end diff --git a/lib/gitlab/git/rev_list.rb b/lib/gitlab/git/rev_list.rb index d8c78d806ea..ecd038e04df 100644 --- a/lib/gitlab/git/rev_list.rb +++ b/lib/gitlab/git/rev_list.rb @@ -3,12 +3,10 @@ module Gitlab module Git class RevList - include ActiveModel::Validations - - validates :env, git_environment_variables: true - 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 || {} @@ -21,17 +19,21 @@ module Gitlab end def execute - if self.valid? - Gitlab::Popen.popen(@args, nil, @env.slice(*allowed_environment_variables)) - else - Gitlab::Popen.popen(@args) + Gitlab::Popen.popen(@args, nil, parse_environment_variables) + end + + def valid? + env.slice(*ALLOWED_VARIABLES).all? do |(name, value)| + value =~ /^#{project.repository.path_to_repo}/ end end private - def allowed_environment_variables - %w(GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_OBJECT_DIRECTORY) + def parse_environment_variables + return {} unless valid? + + env.slice(*ALLOWED_VARIABLES) end end end diff --git a/spec/lib/gitlab/git/rev_list_spec.rb b/spec/lib/gitlab/git/rev_list_spec.rb index cdfbff5658c..f76aca29107 100644 --- a/spec/lib/gitlab/git/rev_list_spec.rb +++ b/spec/lib/gitlab/git/rev_list_spec.rb @@ -1,14 +1,54 @@ require 'spec_helper' -require 'validators/git_environment_variables_validator_spec' describe Gitlab::Git::RevList, lib: true do let(:project) { create(:project) } context "validations" do - it_behaves_like( - "validated git environment variables", - ->(env, project) { Gitlab::Git::RevList.new('oldrev', 'newrev', project: project, env: env) } - ) + context "GIT_OBJECT_DIRECTORY" do + it "accepts values starting with the project repo path" do + env = { "GIT_OBJECT_DIRECTORY" => "#{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 = { "GIT_OBJECT_DIRECTORY" => "/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 = { "GIT_OBJECT_DIRECTORY" => "/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 + end + + context "GIT_ALTERNATE_OBJECT_DIRECTORIES" do + it "accepts values starting with the project repo path" do + env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => project.repository.path_to_repo } + 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 = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/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 = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/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 + end end context "#execute" do diff --git a/spec/validators/git_environment_variables_validator_spec.rb b/spec/validators/git_environment_variables_validator_spec.rb deleted file mode 100644 index 81b028b6572..00000000000 --- a/spec/validators/git_environment_variables_validator_spec.rb +++ /dev/null @@ -1,64 +0,0 @@ -require 'spec_helper' - -shared_examples_for "validated git environment variables" do |record_fn| - subject { GitEnvironmentVariablesValidator.new(attributes: ['env']) } - let(:project) { create(:project) } - - context "GIT_OBJECT_DIRECTORY" do - it "accepts values starting with the project repo path" do - env = { "GIT_OBJECT_DIRECTORY" => "#{project.repository.path_to_repo}/objects" } - record = record_fn[env, project] - - subject.validate_each(record, 'env', env) - - expect(record).to be_valid, "expected #{project.repository.path_to_repo}" - end - - it "rejects values starting not with the project repo path" do - env = { "GIT_OBJECT_DIRECTORY" => "/some/other/path" } - record = record_fn[env, project] - - subject.validate_each(record, 'env', env) - - expect(record).to be_invalid - end - - it "rejects values containing the project repo path but not starting with it" do - env = { "GIT_OBJECT_DIRECTORY" => "/some/other/path/#{project.repository.path_to_repo}" } - record = record_fn[env, project] - - subject.validate_each(record, 'env', env) - - expect(record).to be_invalid - end - end - - context "GIT_ALTERNATE_OBJECT_DIRECTORIES" do - it "accepts values starting with the project repo path" do - env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => project.repository.path_to_repo } - record = record_fn[env, project] - - subject.validate_each(record, 'env', env) - - expect(record).to be_valid, "expected #{project.repository.path_to_repo}" - end - - it "rejects values starting not with the project repo path" do - env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/some/other/path" } - record = record_fn[env, project] - - subject.validate_each(record, 'env', env) - - expect(record).to be_invalid - end - - it "rejects values containing the project repo path but not starting with it" do - env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/some/other/path/#{project.repository.path_to_repo}" } - record = record_fn[env, project] - - subject.validate_each(record, 'env', env) - - expect(record).to be_invalid - end - end -end -- cgit v1.2.1