Refactor pre/post receive commit services into one class

app/services/commit_service.rb

@@ -4,48 +4,53 @@ class CommitService
   class PreReceiveError < StandardError; end
   class CommitError < StandardError; end
 
-  def self.transaction(project, current_user, ref)
+  def self.transaction(project, current_user, branch)
     repository = project.repository
     path_to_repo = repository.path_to_repo
     empty_repo = repository.empty?
+    oldrev = Gitlab::Git::BLANK_SHA
+    ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
+    gl_id = Gitlab::ShellEnv.gl_id(current_user)
 
     # Create temporary ref
     random_string = SecureRandom.hex
     tmp_ref = "refs/tmp/#{random_string}/head"
 
     unless empty_repo
-      target = repository.find_branch(ref).target
-      repository.rugged.references.create(tmp_ref, target)
+      oldrev = repository.find_branch(branch).target
+      repository.rugged.references.create(tmp_ref, oldrev)
     end
 
     # Make commit in tmp ref
-    sha = yield(tmp_ref)
+    newrev = yield(tmp_ref)
 
-    unless sha
+    unless newrev
       raise CommitError.new('Failed to create commit')
     end
 
     # Run GitLab pre-receive hook
-    status = PreCommitService.new(project, current_user).execute(sha, ref)
+    pre_receive_hook = Gitlab::Git::Hook.new('pre-receive', path_to_repo)
+    status = pre_receive_hook.trigger(gl_id, oldrev, newrev, ref)
 
     if status
       if empty_repo
         # Create branch
-        repository.rugged.references.create(Gitlab::Git::BRANCH_REF_PREFIX + ref, sha)
+        repository.rugged.references.create(ref, newrev)
       else
         # Update head
-        current_target = repository.find_branch(ref).target
+        current_head = repository.find_branch(branch).target
 
         # Make sure target branch was not changed during pre-receive hook
-        if current_target == target
-          repository.rugged.references.update(Gitlab::Git::BRANCH_REF_PREFIX + ref, sha)
+        if current_head == oldrev
+          repository.rugged.references.update(ref, newrev)
         else
           raise CommitError.new('Commit was rejected because branch received new push')
         end
       end
 
       # Run GitLab post receive hook
-      PostCommitService.new(project, current_user).execute(sha, ref)
+      post_receive_hook = Gitlab::Git::Hook.new('post-receive', path_to_repo)
+      status = post_receive_hook.trigger(gl_id, oldrev, newrev, ref)
     else
       # Remove tmp ref and return error to user
       repository.rugged.references.delete(tmp_ref)

app/services/pre_commit_service.rb

-class PreCommitService < BaseService
-  include Gitlab::Popen
-
-  attr_reader :changes, :repo_path
-
-  def execute(sha, branch)
-    commit = repository.commit(sha)
-    full_ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
-    old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA
-    @changes = "#{old_sha} #{sha} #{full_ref}"
-    @repo_path = repository.path_to_repo
-
-    pre_receive
-  end
-
-  private
-
-  def pre_receive
-    hook = hook_file('pre-receive', repo_path)
-    return true if hook.nil?
-    call_receive_hook(hook)
-  end
-
-  def call_receive_hook(hook)
-    # function  will return true if succesful
-    exit_status = false
-
-    vars = {
-      'GL_ID' => Gitlab::ShellEnv.gl_id(current_user),
-      'PWD' => repo_path
-    }
-
-    options = {
-      chdir: repo_path
-    }
-
-    # we combine both stdout and stderr as we don't know what stream
-    # will be used by the custom hook
-    Open3.popen2e(vars, hook, options) do |stdin, stdout_stderr, wait_thr|
-      exit_status = true
-      stdin.sync = true
-
-      # in git, pre- and post- receive hooks may just exit without
-      # reading stdin. We catch the exception to avoid a broken pipe
-      # warning
-      begin
-        # inject all the changes as stdin to the hook
-        changes.lines do |line|
-          stdin.puts line
-        end
-      rescue Errno::EPIPE
-      end
-
-      # need to close stdin before reading stdout
-      stdin.close
-
-      # only output stdut_stderr if scripts doesn't return 0
-      unless wait_thr.value == 0
-        exit_status = false
-      end
-    end
-
-    exit_status
-  end
-
-  def hook_file(hook_type, repo_path)
-    hook_path = File.join(repo_path.strip, 'hooks')
-    hook_file = "#{hook_path}/#{hook_type}"
-    hook_file if File.exist?(hook_file)
-  end
-end

