Merge branch '1736-fix-push-rules-git-2-11' into 'master'

Fix push rules on Git 2.11

Closes #1736

See merge request !1525

app/models/ee/repository.rb

@@ -12,5 +12,14 @@ module EE
       expire_branch_cache
       expire_content_cache
     end
+
+    # Returns a list of commits that are not present in any reference
+    def new_commits(newrev)
+      refs = ::Gitlab::Git::RevList.new(
+        path_to_repo: path_to_repo,
+        newrev: newrev).new_refs
+
+      refs.map { |sha| commit(sha.strip) }
+    end
   end
 end

app/models/repository.rb

@@ -634,15 +634,6 @@ class Repository
     commit(sha)
   end
 
-  # Returns a list of commits that are not present in any reference
-  def new_commits(newrev)
-    args = %W(#{Gitlab.config.git.bin_path} rev-list #{newrev} --not --all)
-
-    Gitlab::Popen.popen(args, path_to_repo).first.split("\n").map do |sha|
-      commit(sha.strip)
-    end
-  end
-
   def last_commit_id_for_path(sha, path)
     key = path.blank? ? "last_commit_id_for_path:#{sha}" : "last_commit_id_for_path:#{sha}:#{Digest::SHA1.hexdigest(path)}"
 

changelogs/unreleased-ee/1736-fix-push-rules-git-2-11.yml

+---
+title: Fix pre-receive hooks when using Git 2.11 or later.
+merge_request: 1525
+author:

lib/api/helpers/internal_helpers.rb

@@ -59,12 +59,12 @@ module API
         ::Users::ActivityService.new(actor, 'Git SSH').execute if commands.include?(params[:action])
       end
 
-      def parse_allowed_environment_variables
-        return if params[:env].blank?
+      def parse_env
+        return {} if params[:env].blank?
 
         JSON.parse(params[:env])
-
       rescue JSON::ParserError
+        {}
       end
     end
   end

lib/api/internal.rb

@@ -11,14 +11,16 @@ module API
       # Params:
       #   key_id - ssh key id for Git over SSH
       #   user_id - user id for Git over HTTP
+      #   protocol - Git access protocol being used, e.g. HTTP or SSH
       #   project - project path with namespace
       #   action - git action (git-upload-pack or git-receive-pack)
-      #   ref - branch name
-      #   forced_push - forced_push
-      #   protocol - Git access protocol being used, e.g. HTTP or SSH
+      #   changes - changes as "oldrev newrev ref", see Gitlab::ChangesList
       post "/allowed" do
         status 200
 
+        # Stores some Git-specific env thread-safely
+        Gitlab::Git::Env.set(parse_env)
+
         actor =
           if params[:key_id]
             Key.find_by(id: params[:key_id])
@@ -30,18 +32,10 @@ module API
 
         actor.update_last_used_at if actor.is_a?(Key)
 
-        access =
-          if wiki?
-            Gitlab::GitAccessWiki.new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
-          else
-            Gitlab::GitAccess.new(actor,
-                                  project,
-                                  protocol,
-                                  authentication_abilities: ssh_authentication_abilities,
-                                  env: parse_allowed_environment_variables)
-          end
-
-        access_status = access.check(params[:action], params[:changes])
+        access_checker = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess
+        access_status = access_checker
+          .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
+          .check(params[:action], params[:changes])
 
         response = { status: access_status.status, message: access_status.message }
 

lib/gitlab/checks/change_access.rb

@@ -7,7 +7,7 @@ module Gitlab
       attr_reader :user_access, :project, :skip_authorization, :protocol
 
       def initialize(
-        change, user_access:, project:, env: {}, skip_authorization: false,
+        change, user_access:, project:, skip_authorization: false,
         protocol:
       )
         @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@@ -15,7 +15,6 @@ module Gitlab
         @tag_name = Gitlab::Git.tag_name(@ref)
         @user_access = user_access
         @project = project
-        @env = env
         @skip_authorization = skip_authorization
         @protocol = protocol
       end
@@ -99,7 +98,7 @@ module Gitlab
       end
 
       def forced_push?
-        Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev, env: @env)
+        Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev)
       end
 
       def update?

lib/gitlab/checks/force_push.rb

 module Gitlab
   module Checks
     class ForcePush
-      def self.force_push?(project, oldrev, newrev, env: {})
+      def self.force_push?(project, oldrev, newrev)
         return false if project.empty_repo?
 
         # Created or deleted branch
         if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
           false
         else
-          missed_ref, exit_status = Gitlab::Git::RevList.new(oldrev, newrev, project: project, env: env).execute
-
-          if exit_status == 0
-            missed_ref.present?
-          else
-            raise "Got a non-zero exit code while calling out to `git rev-list` in the force-push check."
-          end
+          Gitlab::Git::RevList.new(
+            path_to_repo: project.repository.path_to_repo,
+            oldrev: oldrev, newrev: newrev).missed_ref.present?
         end
       end
     end

lib/gitlab/git/env.rb

+module Gitlab
+  module Git
+    # Ephemeral (per request) storage for environment variables that some Git
+    # commands may need.
+    #
+    # 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
+      WHITELISTED_GIT_VARIABLES = %w[
+        GIT_OBJECT_DIRECTORY
+        GIT_ALTERNATE_OBJECT_DIRECTORIES
+      ].freeze
+
+      def self.set(env)
+        return unless RequestStore.active?
+
+        RequestStore.store[:gitlab_git_env] = whitelist_git_env(env)
+      end
+
+      def self.all
+        return {} unless RequestStore.active?
+
+        RequestStore.fetch(:gitlab_git_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
+      end
+    end
+  end
+end

lib/gitlab/git/repository.rb

@@ -8,6 +8,10 @@ module Gitlab
     class Repository
       include Gitlab::Git::Popen
 
+      ALLOWED_OBJECT_DIRECTORIES_VARIABLES = %w[
+        GIT_OBJECT_DIRECTORY
+        GIT_ALTERNATE_OBJECT_DIRECTORIES
+      ].freeze
       SEARCH_CONTEXT_LINES = 3
 
       NoRepository = Class.new(StandardError)
@@ -58,7 +62,7 @@ module Gitlab
       end
 
       def rugged
-        @rugged ||= Rugged::Repository.new(path)
+        @rugged ||= Rugged::Repository.new(path, alternates: alternate_object_directories)
       rescue Rugged::RepositoryError, Rugged::OSError
         raise NoRepository.new('no repository for such path')
       end
@@ -955,6 +959,10 @@ module Gitlab
 
       private
 
+      def alternate_object_directories
+        Gitlab::Git::Env.all.values_at(*ALLOWED_OBJECT_DIRECTORIES_VARIABLES).compact
+      end
+
       # Get the content of a blob for a given commit.  If the blob is a commit
       # (for submodules) then return the blob's OID.
       def blob_content(commit, blob_name)

lib/gitlab/git/rev_list.rb

 module Gitlab
   module Git
     class RevList
-      attr_reader :project, :env
-
-      ALLOWED_VARIABLES = %w[GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES].freeze
-
-      def initialize(oldrev, newrev, project:, env: nil)
-        @project = project
-        @env = env.presence || {}
-        @args = [Gitlab.config.git.bin_path,
-                 "--git-dir=#{project.repository.path_to_repo}",
-                 "rev-list",
-                 "--max-count=1",
-                 oldrev,
-                 "^#{newrev}"]
+      attr_reader :oldrev, :newrev, :path_to_repo
+
+      def initialize(path_to_repo:, newrev:, oldrev: nil)
+        @oldrev = oldrev
+        @newrev = newrev
+        @path_to_repo = path_to_repo
       end
 
-      def execute
-        Gitlab::Popen.popen(@args, nil, parse_environment_variables)
+      # This method returns an array of new references
+      def new_refs
+        execute([*base_args, newrev, '--not', '--all'])
       end
 
-      def valid?
-        environment_variables.all? do |(name, value)|
-          value.to_s.start_with?(project.repository.path_to_repo)
-        end
+      # This methods returns an array of missed references
+      def missed_ref
+        execute([*base_args, '--max-count=1', oldrev, "^#{newrev}"])
       end
 
       private
 
-      def parse_environment_variables
-        return {} unless valid?
+      def execute(args)
+        output, status = Gitlab::Popen.popen(args, nil, Gitlab::Git::Env.all.stringify_keys)
+
+        unless status.zero?
+          raise "Got a non-zero exit code while calling out `#{args.join(' ')}`."
+        end
 
-        environment_variables
+        output.split("\n")
       end
 
-      def environment_variables
-        @environment_variables ||= env.slice(*ALLOWED_VARIABLES).compact
+      def base_args
+        [
+          Gitlab.config.git.bin_path,
+          "--git-dir=#{path_to_repo}",
+          'rev-list'
+        ]
       end
     end
   end

lib/gitlab/git_access.rb

@@ -20,13 +20,12 @@ module Gitlab
 
     attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities
 
-    def initialize(actor, project, protocol, authentication_abilities:, env: {})
+    def initialize(actor, project, protocol, authentication_abilities:)
       @actor    = actor
       @project  = project
       @protocol = protocol
       @authentication_abilities = authentication_abilities
       @user_access = UserAccess.new(user, project: project)
-      @env = env
     end
 
     def check(cmd, changes)
@@ -191,7 +190,6 @@ module Gitlab
         change,
         user_access: user_access,
         project: project,
-        env: @env,
         skip_authorization: deploy_key?,
         protocol: protocol
       ).exec

