Merge branch 'fix/pass-git-object-dir-attrs-as-relative' into 'master'

Pass git object dir attributes as relative paths to Gitaly

Closes gitaly#629

See merge request gitlab-org/gitlab-ce!14713

GITALY_SERVER_VERSION

-0.45.1
+0.46.0

GITLAB_SHELL_VERSION

-5.9.3
-
+5.9.4

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

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

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
 

spec/lib/gitlab/git/repository_spec.rb

@@ -68,34 +68,55 @@ describe Gitlab::Git::Repository, seed_helper: true do
       expect { broken_repo.rugged }.to raise_error(Gitlab::Git::Repository::NoRepository)
     end
 
+    describe 'alternates keyword argument' do
       context 'with no Git env stored' do
         before do
-        expect(Gitlab::Git::Env).to receive(:all).and_return({})
+          allow(Gitlab::Git::Env).to receive(:all).and_return({})
         end
 
-      it "whitelist some variables and pass them via the alternates keyword argument" do
+        it "is passed an empty array" do
           expect(Rugged::Repository).to receive(:new).with(repository.path, alternates: [])
 
           repository.rugged
         end
       end
 
-    context 'with some Git env stored' do
+      context 'with absolute and relative Git object dir envvars stored' do
         before do
-        expect(Gitlab::Git::Env).to receive(:all).and_return({
+          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
+
+      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',
+            'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar:baz',
             'GIT_OTHER' => 'another_env'
           })
         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])
+        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
 
   describe "#discover_default_branch" do
     let(:master) { 'master' }

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