app/services/post_commit_service.rb → lib/gitlab/git/hook.rb

-class PostCommitService < BaseService
-  include Gitlab::Popen
-
-  attr_reader :changes, :repo_path
-
-  def execute(sha, branch)
-    commit = repository.commit(sha)
-    full_ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
-    old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA
-    @changes = "#{old_sha} #{sha} #{full_ref}"
-    @repo_path = repository.path_to_repo
-
-    post_receive
-  end
-
-  private
-
-  def post_receive
-    hook = hook_file('post-receive', repo_path)
-    return true if hook.nil?
-    call_receive_hook(hook)
-  end
+module Gitlab
+  module Git
+    class Hook
+      attr_reader :name, :repo_path, :path
+
+      def initialize(name, repo_path)
+        @name = name
+        @repo_path = repo_path
+        @path = File.join(repo_path.strip, 'hooks', name)
+      end
 
-  def call_receive_hook(hook)
-    # function  will return true if succesful
-    exit_status = false
+      def exists?
+        File.exist?(path)
+      end
 
-    vars = {
-      'GL_ID' => Gitlab::ShellEnv.gl_id(current_user),
-      'PWD' => repo_path
-    }
+      def trigger(gl_id, oldrev, newrev, ref)
+        return true unless exists?
 
-    options = {
-      chdir: repo_path
-    }
+        changes = [oldrev, newrev, ref].join(" ")
 
-    # we combine both stdout and stderr as we don't know what stream
-    # will be used by the custom hook
-    Open3.popen2e(vars, hook, options) do |stdin, stdout_stderr, wait_thr|
-      exit_status = true
-      stdin.sync = true
+        # function  will return true if succesful
+        exit_status = false
 
-      # in git, pre- and post- receive hooks may just exit without
-      # reading stdin. We catch the exception to avoid a broken pipe
-      # warning
-      begin
-        # inject all the changes as stdin to the hook
-        changes.lines do |line|
-          stdin.puts line
+        vars = {
+          'GL_ID' => gl_id,
+          'PWD' => repo_path
+        }
+
+        options = {
+          chdir: repo_path
+        }
+
+        Open3.popen2(vars, path, options) do |stdin, _, wait_thr|
+          exit_status = true
+          stdin.sync = true
+
+          # in git, pre- and post- receive hooks may just exit without
+          # reading stdin. We catch the exception to avoid a broken pipe
+          # warning
+          begin
+            # inject all the changes as stdin to the hook
+            changes.lines do |line|
+              stdin.puts line
+            end
+          rescue Errno::EPIPE
+          end
+
+          stdin.close
+
+          unless wait_thr.value == 0
+            exit_status = false
+          end
         end
-      rescue Errno::EPIPE
-      end
-
-      # need to close stdin before reading stdout
-      stdin.close
 
-      # only output stdut_stderr if scripts doesn't return 0
-      unless wait_thr.value == 0
-        exit_status = false
+        exit_status
       end
     end
-
-    exit_status
-  end
-
-  def hook_file(hook_type, repo_path)
-    hook_path = File.join(repo_path.strip, 'hooks')
-    hook_file = "#{hook_path}/#{hook_type}"
-    hook_file if File.exist?(hook_file)
   end
 end

spec/support/test_env.rb

@@ -58,7 +58,7 @@ module TestEnv
   end
 
   def disable_pre_receive
-    allow_any_instance_of(PreCommitService).to receive(:execute).and_return(true)
+    allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return(true)
   end
 
   # Clean /tmp/tests