Merge branch 'port-git-alt-object-dirs-from-ce' into 'master'

Port Git alternate dirs fix to EE

See merge request gitlab-org/gitlab-ee!3157

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)
@@ -28,12 +30,23 @@ module Gitlab
         RequestStore.fetch(:gitlab_git_env) { {} }
       end
 
+      def self.to_env_hash
+        env = {}
+
+        all.compact.each do |key, value|
+          value = value.join(File::PATH_SEPARATOR) if value.is_a?(Array)
+          env[key.to_s] = value
+        end
+
+        env
+      end
+
       def self.[](key)
         all[key]
       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)
@@ -1251,7 +1255,16 @@ 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)
+            .flatten
+            .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/git/rev_list.rb

@@ -28,7 +28,7 @@ module Gitlab
       private
 
       def execute(args)
-        output, status = popen(args, nil, Gitlab::Git::Env.all.stringify_keys)
+        output, status = popen(args, nil, Gitlab::Git::Env.to_env_hash)
 
         unless status.zero?
           raise "Got a non-zero exit code while calling out `#{args.join(' ')}`: #{output}"

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/env_spec.rb

 require 'spec_helper'
 
 describe Gitlab::Git::Env do
-  describe "#set" do
+  describe ".set" do
     context 'with RequestStore.store disabled' do
       before do
         allow(RequestStore).to receive(:active?).and_return(false)
@@ -34,25 +34,57 @@ describe Gitlab::Git::Env do
     end
   end
 
-  describe "#all" do
+  describe ".all" do
     context 'with RequestStore.store enabled' do
       before do
         allow(RequestStore).to receive(:active?).and_return(true)
         described_class.set(
           GIT_OBJECT_DIRECTORY: 'foo',
-          GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar')
+          GIT_ALTERNATE_OBJECT_DIRECTORIES: ['bar'])
       end
 
       it 'returns an env hash' do
         expect(described_class.all).to eq({
           'GIT_OBJECT_DIRECTORY' => 'foo',
-          'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar'
+          'GIT_ALTERNATE_OBJECT_DIRECTORIES' => ['bar']
         })
       end
     end
   end
 
-  describe "#[]" do
+  describe ".to_env_hash" do
+    context 'with RequestStore.store enabled' do
+      using RSpec::Parameterized::TableSyntax
+
+      let(:key) { 'GIT_OBJECT_DIRECTORY' }
+      subject { described_class.to_env_hash }
+
+      where(:input, :output) do
+        nil         | nil
+        'foo'       | 'foo'
+        []          | ''
+        ['foo']     | 'foo'
+        %w[foo bar] | 'foo:bar'
+      end
+
+      with_them do
+        before do
+          allow(RequestStore).to receive(:active?).and_return(true)
+          described_class.set(key.to_sym => input)
+        end
+
+        it 'puts the right value in the hash' do
+          if output
+            expect(subject.fetch(key)).to eq(output)
+          else
+            expect(subject.has_key?(key)).to eq(false)
+          end
+        end
+      end
+    end
+  end
+
+  describe ".[]" do
     context 'with RequestStore.store enabled' do
       before do
         allow(RequestStore).to receive(:active?).and_return(true)

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' => %w[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' => %w[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