From 3e1442766f3e2327e1e620b3b11623b09c35142b Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 15 Dec 2016 07:53:46 +0530
Subject: [PATCH] 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 +++++++++++++--
...it_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
--
GitLab