Commit e394d287 authored by Timothy Andrew's avatar Timothy Andrew

Implement final review comments from @rymai.

- `raise "string"` raises a `RuntimeError` - no need to be explicit
- Remove top-level comment in the `RevList` class
- Use `%w()` instead of `%w[]`
- Extract an `environment_variables` method to cache `env.slice(*ALLOWED_VARIABLES)`
- Use `start_with?` for env variable validation instead of regex match
- Validation specs for each allowed environment variable were identical. Build them dynamically.
- Minor change to `popen3` expectation.
parent 3e144276
...@@ -13,7 +13,7 @@ module Gitlab ...@@ -13,7 +13,7 @@ module Gitlab
if exit_status == 0 if exit_status == 0
missed_ref.present? missed_ref.present?
else else
raise RuntimeError, "Got a non-zero exit code while calling out to `git rev-list` in the force-push check." raise "Got a non-zero exit code while calling out to `git rev-list` in the force-push check."
end end
end end
end end
......
# Call out to the `git rev-list` command
module Gitlab module Gitlab
module Git module Git
class RevList class RevList
attr_reader :project, :env attr_reader :project, :env
ALLOWED_VARIABLES = %w(GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES).freeze ALLOWED_VARIABLES = %w[GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES].freeze
def initialize(oldrev, newrev, project:, env: nil) def initialize(oldrev, newrev, project:, env: nil)
@project = project @project = project
...@@ -23,8 +21,8 @@ module Gitlab ...@@ -23,8 +21,8 @@ module Gitlab
end end
def valid? def valid?
env.slice(*ALLOWED_VARIABLES).all? do |(name, value)| environment_variables.all? do |(name, value)|
value =~ /^#{project.repository.path_to_repo}/ value.start_with?(project.repository.path_to_repo)
end end
end end
...@@ -33,7 +31,11 @@ module Gitlab ...@@ -33,7 +31,11 @@ module Gitlab
def parse_environment_variables def parse_environment_variables
return {} unless valid? return {} unless valid?
env.slice(*ALLOWED_VARIABLES) environment_variables
end
def environment_variables
@environment_variables ||= env.slice(*ALLOWED_VARIABLES)
end end
end end
end end
......
...@@ -4,50 +4,29 @@ describe Gitlab::Git::RevList, lib: true do ...@@ -4,50 +4,29 @@ describe Gitlab::Git::RevList, lib: true do
let(:project) { create(:project) } let(:project) { create(:project) }
context "validations" do context "validations" do
context "GIT_OBJECT_DIRECTORY" do described_class::ALLOWED_VARIABLES.each do |var|
context var do
it "accepts values starting with the project repo path" do it "accepts values starting with the project repo path" do
env = { "GIT_OBJECT_DIRECTORY" => "#{project.repository.path_to_repo}/objects" } env = { var => "#{project.repository.path_to_repo}/objects" }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env) rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).to be_valid expect(rev_list).to be_valid
end end
it "rejects values starting not with the project repo path" do it "rejects values starting not with the project repo path" do
env = { "GIT_OBJECT_DIRECTORY" => "/some/other/path" } env = { var => "/some/other/path" }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env) rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).not_to be_valid expect(rev_list).not_to be_valid
end end
it "rejects values containing the project repo path but not starting with it" do 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}" } env = { var => "/some/other/path/#{project.repository.path_to_repo}" }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env) rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).not_to be_valid expect(rev_list).not_to be_valid
end end
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
end end
...@@ -57,20 +36,18 @@ describe Gitlab::Git::RevList, lib: true do ...@@ -57,20 +36,18 @@ describe Gitlab::Git::RevList, lib: true do
it "calls out to `popen` without environment variables if the record is invalid" do it "calls out to `popen` without environment variables if the record is invalid" do
allow(rev_list).to receive(:valid?).and_return(false) allow(rev_list).to receive(:valid?).and_return(false)
allow(Open3).to receive(:popen3)
rev_list.execute expect(Open3).to receive(:popen3).with(hash_excluding(env), any_args)
expect(Open3).to have_received(:popen3).with(hash_excluding(env), any_args) rev_list.execute
end end
it "calls out to `popen` with environment variables if the record is valid" do it "calls out to `popen` with environment variables if the record is valid" do
allow(rev_list).to receive(:valid?).and_return(true) allow(rev_list).to receive(:valid?).and_return(true)
allow(Open3).to receive(:popen3)
rev_list.execute expect(Open3).to receive(:popen3).with(hash_including(env), any_args)
expect(Open3).to have_received(:popen3).with(hash_including(env), any_args) rev_list.execute
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment