Merge branch 'fix_push_rules' into 'master'

[Fix] Push rules check existing commits in some cases

Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/815

See merge request !640

CHANGELOG-EE

@@ -10,6 +10,7 @@ v 8.11.0 (unreleased)
   - Enable monitoring for ES classes
   - [Elastic] Improve code search
   - [Elastic] Significant improvement of global search performance
+  - [Fix] Push rules check existing commits in some cases
 
 v 8.10.5
   - Used cached value of project count in `Elastic::RepositoriesSearch` to reduce DB load. !637

app/models/repository.rb

@@ -666,6 +666,15 @@ 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 next_branch(name, opts = {})
     branch_ids = self.branch_names.map do |n|
       next 1 if n == name

lib/gitlab/checks/change_access.rb

@@ -119,11 +119,8 @@ module Gitlab
           # if newrev is blank, the branch was deleted
           return if Gitlab::Git.blank_ref?(@newrev) || !push_rule.commit_validation?
 
-          # if oldrev is blank, the branch was just created
-          oldrev = Gitlab::Git.blank_ref?(@oldrev) ? project.default_branch : @oldrev
-
-          commits(oldrev).each do |commit|
-            next if commit_from_annex_sync?(commit.safe_message) || old_commit?(commit)
+          commits.each do |commit|
+            next if commit_from_annex_sync?(commit.safe_message)
 
             if error = check_commit(commit, push_rule)
               return error
@@ -140,7 +137,7 @@ module Gitlab
         # locks protect default branch only
         return if project.default_branch != branch_name(@ref)
 
-        commits(@oldrev).each do |commit|
+        commits.each do |commit|
           next if commit_from_annex_sync?(commit.safe_message)
 
           commit.raw_diffs.each do |diff|
@@ -216,12 +213,8 @@ module Gitlab
         nil
       end
 
-      def commits(oldrev)
-        if oldrev
-          project.repository.commits_between(oldrev, @newrev)
-        else
-          project.repository.commits(@newrev)
-        end
+      def commits
+        project.repository.new_commits(@newrev)
       end
 
       def commit_from_annex_sync?(commit_message)
@@ -230,11 +223,6 @@ module Gitlab
         # Commit message starting with <git-annex in > so avoid push rules on this
         commit_message.start_with?('git-annex in')
       end
-
-      def old_commit?(commit)
-        # We skip refs/tmp ref because we use it for Web UI commiting
-        commit.refs(project.repository).reject { |ref| ref.name.start_with?('refs/tmp') }.any?
-      end
     end
   end
 end

spec/lib/gitlab/git_access_spec.rb

@@ -324,7 +324,13 @@ describe Gitlab::GitAccess, lib: true do
     end
 
     context "when using git annex" do
-      before { project.team << [user, :master] }
+      before do
+        project.team << [user, :master]
+
+        allow_any_instance_of(Repository).to receive(:new_commits).and_return(
+          project.repository.commits_between('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9', '570e7b2abdd848b95f2f578043fc23bd6f6fd24d')
+        )
+      end
 
       describe 'and gitlab geo is enabled in a secondary node' do
         before do
@@ -374,7 +380,9 @@ describe Gitlab::GitAccess, lib: true do
           end
 
           describe 'git annex disabled' do
-            before { allow(Gitlab.config.gitlab_shell).to receive(:git_annex_enabled).and_return(false) }
+            before do
+              allow(Gitlab.config.gitlab_shell).to receive(:git_annex_enabled).and_return(false)
+            end
 
             it { expect(access.push_access_check(git_annex_changes)).not_to be_allowed }
           end
@@ -394,7 +402,9 @@ describe Gitlab::GitAccess, lib: true do
           end
 
           describe 'git annex disabled' do
-            before { allow(Gitlab.config.gitlab_shell).to receive(:git_annex_enabled).and_return(false) }
+            before do
+              allow(Gitlab.config.gitlab_shell).to receive(:git_annex_enabled).and_return(false)
+            end
 
             it { expect(access.check('git-annex-shell', git_annex_changes).allowed?).to be_falsey }
             it { expect(access.push_access_check(git_annex_changes)).not_to be_allowed }
@@ -404,7 +414,13 @@ describe Gitlab::GitAccess, lib: true do
     end
 
     describe "push_rule_check" do
-      before { project.team << [user, :developer] }
+      before do
+        project.team << [user, :developer]
+
+        allow_any_instance_of(Repository).to receive(:new_commits).and_return(
+          project.repository.commits_between('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9', '570e7b2abdd848b95f2f578043fc23bd6f6fd24d')
+        )
+      end
 
       describe "author email check" do
         it 'returns true' do
@@ -455,6 +471,7 @@ describe Gitlab::GitAccess, lib: true do
           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])
 
           project.create_push_rule
           project.push_rule.update(commit_message_regex: "Change some files")
@@ -481,6 +498,12 @@ describe Gitlab::GitAccess, lib: true do
       end
 
       describe "file names check" do
+        before do
+          allow_any_instance_of(Repository).to receive(:new_commits).and_return(
+            project.repository.commits_between('913c66a37b4a45b9769037c55c2d238bd0942d2e', '33f3729a45c02fc67d00adb1b8bca394b0e761d9')
+          )
+        end
+
         it 'returns false when filename is prohibited' do
           project.create_push_rule
           project.push_rule.update(file_name_regex: "jpg$")