diff options
| -rw-r--r-- | lib/api/helpers/internal_helpers.rb | 12 | ||||
| -rw-r--r-- | lib/api/internal.rb | 3 | ||||
| -rw-r--r-- | lib/gitlab/git/hook_env.rb (renamed from lib/gitlab/git/env.rb) | 26 | ||||
| -rw-r--r-- | lib/gitlab/git/repository.rb | 14 | ||||
| -rw-r--r-- | lib/gitlab/gitaly_client/util.rb | 8 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/hook_env_spec.rb (renamed from spec/lib/gitlab/git/env_spec.rb) | 71 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/repository_spec.rb | 20 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/rev_list_spec.rb | 13 | ||||
| -rw-r--r-- | spec/lib/gitlab/gitaly_client/util_spec.rb | 11 | ||||
| -rw-r--r-- | spec/requests/api/internal_spec.rb | 49 |
10 files changed, 70 insertions, 157 deletions
diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 14648588dfd..abe3d353984 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -29,18 +29,6 @@ module API {} end - def fix_git_env_repository_paths(env, repository_path) - if obj_dir_relative = env['GIT_OBJECT_DIRECTORY_RELATIVE'].presence - env['GIT_OBJECT_DIRECTORY'] = File.join(repository_path, obj_dir_relative) - end - - if alt_obj_dirs_relative = env['GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE'].presence - env['GIT_ALTERNATE_OBJECT_DIRECTORIES'] = alt_obj_dirs_relative.map { |dir| File.join(repository_path, dir) } - end - - env - end - def log_user_activity(actor) commands = Gitlab::GitAccess::DOWNLOAD_COMMANDS diff --git a/lib/api/internal.rb b/lib/api/internal.rb index b3660e4a1d0..fcbc248fc3b 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -21,8 +21,7 @@ module API # Stores some Git-specific env thread-safely env = parse_env - env = fix_git_env_repository_paths(env, repository_path) if project - Gitlab::Git::Env.set(env) + Gitlab::Git::HookEnv.set(gl_repository, env) if project actor = if params[:key_id] diff --git a/lib/gitlab/git/env.rb b/lib/gitlab/git/hook_env.rb index 9d0b47a1a6d..455e8451c10 100644 --- a/lib/gitlab/git/env.rb +++ b/lib/gitlab/git/hook_env.rb @@ -3,37 +3,39 @@ module Gitlab module Git # Ephemeral (per request) storage for environment variables that some Git - # commands may need. + # commands need during internal API calls made from Git push hooks. # # For example, in pre-receive hooks, new objects are put in a temporary # $GIT_OBJECT_DIRECTORY. Without it set, the new objects cannot be retrieved # (this would break push rules for instance). # # This class is thread-safe via RequestStore. - class Env + class HookEnv WHITELISTED_VARIABLES = %w[ - GIT_OBJECT_DIRECTORY GIT_OBJECT_DIRECTORY_RELATIVE - GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE ].freeze - def self.set(env) + def self.set(gl_repository, env) return unless RequestStore.active? - RequestStore.store[:gitlab_git_env] = whitelist_git_env(env) + raise "missing gl_repository" if gl_repository.blank? + + RequestStore.store[:gitlab_git_env] ||= {} + RequestStore.store[:gitlab_git_env][gl_repository] = whitelist_git_env(env) end - def self.all + def self.all(gl_repository) return {} unless RequestStore.active? - RequestStore.fetch(:gitlab_git_env) { {} } + h = RequestStore.fetch(:gitlab_git_env) { {} } + h.fetch(gl_repository, {}) end - def self.to_env_hash + def self.to_env_hash(gl_repository) env = {} - all.compact.each do |key, value| + all(gl_repository).compact.each do |key, value| value = value.join(File::PATH_SEPARATOR) if value.is_a?(Array) env[key.to_s] = value end @@ -41,10 +43,6 @@ module Gitlab env end - def self.[](key) - all[key] - end - def self.whitelist_git_env(env) env.select { |key, _| WHITELISTED_VARIABLES.include?(key.to_s) }.with_indifferent_access end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 2d16a81c888..e692c9ce342 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1745,21 +1745,11 @@ module Gitlab end def alternate_object_directories - relative_paths = relative_object_directories - - if relative_paths.any? - relative_paths.map { |d| File.join(path, d) } - else - absolute_object_directories.flat_map { |d| d.split(File::PATH_SEPARATOR) } - end + relative_object_directories.map { |d| File.join(path, d) } end def relative_object_directories - Gitlab::Git::Env.all.values_at(*ALLOWED_OBJECT_RELATIVE_DIRECTORIES_VARIABLES).flatten.compact - end - - def absolute_object_directories - Gitlab::Git::Env.all.values_at(*ALLOWED_OBJECT_DIRECTORIES_VARIABLES).flatten.compact + Gitlab::Git::HookEnv.all(gl_repository).values_at(*ALLOWED_OBJECT_RELATIVE_DIRECTORIES_VARIABLES).flatten.compact end # Get the content of a blob for a given commit. If the blob is a commit diff --git a/lib/gitlab/gitaly_client/util.rb b/lib/gitlab/gitaly_client/util.rb index a8c6d478de8..405567db94a 100644 --- a/lib/gitlab/gitaly_client/util.rb +++ b/lib/gitlab/gitaly_client/util.rb @@ -3,11 +3,9 @@ module Gitlab module Util class << self def repository(repository_storage, relative_path, gl_repository) - git_object_directory = Gitlab::Git::Env['GIT_OBJECT_DIRECTORY_RELATIVE'].presence || - Gitlab::Git::Env['GIT_OBJECT_DIRECTORY'].presence - git_alternate_object_directories = - Array.wrap(Gitlab::Git::Env['GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE']).presence || - Array.wrap(Gitlab::Git::Env['GIT_ALTERNATE_OBJECT_DIRECTORIES']).flat_map { |d| d.split(File::PATH_SEPARATOR) } + git_env = Gitlab::Git::HookEnv.all(gl_repository) + git_object_directory = git_env['GIT_OBJECT_DIRECTORY_RELATIVE'].presence + git_alternate_object_directories = Array.wrap(git_env['GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE']) Gitaly::Repository.new( storage_name: repository_storage, diff --git a/spec/lib/gitlab/git/env_spec.rb b/spec/lib/gitlab/git/hook_env_spec.rb index 03836d49518..e6aa5ad8c90 100644 --- a/spec/lib/gitlab/git/env_spec.rb +++ b/spec/lib/gitlab/git/hook_env_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' -describe Gitlab::Git::Env do +describe Gitlab::Git::HookEnv do + let(:gl_repository) { 'project-123' } + describe ".set" do context 'with RequestStore.store disabled' do before do @@ -8,9 +10,9 @@ describe Gitlab::Git::Env do end it 'does not store anything' do - described_class.set(GIT_OBJECT_DIRECTORY: 'foo') + described_class.set(gl_repository, GIT_OBJECT_DIRECTORY_RELATIVE: 'foo') - expect(described_class.all).to be_empty + expect(described_class.all(gl_repository)).to be_empty end end @@ -21,15 +23,19 @@ describe Gitlab::Git::Env do it 'whitelist some `GIT_*` variables and stores them using RequestStore' do described_class.set( - GIT_OBJECT_DIRECTORY: 'foo', - GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar', + gl_repository, + GIT_OBJECT_DIRECTORY_RELATIVE: 'foo', + GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE: 'bar', GIT_EXEC_PATH: 'baz', PATH: '~/.bin:/bin') - expect(described_class[:GIT_OBJECT_DIRECTORY]).to eq('foo') - expect(described_class[:GIT_ALTERNATE_OBJECT_DIRECTORIES]).to eq('bar') - expect(described_class[:GIT_EXEC_PATH]).to be_nil - expect(described_class[:bar]).to be_nil + git_env = described_class.all(gl_repository) + + expect(git_env[:GIT_OBJECT_DIRECTORY_RELATIVE]).to eq('foo') + expect(git_env[:GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE]).to eq('bar') + expect(git_env[:GIT_EXEC_PATH]).to be_nil + expect(git_env[:PATH]).to be_nil + expect(git_env[:bar]).to be_nil end end end @@ -39,14 +45,15 @@ describe Gitlab::Git::Env do before do allow(RequestStore).to receive(:active?).and_return(true) described_class.set( - GIT_OBJECT_DIRECTORY: 'foo', - GIT_ALTERNATE_OBJECT_DIRECTORIES: ['bar']) + gl_repository, + GIT_OBJECT_DIRECTORY_RELATIVE: 'foo', + GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE: ['bar']) end it 'returns an env hash' do - expect(described_class.all).to eq({ - 'GIT_OBJECT_DIRECTORY' => 'foo', - 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => ['bar'] + expect(described_class.all(gl_repository)).to eq({ + 'GIT_OBJECT_DIRECTORY_RELATIVE' => 'foo', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => ['bar'] }) end end @@ -56,8 +63,8 @@ describe Gitlab::Git::Env do context 'with RequestStore.store enabled' do using RSpec::Parameterized::TableSyntax - let(:key) { 'GIT_OBJECT_DIRECTORY' } - subject { described_class.to_env_hash } + let(:key) { 'GIT_OBJECT_DIRECTORY_RELATIVE' } + subject { described_class.to_env_hash(gl_repository) } where(:input, :output) do nil | nil @@ -70,7 +77,7 @@ describe Gitlab::Git::Env do with_them do before do allow(RequestStore).to receive(:active?).and_return(true) - described_class.set(key.to_sym => input) + described_class.set(gl_repository, key.to_sym => input) end it 'puts the right value in the hash' do @@ -84,47 +91,25 @@ describe Gitlab::Git::Env do end end - describe ".[]" do - context 'with RequestStore.store enabled' do - before do - allow(RequestStore).to receive(:active?).and_return(true) - end - - before do - described_class.set( - GIT_OBJECT_DIRECTORY: 'foo', - GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar') - end - - it 'returns a stored value for an existing key' do - expect(described_class[:GIT_OBJECT_DIRECTORY]).to eq('foo') - end - - it 'returns nil for an non-existing key' do - expect(described_class[:foo]).to be_nil - end - end - end - describe 'thread-safety' do context 'with RequestStore.store enabled' do before do allow(RequestStore).to receive(:active?).and_return(true) - described_class.set(GIT_OBJECT_DIRECTORY: 'foo') + described_class.set(gl_repository, GIT_OBJECT_DIRECTORY_RELATIVE: 'foo') end it 'is thread-safe' do another_thread = Thread.new do - described_class.set(GIT_OBJECT_DIRECTORY: 'bar') + described_class.set(gl_repository, GIT_OBJECT_DIRECTORY_RELATIVE: 'bar') Thread.stop - described_class[:GIT_OBJECT_DIRECTORY] + described_class.all(gl_repository)[:GIT_OBJECT_DIRECTORY_RELATIVE] end # Ensure another_thread runs first sleep 0.1 until another_thread.stop? - expect(described_class[:GIT_OBJECT_DIRECTORY]).to eq('foo') + expect(described_class.all(gl_repository)[:GIT_OBJECT_DIRECTORY_RELATIVE]).to eq('foo') another_thread.run expect(another_thread.value).to eq('bar') diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 0e315b3f49e..5cbe2808d0b 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -120,7 +120,7 @@ describe Gitlab::Git::Repository, seed_helper: true do describe 'alternates keyword argument' do context 'with no Git env stored' do before do - allow(Gitlab::Git::Env).to receive(:all).and_return({}) + allow(Gitlab::Git::HookEnv).to receive(:all).and_return({}) end it "is passed an empty array" do @@ -132,7 +132,7 @@ describe Gitlab::Git::Repository, seed_helper: true do context 'with absolute and relative Git object dir envvars stored' do before do - allow(Gitlab::Git::Env).to receive(:all).and_return({ + allow(Gitlab::Git::HookEnv).to receive(:all).and_return({ 'GIT_OBJECT_DIRECTORY_RELATIVE' => './objects/foo', 'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => ['./objects/bar', './objects/baz'], 'GIT_OBJECT_DIRECTORY' => 'ignored', @@ -148,22 +148,6 @@ describe Gitlab::Git::Repository, seed_helper: true do repository.rugged end end - - context 'with only absolute Git object dir envvars stored' do - before do - allow(Gitlab::Git::Env).to receive(:all).and_return({ - 'GIT_OBJECT_DIRECTORY' => 'foo', - 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => %w[bar baz], - 'GIT_OTHER' => 'another_env' - }) - end - - it "is passed the absolute object dir envvars as is" do - expect(Rugged::Repository).to receive(:new).with(repository.path, alternates: %w[foo bar baz]) - - repository.rugged - end - end end end diff --git a/spec/lib/gitlab/git/rev_list_spec.rb b/spec/lib/gitlab/git/rev_list_spec.rb index 4e0ee206219..32ec1e029c8 100644 --- a/spec/lib/gitlab/git/rev_list_spec.rb +++ b/spec/lib/gitlab/git/rev_list_spec.rb @@ -3,17 +3,6 @@ require 'spec_helper' describe Gitlab::Git::RevList do 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) - end def args_for_popen(args_list) [Gitlab.config.git.bin_path, 'rev-list', *args_list] @@ -23,7 +12,7 @@ describe Gitlab::Git::RevList do params = [ args_for_popen(additional_args), repository.path, - command_env, + {}, hash_including(lazy_block: with_lazy_block ? anything : nil) ] diff --git a/spec/lib/gitlab/gitaly_client/util_spec.rb b/spec/lib/gitlab/gitaly_client/util_spec.rb index d1e0136f8c1..550db6db6d9 100644 --- a/spec/lib/gitlab/gitaly_client/util_spec.rb +++ b/spec/lib/gitlab/gitaly_client/util_spec.rb @@ -7,16 +7,19 @@ describe Gitlab::GitalyClient::Util do let(:gl_repository) { 'project-1' } let(:git_object_directory) { '.git/objects' } let(:git_alternate_object_directory) { ['/dir/one', '/dir/two'] } + let(:git_env) do + { + 'GIT_OBJECT_DIRECTORY_RELATIVE' => git_object_directory, + 'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => git_alternate_object_directory + } + end subject do described_class.repository(repository_storage, relative_path, gl_repository) end it 'creates a Gitaly::Repository with the given data' do - allow(Gitlab::Git::Env).to receive(:[]).with('GIT_OBJECT_DIRECTORY_RELATIVE') - .and_return(git_object_directory) - allow(Gitlab::Git::Env).to receive(:[]).with('GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE') - .and_return(git_alternate_object_directory) + allow(Gitlab::Git::HookEnv).to receive(:all).with(gl_repository).and_return(git_env) expect(subject).to be_a(Gitaly::Repository) expect(subject.storage_name).to eq(repository_storage) diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 3cb90a1b8ef..db8c5f963d6 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -251,44 +251,23 @@ describe API::Internal do end context 'with env passed as a JSON' do - context 'when relative path envs are not set' do - it 'sets env in RequestStore' do - expect(Gitlab::Git::Env).to receive(:set).with({ - 'GIT_OBJECT_DIRECTORY' => 'foo', - 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar' - }) - - push(key, project.wiki, env: { - GIT_OBJECT_DIRECTORY: 'foo', - GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar' - }.to_json) + let(:gl_repository) { project.gl_repository(is_wiki: true) } - expect(response).to have_gitlab_http_status(200) - end - end + it 'sets env in RequestStore' do + obj_dir_relative = './objects' + alt_obj_dirs_relative = ['./alt-objects-1', './alt-objects-2'] - context 'when relative path envs are set' do - it 'sets env in RequestStore' do - obj_dir_relative = './objects' - alt_obj_dirs_relative = ['./alt-objects-1', './alt-objects-2'] - repo_path = project.wiki.repository.path_to_repo - - expect(Gitlab::Git::Env).to receive(:set).with({ - 'GIT_OBJECT_DIRECTORY' => File.join(repo_path, obj_dir_relative), - 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => alt_obj_dirs_relative.map { |d| File.join(repo_path, d) }, - 'GIT_OBJECT_DIRECTORY_RELATIVE' => obj_dir_relative, - 'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => alt_obj_dirs_relative - }) - - push(key, project.wiki, env: { - GIT_OBJECT_DIRECTORY: 'foo', - GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar', - GIT_OBJECT_DIRECTORY_RELATIVE: obj_dir_relative, - GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE: alt_obj_dirs_relative - }.to_json) + expect(Gitlab::Git::HookEnv).to receive(:set).with(gl_repository, { + 'GIT_OBJECT_DIRECTORY_RELATIVE' => obj_dir_relative, + 'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => alt_obj_dirs_relative + }) - expect(response).to have_gitlab_http_status(200) - end + push(key, project.wiki, env: { + GIT_OBJECT_DIRECTORY_RELATIVE: obj_dir_relative, + GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE: alt_obj_dirs_relative + }.to_json) + + expect(response).to have_gitlab_http_status(200) end end |