fix complexity issue and added spec

lib/gitlab/git_access.rb

@@ -207,14 +207,7 @@ module Gitlab
         # if oldrev is blank, the branch was just created
         oldrev = project.default_branch if blank_oldrev
 
-        commits =
-          if oldrev
-            project.repository.commits_between(oldrev, newrev)
-          else
-            project.repository.commits(newrev)
-          end
-
-        commits.each do |commit|
+        commits(newrev, oldrev, project).each do |commit|
           next if commit_from_annex_sync?(commit.safe_message) || (blank_oldrev && old_commit?(commit))
 
           if status_object = check_commit(commit, git_hook)
@@ -228,6 +221,14 @@ module Gitlab
 
     private
 
+    def commits(newrev, oldrev, project)
+      if oldrev
+        project.repository.commits_between(oldrev, newrev)
+      else
+        project.repository.commits(newrev)
+      end
+    end
+
     # If commit does not pass git hook validation the whole push should be rejected.
     # This method should return nil if no error found or status object if there are some errors.
     # In case of errors - all other checks will be canceled and push will be rejected.

spec/lib/gitlab/git_access_spec.rb

@@ -10,6 +10,19 @@ describe Gitlab::GitAccess, lib: true do
      "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/synced/named-branch"]
   end
   let(:git_annex_master_changes) { "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master" }
+  let(:bad_commit) do
+    OpenStruct.new(
+      id: "570e7b2abdd848b95f2f578043fc23bd6f6fd24d",
+      parent_id: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9',
+      author_full_name: "Dmitriy Zaporozhets",
+      author_email: "dmitriy.zaporozhets@gitlab.com",
+      files_changed_count: 2,
+      line_code: '2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14',
+      line_code_path: 'files/ruby/popen.rb',
+      del_line_code: '2f6fcd96b88b36ce98c38da085c795a27d92a3dd_13_13',
+      safe_message: 'Change some stuff'
+    )
+  end
 
   describe 'can_push_to_branch?' do
     describe 'push to none protected branch' do
@@ -379,6 +392,15 @@ describe Gitlab::GitAccess, lib: true do
         project.git_hook.update(commit_message_regex: "@only.com")
         expect(access.git_hook_check(user, project, 'refs/tags/v1', '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9', '570e7b2abdd848b95f2f578043fc23bd6f6fd24d')).to be_allowed
       end
+
+      it 'allows githook for new branch with an old bad commit' do
+        allow(bad_commit).to receive(:refs).and_return(['heads/master'])
+        allow_any_instance_of(Repository).to receive(:commits_between).and_return([bad_commit])
+        project.create_git_hook
+        project.git_hook.update(commit_message_regex: "Change some files")
+        # push to new branch, so use a blank old rev and new ref
+        expect(access.git_hook_check(user, project, 'refs/heads/new-branch', Gitlab::Git::BLANK_SHA, '570e7b2abdd848b95f2f578043fc23bd6f6fd24d')).to be_allowed
+      end
     end
 
     describe "member_check" do
@@ -437,3 +459,4 @@ describe Gitlab::GitAccess, lib: true do
     end
   end
 end
+