From 4378f56c7b1f6f6031c284ff8879cdc4ad79468f Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Thu, 5 Oct 2017 16:51:31 +0200 Subject: Pass git object dir attributes as relative paths to Gitaly Fixes gitaly#629 --- GITALY_SERVER_VERSION | 2 +- GITLAB_SHELL_VERSION | 3 +- lib/gitlab/git/env.rb | 6 ++-- lib/gitlab/git/repository.rb | 14 +++++++- lib/gitlab/gitaly_client/util.rb | 10 ++++-- spec/lib/gitlab/git/repository_spec.rb | 57 ++++++++++++++++++++---------- spec/lib/gitlab/gitaly_client/util_spec.rb | 8 ++--- 7 files changed, 70 insertions(+), 30 deletions(-) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index ff12df2fbb0..301092317fe 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.45.1 +0.46.0 diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index 5508e17c23b..c5b7013b9c5 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1,2 +1 @@ -5.9.3 - +5.9.4 diff --git a/lib/gitlab/git/env.rb b/lib/gitlab/git/env.rb index f80193ac553..80f0731cd99 100644 --- a/lib/gitlab/git/env.rb +++ b/lib/gitlab/git/env.rb @@ -11,9 +11,11 @@ module Gitlab # # This class is thread-safe via RequestStore. class Env - WHITELISTED_GIT_VARIABLES = %w[ + WHITELISTED_VARIABLES = %w[ GIT_OBJECT_DIRECTORY + GIT_OBJECT_DIRECTORY_RELATIVE GIT_ALTERNATE_OBJECT_DIRECTORIES + GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE ].freeze def self.set(env) @@ -33,7 +35,7 @@ module Gitlab end def self.whitelist_git_env(env) - env.select { |key, _| WHITELISTED_GIT_VARIABLES.include?(key.to_s) }.with_indifferent_access + env.select { |key, _| WHITELISTED_VARIABLES.include?(key.to_s) }.with_indifferent_access end end end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index fa13920a3f3..a082cfed706 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -12,6 +12,10 @@ module Gitlab GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES ].freeze + ALLOWED_OBJECT_RELATIVE_DIRECTORIES_VARIABLES = %w[ + GIT_OBJECT_DIRECTORY_RELATIVE + GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE + ].freeze SEARCH_CONTEXT_LINES = 3 NoRepository = Class.new(StandardError) @@ -1220,7 +1224,15 @@ module Gitlab end def alternate_object_directories - Gitlab::Git::Env.all.values_at(*ALLOWED_OBJECT_DIRECTORIES_VARIABLES).compact + relative_paths = Gitlab::Git::Env.all.values_at(*ALLOWED_OBJECT_RELATIVE_DIRECTORIES_VARIABLES).flatten.compact + + if relative_paths.any? + relative_paths.map { |d| File.join(path, d) } + else + Gitlab::Git::Env.all.values_at(*ALLOWED_OBJECT_DIRECTORIES_VARIABLES) + .compact + .flat_map { |d| d.split(File::PATH_SEPARATOR) } + end 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 da43c616b94..a1222a7e718 100644 --- a/lib/gitlab/gitaly_client/util.rb +++ b/lib/gitlab/gitaly_client/util.rb @@ -3,12 +3,18 @@ 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) } + Gitaly::Repository.new( storage_name: repository_storage, relative_path: relative_path, gl_repository: gl_repository, - git_object_directory: Gitlab::Git::Env['GIT_OBJECT_DIRECTORY'].to_s, - git_alternate_object_directories: Array.wrap(Gitlab::Git::Env['GIT_ALTERNATE_OBJECT_DIRECTORIES']) + git_object_directory: git_object_directory.to_s, + git_alternate_object_directories: git_alternate_object_directories ) end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 1ee4acfd193..d959562c951 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -68,31 +68,52 @@ describe Gitlab::Git::Repository, seed_helper: true do expect { broken_repo.rugged }.to raise_error(Gitlab::Git::Repository::NoRepository) end - context 'with no Git env stored' do - before do - expect(Gitlab::Git::Env).to receive(:all).and_return({}) - end + describe 'alternates keyword argument' do + context 'with no Git env stored' do + before do + allow(Gitlab::Git::Env).to receive(:all).and_return({}) + end - it "whitelist some variables and pass them via the alternates keyword argument" do - expect(Rugged::Repository).to receive(:new).with(repository.path, alternates: []) + it "is passed an empty array" do + expect(Rugged::Repository).to receive(:new).with(repository.path, alternates: []) - repository.rugged + repository.rugged + end end - end - context 'with some Git env stored' do - before do - expect(Gitlab::Git::Env).to receive(:all).and_return({ - 'GIT_OBJECT_DIRECTORY' => 'foo', - 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar', - 'GIT_OTHER' => 'another_env' - }) + context 'with absolute and relative Git object dir envvars stored' do + before do + allow(Gitlab::Git::Env).to receive(:all).and_return({ + 'GIT_OBJECT_DIRECTORY_RELATIVE' => './objects/foo', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE' => ['./objects/bar', './objects/baz'], + 'GIT_OBJECT_DIRECTORY' => 'ignored', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'ignored:ignored', + 'GIT_OTHER' => 'another_env' + }) + end + + it "is passed the relative object dir envvars after being converted to absolute ones" do + alternates = %w[foo bar baz].map { |d| File.join(repository.path, './objects', d) } + expect(Rugged::Repository).to receive(:new).with(repository.path, alternates: alternates) + + repository.rugged + end end - it "whitelist some variables and pass them via the alternates keyword argument" do - expect(Rugged::Repository).to receive(:new).with(repository.path, alternates: %w[foo bar]) + 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' => 'bar:baz', + 'GIT_OTHER' => 'another_env' + }) + end - repository.rugged + 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/gitaly_client/util_spec.rb b/spec/lib/gitlab/gitaly_client/util_spec.rb index 498f6886bee..c0c29552494 100644 --- a/spec/lib/gitlab/gitaly_client/util_spec.rb +++ b/spec/lib/gitlab/gitaly_client/util_spec.rb @@ -6,16 +6,16 @@ describe Gitlab::GitalyClient::Util do let(:relative_path) { 'my/repo.git' } let(:gl_repository) { 'project-1' } let(:git_object_directory) { '.git/objects' } - let(:git_alternate_object_directory) { '/dir/one:/dir/two' } + let(:git_alternate_object_directory) { ['/dir/one', '/dir/two'] } subject do described_class.repository(repository_storage, relative_path, gl_repository) end it 'creates a Gitaly::Repository with the given data' do - expect(Gitlab::Git::Env).to receive(:[]).with('GIT_OBJECT_DIRECTORY') + allow(Gitlab::Git::Env).to receive(:[]).with('GIT_OBJECT_DIRECTORY_RELATIVE') .and_return(git_object_directory) - expect(Gitlab::Git::Env).to receive(:[]).with('GIT_ALTERNATE_OBJECT_DIRECTORIES') + 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) @@ -23,7 +23,7 @@ describe Gitlab::GitalyClient::Util do expect(subject.relative_path).to eq(relative_path) expect(subject.gl_repository).to eq(gl_repository) expect(subject.git_object_directory).to eq(git_object_directory) - expect(subject.git_alternate_object_directories).to eq([git_alternate_object_directory]) + expect(subject.git_alternate_object_directories).to eq(git_alternate_object_directory) end end -- cgit v1.2.1