Merge branch 'fix/git-hooks-old-commits' into 'master'

Fix for old commits triggering git hooks on new branches from another branch

Fixes https://gitlab.com/gitlab-org/gitlab-ee/issues/279

See merge request !281

CHANGELOG-EE

@@ -4,6 +4,7 @@ v 8.7.0 (unreleased)
 
 v 8.6.2 (unreleased)
   - Mark pending todos as done when approving a merge request
+  - Fix old commits triggering git hooks on new branches branched off another branch
 
 v 8.6.1
   - Only rename the `light_logo` column in the `appearances` table if its not there yet. !290

lib/gitlab/git_access.rb

@@ -202,18 +202,13 @@ module Gitlab
           return build_status_object(true)
         end
 
-        # if oldrev is blank, the branch was just created
-        oldrev = project.default_branch if Gitlab::Git.blank_ref?(oldrev)
+        blank_oldrev = Gitlab::Git.blank_ref?(oldrev)
 
-        commits =
-          if oldrev
-            project.repository.commits_between(oldrev, newrev)
-          else
-            project.repository.commits(newrev)
-          end
+        # if oldrev is blank, the branch was just created
+        oldrev = project.default_branch if blank_oldrev
 
-        commits.each do |commit|
-          next if commit_from_annex_sync?(commit.safe_message)
+        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)
             return status_object
@@ -226,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.
@@ -373,5 +376,9 @@ module Gitlab
       # Commit message starting with <git-annex in > so avoid git hooks on this
       commit_message.start_with?('git-annex in')
     end
+
+    def old_commit?(commit)
+      commit.refs(project.repository).any?
+    end
   end
 end

spec/lib/gitlab/git_access_spec.rb

@@ -379,6 +379,18 @@ 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
+        bad_commit = double("Commit", safe_message: 'Some change').as_null_object
+        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