spec/lib/gitlab/git/env_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Git::Env do
+  describe "#set" do
+    context 'with RequestStore.store disabled' do
+      before do
+        allow(RequestStore).to receive(:active?).and_return(false)
+      end
+
+      it 'does not store anything' do
+        described_class.set(GIT_OBJECT_DIRECTORY: 'foo')
+
+        expect(described_class.all).to be_empty
+      end
+    end
+
+    context 'with RequestStore.store enabled' do
+      before do
+        allow(RequestStore).to receive(:active?).and_return(true)
+      end
+
+      it 'whitelist some `GIT_*` variables and stores them using RequestStore' do
+        described_class.set(
+          GIT_OBJECT_DIRECTORY: 'foo',
+          GIT_ALTERNATE_OBJECT_DIRECTORIES: '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
+      end
+    end
+  end
+
+  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')
+      end
+
+      it 'returns an env hash' do
+        expect(described_class.all).to eq({
+          'GIT_OBJECT_DIRECTORY' => 'foo',
+          'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar'
+        })
+      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
+    context 'with RequestStore.store enabled' do
+      before do
+        allow(RequestStore).to receive(:active?).and_return(true)
+        described_class.set(GIT_OBJECT_DIRECTORY: 'foo')
+      end
+
+      it 'is thread-safe' do
+        another_thread = Thread.new do
+          described_class.set(GIT_OBJECT_DIRECTORY: 'bar')
+
+          Thread.stop
+          described_class[:GIT_OBJECT_DIRECTORY]
+        end
+
+        # Ensure another_thread runs first
+        sleep 0.1 until another_thread.stop?
+
+        expect(described_class[:GIT_OBJECT_DIRECTORY]).to eq('foo')
+
+        another_thread.run
+        expect(another_thread.value).to eq('bar')
+      end
+    end
+  end
+end

spec/lib/gitlab/git/repository_spec.rb

@@ -40,6 +40,36 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
+  describe "#rugged" do
+    context 'with no Git env stored' do
+      before do
+        expect(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: [])
+
+        repository.rugged
+      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'
+        })
+      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])
+
+        repository.rugged
+      end
+    end
+  end
+
   describe "#discover_default_branch" do
     let(:master) { 'master' }
     let(:feature) { 'feature' }

