kotovalexarian-likes-gitlab
/
gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Remove support for absolute dirs from Git::Env

This commit is contained in:
Jacob Vosmaer (GitLab) 2018-03-30 09:19:46 +00:00 committed by Sean McGivern
parent 85418f9ae8
commit b942462788
10 changed files with 69 additions and 156 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
lib/api/helpers/internal_helpers.rb
View File

@ -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

3
lib/api/internal.rb
View File

@ -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]

26
lib/gitlab/git/env.rb → lib/gitlab/git/hook_env.rb
View File

@ -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

14
lib/gitlab/git/repository.rb
View File

@ -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

8
lib/gitlab/gitaly_client/util.rb
View File

@ -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,

71
spec/lib/gitlab/git/env_spec.rb → spec/lib/gitlab/git/hook_env_spec.rb
View File

@ -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')

20
spec/lib/gitlab/git/repository_spec.rb
View File

@ -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

13
spec/lib/gitlab/git/rev_list_spec.rb
View File

@ -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)
]

11
spec/lib/gitlab/gitaly_client/util_spec.rb
View File

@ -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)

47
spec/requests/api/internal_spec.rb
View File

@ -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'
})
let(:gl_repository) { project.gl_repository(is_wiki: true) }
push(key, project.wiki, env: {
GIT_OBJECT_DIRECTORY: 'foo',
GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar'
}.to_json)
it 'sets env in RequestStore' do
obj_dir_relative = './objects'
alt_obj_dirs_relative = ['./alt-objects-1', './alt-objects-2']
expect(response).to have_gitlab_http_status(200)
end
end
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
})
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
push(key, project.wiki, env: {
GIT_OBJECT_DIRECTORY_RELATIVE: obj_dir_relative,
GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE: alt_obj_dirs_relative
}.to_json)
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(response).to have_gitlab_http_status(200)
end
expect(response).to have_gitlab_http_status(200)
end
end