Commit fa1eabe8 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'git-env-repo' into 'master'

Remove support for absolute dirs from Git::Env

See merge request gitlab-org/gitlab-ce!18024
parents 85418f9a b9424627
...@@ -29,18 +29,6 @@ module API ...@@ -29,18 +29,6 @@ module API
{} {}
end 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) def log_user_activity(actor)
commands = Gitlab::GitAccess::DOWNLOAD_COMMANDS commands = Gitlab::GitAccess::DOWNLOAD_COMMANDS
......
...@@ -21,8 +21,7 @@ module API ...@@ -21,8 +21,7 @@ module API
# Stores some Git-specific env thread-safely # Stores some Git-specific env thread-safely
env = parse_env env = parse_env
env = fix_git_env_repository_paths(env, repository_path) if project Gitlab::Git::HookEnv.set(gl_repository, env) if project
Gitlab::Git::Env.set(env)
actor = actor =
if params[:key_id] if params[:key_id]
......
...@@ -3,37 +3,39 @@ ...@@ -3,37 +3,39 @@
module Gitlab module Gitlab
module Git module Git
# Ephemeral (per request) storage for environment variables that some 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 # 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 # $GIT_OBJECT_DIRECTORY. Without it set, the new objects cannot be retrieved
# (this would break push rules for instance). # (this would break push rules for instance).
# #
# This class is thread-safe via RequestStore. # This class is thread-safe via RequestStore.
class Env class HookEnv
WHITELISTED_VARIABLES = %w[ WHITELISTED_VARIABLES = %w[
GIT_OBJECT_DIRECTORY
GIT_OBJECT_DIRECTORY_RELATIVE GIT_OBJECT_DIRECTORY_RELATIVE
GIT_ALTERNATE_OBJECT_DIRECTORIES
GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE
].freeze ].freeze
def self.set(env) def self.set(gl_repository, env)
return unless RequestStore.active? 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 end
def self.all def self.all(gl_repository)
return {} unless RequestStore.active? return {} unless RequestStore.active?
RequestStore.fetch(:gitlab_git_env) { {} } h = RequestStore.fetch(:gitlab_git_env) { {} }
h.fetch(gl_repository, {})
end end
def self.to_env_hash def self.to_env_hash(gl_repository)
env = {} 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) value = value.join(File::PATH_SEPARATOR) if value.is_a?(Array)
env[key.to_s] = value env[key.to_s] = value
end end
...@@ -41,10 +43,6 @@ module Gitlab ...@@ -41,10 +43,6 @@ module Gitlab
env env
end end
def self.[](key)
all[key]
end
def self.whitelist_git_env(env) def self.whitelist_git_env(env)
env.select { |key, _| WHITELISTED_VARIABLES.include?(key.to_s) }.with_indifferent_access env.select { |key, _| WHITELISTED_VARIABLES.include?(key.to_s) }.with_indifferent_access
end end
......
...@@ -1745,21 +1745,11 @@ module Gitlab ...@@ -1745,21 +1745,11 @@ module Gitlab
end end
def alternate_object_directories def alternate_object_directories
relative_paths = relative_object_directories relative_object_directories.map { |d| File.join(path, d) }
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
end end
def relative_object_directories def relative_object_directories
Gitlab::Git::Env.all.values_at(*ALLOWED_OBJECT_RELATIVE_DIRECTORIES_VARIABLES).flatten.compact Gitlab::Git::HookEnv.all(gl_repository).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
end end
# Get the content of a blob for a given commit. If the blob is a commit # Get the content of a blob for a given commit. If the blob is a commit
......
...@@ -3,11 +3,9 @@ module Gitlab ...@@ -3,11 +3,9 @@ module Gitlab
module Util module Util
class << self class << self
def repository(repository_storage, relative_path, gl_repository) def repository(repository_storage, relative_path, gl_repository)
git_object_directory = Gitlab::Git::Env['GIT_OBJECT_DIRECTORY_RELATIVE'].presence || git_env = Gitlab::Git::HookEnv.all(gl_repository)
Gitlab::Git::Env['GIT_OBJECT_DIRECTORY'].presence git_object_directory = git_env['GIT_OBJECT_DIRECTORY_RELATIVE'].presence
git_alternate_object_directories = git_alternate_object_directories = Array.wrap(git_env['GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE'])
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) }
Gitaly::Repository.new( Gitaly::Repository.new(
storage_name: repository_storage, storage_name: repository_storage,
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::Git::Env do describe Gitlab::Git::HookEnv do
let(:gl_repository) { 'project-123' }
describe ".set" do describe ".set" do
context 'with RequestStore.store disabled' do context 'with RequestStore.store disabled' do
before do before do
...@@ -8,9 +10,9 @@ describe Gitlab::Git::Env do ...@@ -8,9 +10,9 @@ describe Gitlab::Git::Env do
end end
it 'does not store anything' do 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
end end
...@@ -21,15 +23,19 @@ describe Gitlab::Git::Env do ...@@ -21,15 +23,19 @@ describe Gitlab::Git::Env do
it 'whitelist some `GIT_*` variables and stores them using RequestStore' do it 'whitelist some `GIT_*` variables and stores them using RequestStore' do
described_class.set( described_class.set(
GIT_OBJECT_DIRECTORY: 'foo', gl_repository,
GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar', GIT_OBJECT_DIRECTORY_RELATIVE: 'foo',
GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE: 'bar',
GIT_EXEC_PATH: 'baz', GIT_EXEC_PATH: 'baz',
PATH: '~/.bin:/bin') PATH: '~/.bin:/bin')
expect(described_class[:GIT_OBJECT_DIRECTORY]).to eq('foo') git_env = described_class.all(gl_repository)
expect(described_class[:GIT_ALTERNATE_OBJECT_DIRECTORIES]).to eq('bar')
expect(described_class[:GIT_EXEC_PATH]).to be_nil expect(git_env[:GIT_OBJECT_DIRECTORY_RELATIVE]).to eq('foo')
expect(described_class[:bar]).to be_nil 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 end
end end
...@@ -39,14 +45,15 @@ describe Gitlab::Git::Env do ...@@ -39,14 +45,15 @@ describe Gitlab::Git::Env do
before do before do
allow(RequestStore).to receive(:active?).and_return(true) allow(RequestStore).to receive(:active?).and_return(true)
described_class.set( described_class.set(
GIT_OBJECT_DIRECTORY: 'foo', gl_repository,
GIT_ALTERNATE_OBJECT_DIRECTORIES: ['bar']) GIT_OBJECT_DIRECTORY_RELATIVE: 'foo',
GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE: ['bar'])
end end
it 'returns an env hash' do it 'returns an env hash' do
expect(described_class.all).to eq({ expect(described_class.all(gl_repository)).to eq({
'GIT_OBJECT_DIRECTORY' => 'foo', 'GIT_OBJECT_DIRECTORY_RELATIVE' => 'foo',
'GIT_ALTERNATE_OBJECT_DIRECTORIES' => ['bar'] 'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => ['bar']
}) })
end end
end end
...@@ -56,8 +63,8 @@ describe Gitlab::Git::Env do ...@@ -56,8 +63,8 @@ describe Gitlab::Git::Env do
context 'with RequestStore.store enabled' do context 'with RequestStore.store enabled' do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
let(:key) { 'GIT_OBJECT_DIRECTORY' } let(:key) { 'GIT_OBJECT_DIRECTORY_RELATIVE' }
subject { described_class.to_env_hash } subject { described_class.to_env_hash(gl_repository) }
where(:input, :output) do where(:input, :output) do
nil | nil nil | nil
...@@ -70,7 +77,7 @@ describe Gitlab::Git::Env do ...@@ -70,7 +77,7 @@ describe Gitlab::Git::Env do
with_them do with_them do
before do before do
allow(RequestStore).to receive(:active?).and_return(true) 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 end
it 'puts the right value in the hash' do it 'puts the right value in the hash' do
...@@ -84,47 +91,25 @@ describe Gitlab::Git::Env do ...@@ -84,47 +91,25 @@ describe Gitlab::Git::Env do
end end
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 describe 'thread-safety' do
context 'with RequestStore.store enabled' do context 'with RequestStore.store enabled' do
before do before do
allow(RequestStore).to receive(:active?).and_return(true) 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 end
it 'is thread-safe' do it 'is thread-safe' do
another_thread = Thread.new 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 Thread.stop
described_class[:GIT_OBJECT_DIRECTORY] described_class.all(gl_repository)[:GIT_OBJECT_DIRECTORY_RELATIVE]
end end
# Ensure another_thread runs first # Ensure another_thread runs first
sleep 0.1 until another_thread.stop? 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 another_thread.run
expect(another_thread.value).to eq('bar') expect(another_thread.value).to eq('bar')
......
...@@ -120,7 +120,7 @@ describe Gitlab::Git::Repository, seed_helper: true do ...@@ -120,7 +120,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
describe 'alternates keyword argument' do describe 'alternates keyword argument' do
context 'with no Git env stored' do context 'with no Git env stored' do
before do before do
allow(Gitlab::Git::Env).to receive(:all).and_return({}) allow(Gitlab::Git::HookEnv).to receive(:all).and_return({})
end end
it "is passed an empty array" do it "is passed an empty array" do
...@@ -132,7 +132,7 @@ describe Gitlab::Git::Repository, seed_helper: true do ...@@ -132,7 +132,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
context 'with absolute and relative Git object dir envvars stored' do context 'with absolute and relative Git object dir envvars stored' do
before 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_OBJECT_DIRECTORY_RELATIVE' => './objects/foo',
'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => ['./objects/bar', './objects/baz'], 'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => ['./objects/bar', './objects/baz'],
'GIT_OBJECT_DIRECTORY' => 'ignored', 'GIT_OBJECT_DIRECTORY' => 'ignored',
...@@ -148,22 +148,6 @@ describe Gitlab::Git::Repository, seed_helper: true do ...@@ -148,22 +148,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
repository.rugged repository.rugged
end end
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
end end
......
...@@ -3,17 +3,6 @@ require 'spec_helper' ...@@ -3,17 +3,6 @@ require 'spec_helper'
describe Gitlab::Git::RevList do describe Gitlab::Git::RevList do
let(:repository) { create(:project, :repository).repository.raw } let(:repository) { create(:project, :repository).repository.raw }
let(:rev_list) { described_class.new(repository, newrev: 'newrev') } 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) def args_for_popen(args_list)
[Gitlab.config.git.bin_path, 'rev-list', *args_list] [Gitlab.config.git.bin_path, 'rev-list', *args_list]
...@@ -23,7 +12,7 @@ describe Gitlab::Git::RevList do ...@@ -23,7 +12,7 @@ describe Gitlab::Git::RevList do
params = [ params = [
args_for_popen(additional_args), args_for_popen(additional_args),
repository.path, repository.path,
command_env, {},
hash_including(lazy_block: with_lazy_block ? anything : nil) hash_including(lazy_block: with_lazy_block ? anything : nil)
] ]
......
...@@ -7,16 +7,19 @@ describe Gitlab::GitalyClient::Util do ...@@ -7,16 +7,19 @@ describe Gitlab::GitalyClient::Util do
let(:gl_repository) { 'project-1' } let(:gl_repository) { 'project-1' }
let(:git_object_directory) { '.git/objects' } let(:git_object_directory) { '.git/objects' }
let(:git_alternate_object_directory) { ['/dir/one', '/dir/two'] } 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 subject do
described_class.repository(repository_storage, relative_path, gl_repository) described_class.repository(repository_storage, relative_path, gl_repository)
end end
it 'creates a Gitaly::Repository with the given data' do it 'creates a Gitaly::Repository with the given data' do
allow(Gitlab::Git::Env).to receive(:[]).with('GIT_OBJECT_DIRECTORY_RELATIVE') allow(Gitlab::Git::HookEnv).to receive(:all).with(gl_repository).and_return(git_env)
.and_return(git_object_directory)
allow(Gitlab::Git::Env).to receive(:[]).with('GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE')
.and_return(git_alternate_object_directory)
expect(subject).to be_a(Gitaly::Repository) expect(subject).to be_a(Gitaly::Repository)
expect(subject.storage_name).to eq(repository_storage) expect(subject.storage_name).to eq(repository_storage)
......
...@@ -251,38 +251,18 @@ describe API::Internal do ...@@ -251,38 +251,18 @@ describe API::Internal do
end end
context 'with env passed as a JSON' do context 'with env passed as a JSON' do
context 'when relative path envs are not set' do let(:gl_repository) { project.gl_repository(is_wiki: true) }
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)
expect(response).to have_gitlab_http_status(200)
end
end
context 'when relative path envs are set' do
it 'sets env in RequestStore' do it 'sets env in RequestStore' do
obj_dir_relative = './objects' obj_dir_relative = './objects'
alt_obj_dirs_relative = ['./alt-objects-1', './alt-objects-2'] 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({ expect(Gitlab::Git::HookEnv).to receive(:set).with(gl_repository, {
'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_OBJECT_DIRECTORY_RELATIVE' => obj_dir_relative,
'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => alt_obj_dirs_relative 'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => alt_obj_dirs_relative
}) })
push(key, project.wiki, env: { push(key, project.wiki, env: {
GIT_OBJECT_DIRECTORY: 'foo',
GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar',
GIT_OBJECT_DIRECTORY_RELATIVE: obj_dir_relative, GIT_OBJECT_DIRECTORY_RELATIVE: obj_dir_relative,
GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE: alt_obj_dirs_relative GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE: alt_obj_dirs_relative
}.to_json) }.to_json)
...@@ -290,7 +270,6 @@ describe API::Internal do ...@@ -290,7 +270,6 @@ describe API::Internal do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
end end
end end
end
context "git push with project.wiki" do context "git push with project.wiki" do
it 'responds with success' do it 'responds with success' do
......
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