spec/lib/gitlab/git/rev_list_spec.rb

@@ -3,58 +3,54 @@ require 'spec_helper'
 describe Gitlab::Git::RevList, lib: true do
   let(:project) { create(:project, :repository) }
 
-  context "validations" do
-    described_class::ALLOWED_VARIABLES.each do |var|
-      context var do
-        it "accepts values starting with the project repo path" do
-          env = { var => "#{project.repository.path_to_repo}/objects" }
-          rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
-
-          expect(rev_list).to be_valid
-        end
-
-        it "rejects values starting not with the project repo path" do
-          env = { var => "/some/other/path" }
-          rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
-
-          expect(rev_list).not_to be_valid
-        end
-
-        it "rejects values containing the project repo path but not starting with it" do
-          env = { var => "/some/other/path/#{project.repository.path_to_repo}" }
-          rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
-
-          expect(rev_list).not_to be_valid
-        end
-
-        it "ignores nil values" do
-          env = { var => nil }
-          rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
-
-          expect(rev_list).to be_valid
-        end
-      end
-    end
+  before do
+    expect(Gitlab::Git::Env).to receive(:all).and_return({
+      GIT_OBJECT_DIRECTORY: 'foo',
+      GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar'
+    })
   end
 
-  context "#execute" do
-    let(:env) { { "GIT_OBJECT_DIRECTORY" => project.repository.path_to_repo } }
-    let(:rev_list) { Gitlab::Git::RevList.new('oldrev', 'newrev', project: project, env: env) }
-
-    it "calls out to `popen` without environment variables if the record is invalid" do
-      allow(rev_list).to receive(:valid?).and_return(false)
-
-      expect(Open3).to receive(:popen3).with(hash_excluding(env), any_args)
-
-      rev_list.execute
+  context "#new_refs" do
+    let(:rev_list) { Gitlab::Git::RevList.new(newrev: 'newrev', path_to_repo: project.repository.path_to_repo) }
+
+    it 'calls out to `popen`' do
+      expect(Gitlab::Popen).to receive(:popen).with([
+        Gitlab.config.git.bin_path,
+        "--git-dir=#{project.repository.path_to_repo}",
+        'rev-list',
+        'newrev',
+        '--not',
+        '--all'
+      ],
+      nil,
+      {
+        'GIT_OBJECT_DIRECTORY' => 'foo',
+        'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar'
+      }).and_return(["sha1\nsha2", 0])
+
+      expect(rev_list.new_refs).to eq(%w[sha1 sha2])
     end
+  end
 
-    it "calls out to `popen` with environment variables if the record is valid" do
-      allow(rev_list).to receive(:valid?).and_return(true)
-
-      expect(Open3).to receive(:popen3).with(hash_including(env), any_args)
-
-      rev_list.execute
+  context "#missed_ref" do
+    let(:rev_list) { Gitlab::Git::RevList.new(oldrev: 'oldrev', newrev: 'newrev', path_to_repo: project.repository.path_to_repo) }
+
+    it 'calls out to `popen`' do
+      expect(Gitlab::Popen).to receive(:popen).with([
+        Gitlab.config.git.bin_path,
+        "--git-dir=#{project.repository.path_to_repo}",
+        'rev-list',
+        '--max-count=1',
+        'oldrev',
+        '^newrev'
+      ],
+      nil,
+      {
+        'GIT_OBJECT_DIRECTORY' => 'foo',
+        'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar'
+      }).and_return(["sha1\nsha2", 0])
+
+      expect(rev_list.missed_ref).to eq(%w[sha1 sha2])
     end
   end
 end

spec/lib/gitlab/git_access_spec.rb

@@ -527,7 +527,7 @@ describe Gitlab::GitAccess, lib: true do
       before do
         project.team << [user, :developer]
 
-        allow_any_instance_of(Repository).to receive(:new_commits).and_return(
+        allow(project.repository).to receive(:new_commits).and_return(
           project.repository.commits_between('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9', '570e7b2abdd848b95f2f578043fc23bd6f6fd24d')
         )
       end
@@ -568,7 +568,7 @@ describe Gitlab::GitAccess, lib: true do
           bad_commit = double("Commit", safe_message: 'Some change').as_null_object
           ref_object = double(name: 'heads/master')
           allow(bad_commit).to receive(:refs).and_return([ref_object])
-          allow_any_instance_of(Repository).to receive(:commits_between).and_return([bad_commit])
+          allow(project.repository).to receive(:commits_between).and_return([bad_commit])
 
           project.create_push_rule
           project.push_rule.update(commit_message_regex: "Change some files")
@@ -582,8 +582,8 @@ describe Gitlab::GitAccess, lib: true do
           # We use tmp ref a a temporary for Web UI commiting
           ref_object = double(name: 'refs/tmp')
           allow(bad_commit).to receive(:refs).and_return([ref_object])
-          allow_any_instance_of(Repository).to receive(:commits_between).and_return([bad_commit])
-          allow_any_instance_of(Repository).to receive(:new_commits).and_return([bad_commit])
+          allow(project.repository).to receive(:commits_between).and_return([bad_commit])
+          allow(project.repository).to receive(:new_commits).and_return([bad_commit])
 
           project.create_push_rule
           project.push_rule.update(commit_message_regex: "Change some files")
@@ -612,7 +612,7 @@ describe Gitlab::GitAccess, lib: true do
 
       describe "file names check" do
         before do
-          allow_any_instance_of(Repository).to receive(:new_commits).and_return(
+          allow(project.repository).to receive(:new_commits).and_return(
             project.repository.commits_between('913c66a37b4a45b9769037c55c2d238bd0942d2e', '33f3729a45c02fc67d00adb1b8bca394b0e761d9')
           )
         end

spec/models/ee/repository_spec.rb

+require 'spec_helper'
+
+describe EE::Repository, models: true do
+  let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
+
+  describe '#new_commits' do
+    let(:new_refs) do
+      double(:git_rev_list, new_refs: %w[
+        c1acaa58bbcbc3eafe538cb8274ba387047b69f8
+        5937ac0a7beb003549fc5fd26fc247adbce4a52e
+      ])
+    end
+
+    it 'delegates to Gitlab::Git::RevList' do
+      expect(Gitlab::Git::RevList).to receive(:new).with(
+        path_to_repo: repository.path_to_repo,
+        newrev: 'aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj').and_return(new_refs)
+
+      commits = repository.new_commits('aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj')
+
+      expect(commits).to eq([
+        repository.commit('c1acaa58bbcbc3eafe538cb8274ba387047b69f8'),
+        repository.commit('5937ac0a7beb003549fc5fd26fc247adbce4a52e')
+      ])
+    end
+  end
+end

spec/requests/api/internal_spec.rb

@@ -206,6 +206,22 @@ describe API::Internal, api: true  do
         Timecop.return
       end
 
+      context 'with env passed as a JSON' do
+        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_http_status(200)
+        end
+      end
+
       context "git push with project.wiki" do
         it 'responds with success' do
           push(key, project.wiki)
@@ -524,7 +540,7 @@ describe API::Internal, api: true  do
     )
   end
 
-  def push(key, project, protocol = 'ssh')
+  def push(key, project, protocol = 'ssh', env: nil)
     post(
       api("/internal/allowed"),
       changes: 'd14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master',
@@ -532,7 +548,8 @@ describe API::Internal, api: true  do
       project: project.repository.path_to_repo,
       action: 'git-receive-pack',
       secret_token: secret_token,
-      protocol: protocol
+      protocol: protocol,
+      env: env
     )
   end