From 9b9309329207449ef022bdcf06bff5f8eae36032 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 22 Aug 2017 13:05:36 +0200
Subject: Decouple GitOperationService from User

---
 app/models/repository.rb              | 27 ++++++++++++++++++---------
 app/services/git_hooks_service.rb     |  6 +++---
 app/services/git_operation_service.rb | 11 +++++++----
 lib/gitlab/git/committer.rb           | 21 +++++++++++++++++++++
 spec/models/repository_spec.rb        | 21 +++++++++++----------
 5 files changed, 60 insertions(+), 26 deletions(-)
 create mode 100644 lib/gitlab/git/committer.rb

diff --git a/app/models/repository.rb b/app/models/repository.rb
index c1e4fcf94a4..3c5074e5157 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -164,7 +164,8 @@ class Repository
 
     return false unless newrev
 
-    GitOperationService.new(user, self).add_branch(branch_name, newrev)
+    committer = Gitlab::Git::Committer.from_user(user)
+    GitOperationService.new(committer, self).add_branch(branch_name, newrev)
 
     after_create_branch
     find_branch(branch_name)
@@ -176,7 +177,8 @@ class Repository
 
     return false unless newrev
 
-    GitOperationService.new(user, self).add_tag(tag_name, newrev, options)
+    committer = Gitlab::Git::Committer.from_user(user)
+    GitOperationService.new(committer, self).add_tag(tag_name, newrev, options)
 
     find_tag(tag_name)
   end
@@ -185,7 +187,8 @@ class Repository
     before_remove_branch
     branch = find_branch(branch_name)
 
-    GitOperationService.new(user, self).rm_branch(branch)
+    committer = Gitlab::Git::Committer.from_user(user)
+    GitOperationService.new(committer, self).rm_branch(branch)
 
     after_remove_branch
     true
@@ -195,7 +198,8 @@ class Repository
     before_remove_tag
     tag = find_tag(tag_name)
 
-    GitOperationService.new(user, self).rm_tag(tag)
+    committer = Gitlab::Git::Committer.from_user(user)
+    GitOperationService.new(committer, self).rm_tag(tag)
 
     after_remove_tag
     true
@@ -763,7 +767,8 @@ class Repository
     author_email: nil, author_name: nil,
     start_branch_name: nil, start_project: project)
 
-    GitOperationService.new(user, self).with_branch(
+    committer = Gitlab::Git::Committer.from_user(user)
+    GitOperationService.new(committer, self).with_branch(
       branch_name,
       start_branch_name: start_branch_name,
       start_project: start_project) do |start_commit|
@@ -819,7 +824,8 @@ class Repository
   end
 
   def merge(user, source, merge_request, options = {})
-    GitOperationService.new(user, self).with_branch(
+    committer = Gitlab::Git::Committer.from_user(user)
+    GitOperationService.new(committer, self).with_branch(
       merge_request.target_branch) do |start_commit|
       our_commit = start_commit.sha
       their_commit = source
@@ -846,7 +852,8 @@ class Repository
   def revert(
     user, commit, branch_name,
     start_branch_name: nil, start_project: project)
-    GitOperationService.new(user, self).with_branch(
+    committer = Gitlab::Git::Committer.from_user(user)
+    GitOperationService.new(committer, self).with_branch(
       branch_name,
       start_branch_name: start_branch_name,
       start_project: start_project) do |start_commit|
@@ -869,7 +876,8 @@ class Repository
   def cherry_pick(
     user, commit, branch_name,
     start_branch_name: nil, start_project: project)
-    GitOperationService.new(user, self).with_branch(
+    committer = Gitlab::Git::Committer.from_user(user)
+    GitOperationService.new(committer, self).with_branch(
       branch_name,
       start_branch_name: start_branch_name,
       start_project: start_project) do |start_commit|
@@ -894,7 +902,8 @@ class Repository
   end
 
   def resolve_conflicts(user, branch_name, params)
-    GitOperationService.new(user, self).with_branch(branch_name) do
+    committer = Gitlab::Git::Committer.from_user(user)
+    GitOperationService.new(committer, self).with_branch(branch_name) do
       committer = user_to_committer(user)
 
       create_commit(params.merge(author: committer, committer: committer))
diff --git a/app/services/git_hooks_service.rb b/app/services/git_hooks_service.rb
index eab65d09299..e85007c26e0 100644
--- a/app/services/git_hooks_service.rb
+++ b/app/services/git_hooks_service.rb
@@ -3,9 +3,9 @@ class GitHooksService
 
   attr_accessor :oldrev, :newrev, :ref
 
-  def execute(user, project, oldrev, newrev, ref)
+  def execute(committer, project, oldrev, newrev, ref)
     @project    = project
-    @user       = Gitlab::GlId.gl_id(user)
+    @gl_id       = committer.gl_id
     @oldrev     = oldrev
     @newrev     = newrev
     @ref        = ref
@@ -27,6 +27,6 @@ class GitHooksService
 
   def run_hook(name)
     hook = Gitlab::Git::Hook.new(name, @project)
-    hook.trigger(@user, oldrev, newrev, ref)
+    hook.trigger(@gl_id, oldrev, newrev, ref)
   end
 end
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 545ca0742e4..f7fce3d8a5d 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -1,8 +1,11 @@
 class GitOperationService
-  attr_reader :user, :repository
+  attr_reader :committer, :repository
 
-  def initialize(new_user, new_repository)
-    @user = new_user
+  def initialize(committer, new_repository)
+    if committer && !committer.is_a?(Gitlab::Git::Committer)
+      raise "expected Gitlab::Git::Committer, got #{committer.inspect}"
+    end
+    @committer = committer
     @repository = new_repository
   end
 
@@ -119,7 +122,7 @@ class GitOperationService
 
   def with_hooks(ref, newrev, oldrev)
     GitHooksService.new.execute(
-      user,
+      committer,
       repository.project,
       oldrev,
       newrev,
diff --git a/lib/gitlab/git/committer.rb b/lib/gitlab/git/committer.rb
new file mode 100644
index 00000000000..ef3576eaa1c
--- /dev/null
+++ b/lib/gitlab/git/committer.rb
@@ -0,0 +1,21 @@
+module Gitlab
+  module Git
+    class Committer
+      attr_reader :name, :email, :gl_id
+      
+      def self.from_user(user)
+        new(user.name, user.email, Gitlab::GlId.gl_id(user))
+      end
+
+      def initialize(name, email, gl_id)
+        @name = name
+        @email = email
+        @gl_id = gl_id
+      end
+
+      def ==(other)
+        [name, email, gl_id] == [other.name, other.email, other.gl_id]
+      end
+    end
+  end
+end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 4926d5d6c49..0b1a35d1920 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -8,6 +8,7 @@ describe Repository, models: true do
   let(:repository) { project.repository }
   let(:broken_repository) { create(:project, :broken_storage).repository }
   let(:user) { create(:user) }
+  let(:committer) { Gitlab::Git::Committer.from_user(user) }
 
   let(:commit_options) do
     author = repository.user_to_committer(user)
@@ -885,7 +886,7 @@ describe Repository, models: true do
     context 'when pre hooks were successful' do
       it 'runs without errors' do
         expect_any_instance_of(GitHooksService).to receive(:execute)
-          .with(user, project, old_rev, blank_sha, 'refs/heads/feature')
+          .with(committer, project, old_rev, blank_sha, 'refs/heads/feature')
 
         expect { repository.rm_branch(user, 'feature') }.not_to raise_error
       end
@@ -928,20 +929,20 @@ describe Repository, models: true do
         service = GitHooksService.new
         expect(GitHooksService).to receive(:new).and_return(service)
         expect(service).to receive(:execute)
-          .with(user, project, old_rev, new_rev, 'refs/heads/feature')
+          .with(committer, project, old_rev, new_rev, 'refs/heads/feature')
           .and_yield(service).and_return(true)
       end
 
       it 'runs without errors' do
         expect do
-          GitOperationService.new(user, repository).with_branch('feature') do
+          GitOperationService.new(committer, repository).with_branch('feature') do
             new_rev
           end
         end.not_to raise_error
       end
 
       it 'ensures the autocrlf Git option is set to :input' do
-        service = GitOperationService.new(user, repository)
+        service = GitOperationService.new(committer, repository)
 
         expect(service).to receive(:update_autocrlf_option)
 
@@ -952,7 +953,7 @@ describe Repository, models: true do
         it 'updates the head' do
           expect(repository.find_branch('feature').dereferenced_target.id).to eq(old_rev)
 
-          GitOperationService.new(user, repository).with_branch('feature') do
+          GitOperationService.new(committer, repository).with_branch('feature') do
             new_rev
           end
 
@@ -974,7 +975,7 @@ describe Repository, models: true do
         end
 
         expect do
-          GitOperationService.new(user, target_project.repository)
+          GitOperationService.new(committer, target_project.repository)
             .with_branch('feature',
                          start_project: project,
                          &:itself)
@@ -996,7 +997,7 @@ describe Repository, models: true do
         repository.add_branch(user, branch, old_rev)
 
         expect do
-          GitOperationService.new(user, repository).with_branch(branch) do
+          GitOperationService.new(committer, repository).with_branch(branch) do
             new_rev
           end
         end.not_to raise_error
@@ -1014,7 +1015,7 @@ describe Repository, models: true do
         # Updating 'master' to new_rev would lose the commits on 'master' that
         # are not contained in new_rev. This should not be allowed.
         expect do
-          GitOperationService.new(user, repository).with_branch(branch) do
+          GitOperationService.new(committer, repository).with_branch(branch) do
             new_rev
           end
         end.to raise_error(Repository::CommitError)
@@ -1026,7 +1027,7 @@ describe Repository, models: true do
         allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
 
         expect do
-          GitOperationService.new(user, repository).with_branch('feature') do
+          GitOperationService.new(committer, repository).with_branch('feature') do
             new_rev
           end
         end.to raise_error(GitHooksService::PreReceiveError)
@@ -1044,7 +1045,7 @@ describe Repository, models: true do
         expect(repository).not_to receive(:expire_emptiness_caches)
         expect(repository).to     receive(:expire_branches_cache)
 
-        GitOperationService.new(user, repository)
+        GitOperationService.new(committer, repository)
           .with_branch('new-feature') do
             new_rev
           end
-- 
cgit v1.2.1


From 65f83941c39c14c2af9da5064393545ea2f7b3e5 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 22 Aug 2017 13:54:14 +0200
Subject: Make gl_repository a G::G::Repository attribute

---
 app/models/repository.rb                       |  2 +-
 app/services/git_hooks_service.rb              |  8 +++----
 app/services/git_operation_service.rb          |  2 +-
 db/migrate/20140502125220_migrate_repo_size.rb |  2 +-
 lib/gitlab/git/hook.rb                         | 13 +++++++----
 lib/gitlab/git/repository.rb                   |  5 ++--
 spec/lib/gitlab/git/blame_spec.rb              |  2 +-
 spec/lib/gitlab/git/blob_spec.rb               |  2 +-
 spec/lib/gitlab/git/branch_spec.rb             |  2 +-
 spec/lib/gitlab/git/commit_spec.rb             |  8 +++----
 spec/lib/gitlab/git/compare_spec.rb            |  2 +-
 spec/lib/gitlab/git/diff_spec.rb               |  2 +-
 spec/lib/gitlab/git/hook_spec.rb               |  9 ++++----
 spec/lib/gitlab/git/index_spec.rb              |  2 +-
 spec/lib/gitlab/git/repository_spec.rb         | 32 +++++++++++++-------------
 spec/lib/gitlab/git/tag_spec.rb                |  2 +-
 spec/lib/gitlab/git/tree_spec.rb               |  2 +-
 spec/models/repository_spec.rb                 |  4 ++--
 18 files changed, 53 insertions(+), 48 deletions(-)

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 3c5074e5157..062f532233f 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1194,7 +1194,7 @@ class Repository
   end
 
   def initialize_raw_repository
-    Gitlab::Git::Repository.new(project.repository_storage, disk_path + '.git')
+    Gitlab::Git::Repository.new(project.repository_storage, disk_path + '.git', Gitlab::GlRepository.gl_repository(project, false))
   end
 
   def circuit_breaker
diff --git a/app/services/git_hooks_service.rb b/app/services/git_hooks_service.rb
index e85007c26e0..c14133230da 100644
--- a/app/services/git_hooks_service.rb
+++ b/app/services/git_hooks_service.rb
@@ -3,9 +3,9 @@ class GitHooksService
 
   attr_accessor :oldrev, :newrev, :ref
 
-  def execute(committer, project, oldrev, newrev, ref)
-    @project    = project
-    @gl_id       = committer.gl_id
+  def execute(committer, repository, oldrev, newrev, ref)
+    @repository = repository
+    @gl_id      = committer.gl_id
     @oldrev     = oldrev
     @newrev     = newrev
     @ref        = ref
@@ -26,7 +26,7 @@ class GitHooksService
   private
 
   def run_hook(name)
-    hook = Gitlab::Git::Hook.new(name, @project)
+    hook = Gitlab::Git::Hook.new(name, @repository)
     hook.trigger(@gl_id, oldrev, newrev, ref)
   end
 end
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index f7fce3d8a5d..d20e4ed9e88 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -123,7 +123,7 @@ class GitOperationService
   def with_hooks(ref, newrev, oldrev)
     GitHooksService.new.execute(
       committer,
-      repository.project,
+      repository,
       oldrev,
       newrev,
       ref) do |service|
diff --git a/db/migrate/20140502125220_migrate_repo_size.rb b/db/migrate/20140502125220_migrate_repo_size.rb
index f5d5d834307..ca1b054600c 100644
--- a/db/migrate/20140502125220_migrate_repo_size.rb
+++ b/db/migrate/20140502125220_migrate_repo_size.rb
@@ -11,7 +11,7 @@ class MigrateRepoSize < ActiveRecord::Migration
       path = File.join(namespace_path, project['project_path'] + '.git')
 
       begin
-        repo = Gitlab::Git::Repository.new('default', path)
+        repo = Gitlab::Git::Repository.new('default', path, '')
         if repo.empty?
           print '-'
         else
diff --git a/lib/gitlab/git/hook.rb b/lib/gitlab/git/hook.rb
index 8f0c377ef4f..08cede42ba2 100644
--- a/lib/gitlab/git/hook.rb
+++ b/lib/gitlab/git/hook.rb
@@ -6,15 +6,18 @@ module Gitlab
   module Git
     class Hook
       GL_PROTOCOL = 'web'.freeze
-      attr_reader :name, :repo_path, :path
+      attr_reader :name, :path, :repository
 
-      def initialize(name, project)
+      def initialize(name, repository)
         @name = name
-        @project = project
-        @repo_path = project.repository.path
+        @repository = repository
         @path = File.join(repo_path.strip, 'hooks', name)
       end
 
+      def repo_path
+        repository.path
+      end
+
       def exists?
         File.exist?(path)
       end
@@ -44,7 +47,7 @@ module Gitlab
           'GL_ID' => gl_id,
           'PWD' => repo_path,
           'GL_PROTOCOL' => GL_PROTOCOL,
-          'GL_REPOSITORY' => Gitlab::GlRepository.gl_repository(@project, false)
+          'GL_REPOSITORY' => repository.gl_repository
         }
 
         options = {
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index eb3731ba35a..976ea7e4035 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -49,13 +49,14 @@ module Gitlab
       # Rugged repo object
       attr_reader :rugged
 
-      attr_reader :storage
+      attr_reader :storage, :gl_repository, :relative_path
 
       # 'path' must be the path to a _bare_ git repository, e.g.
       # /path/to/my-repo.git
-      def initialize(storage, relative_path)
+      def initialize(storage, relative_path, gl_repository)
         @storage = storage
         @relative_path = relative_path
+        @gl_repository = gl_repository
 
         storage_path = Gitlab.config.repositories.storages[@storage]['path']
         @path = File.join(storage_path, @relative_path)
diff --git a/spec/lib/gitlab/git/blame_spec.rb b/spec/lib/gitlab/git/blame_spec.rb
index 800c245b130..465c2012b05 100644
--- a/spec/lib/gitlab/git/blame_spec.rb
+++ b/spec/lib/gitlab/git/blame_spec.rb
@@ -2,7 +2,7 @@
 require "spec_helper"
 
 describe Gitlab::Git::Blame, seed_helper: true do
-  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
   let(:blame) do
     Gitlab::Git::Blame.new(repository, SeedRepo::Commit::ID, "CONTRIBUTING.md")
   end
diff --git a/spec/lib/gitlab/git/blob_spec.rb b/spec/lib/gitlab/git/blob_spec.rb
index dfab0c2fe85..66ba00acb7d 100644
--- a/spec/lib/gitlab/git/blob_spec.rb
+++ b/spec/lib/gitlab/git/blob_spec.rb
@@ -3,7 +3,7 @@
 require "spec_helper"
 
 describe Gitlab::Git::Blob, seed_helper: true do
-  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
 
   describe 'initialize' do
     let(:blob) { Gitlab::Git::Blob.new(name: 'test') }
diff --git a/spec/lib/gitlab/git/branch_spec.rb b/spec/lib/gitlab/git/branch_spec.rb
index cdf1b8beee3..318a7b7a332 100644
--- a/spec/lib/gitlab/git/branch_spec.rb
+++ b/spec/lib/gitlab/git/branch_spec.rb
@@ -1,7 +1,7 @@
 require "spec_helper"
 
 describe Gitlab::Git::Branch, seed_helper: true do
-  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
 
   subject { repository.branches }
 
diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb
index ac33cd8a2c9..14d64d8c4da 100644
--- a/spec/lib/gitlab/git/commit_spec.rb
+++ b/spec/lib/gitlab/git/commit_spec.rb
@@ -1,7 +1,7 @@
 require "spec_helper"
 
 describe Gitlab::Git::Commit, seed_helper: true do
-  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
   let(:commit) { described_class.find(repository, SeedRepo::Commit::ID) }
   let(:rugged_commit) do
     repository.rugged.lookup(SeedRepo::Commit::ID)
@@ -9,7 +9,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
 
   describe "Commit info" do
     before do
-      repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH).rugged
+      repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
 
       @committer = {
         email: 'mike@smith.com',
@@ -59,7 +59,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
 
     after do
       # Erase the new commit so other tests get the original repo
-      repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH).rugged
+      repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
       repo.references.update("refs/heads/master", SeedRepo::LastCommit::ID)
     end
   end
@@ -144,7 +144,7 @@ describe Gitlab::Git::Commit, seed_helper: true do
       end
 
       context 'with broken repo' do
-        let(:repository) { Gitlab::Git::Repository.new('default', TEST_BROKEN_REPO_PATH) }
+        let(:repository) { Gitlab::Git::Repository.new('default', TEST_BROKEN_REPO_PATH, '') }
 
         it 'returns nil' do
           expect(described_class.find(repository, SeedRepo::Commit::ID)).to be_nil
diff --git a/spec/lib/gitlab/git/compare_spec.rb b/spec/lib/gitlab/git/compare_spec.rb
index 4c9f4a28f32..b6a42e422b5 100644
--- a/spec/lib/gitlab/git/compare_spec.rb
+++ b/spec/lib/gitlab/git/compare_spec.rb
@@ -1,7 +1,7 @@
 require "spec_helper"
 
 describe Gitlab::Git::Compare, seed_helper: true do
-  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
   let(:compare) { Gitlab::Git::Compare.new(repository, SeedRepo::BigCommit::ID, SeedRepo::Commit::ID, straight: false) }
   let(:compare_straight) { Gitlab::Git::Compare.new(repository, SeedRepo::BigCommit::ID, SeedRepo::Commit::ID, straight: true) }
 
diff --git a/spec/lib/gitlab/git/diff_spec.rb b/spec/lib/gitlab/git/diff_spec.rb
index 7ea3386ac2a..dfbdbee48f7 100644
--- a/spec/lib/gitlab/git/diff_spec.rb
+++ b/spec/lib/gitlab/git/diff_spec.rb
@@ -1,7 +1,7 @@
 require "spec_helper"
 
 describe Gitlab::Git::Diff, seed_helper: true do
-  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
 
   before do
     @raw_diff_hash = {
diff --git a/spec/lib/gitlab/git/hook_spec.rb b/spec/lib/gitlab/git/hook_spec.rb
index 19391a70cf6..ea3e4680b1d 100644
--- a/spec/lib/gitlab/git/hook_spec.rb
+++ b/spec/lib/gitlab/git/hook_spec.rb
@@ -10,7 +10,8 @@ describe Gitlab::Git::Hook do
 
   describe "#trigger" do
     let(:project) { create(:project, :repository) }
-    let(:repo_path) { project.repository.path }
+    let(:repository) { project.repository.raw_repository }
+    let(:repo_path) { repository.path }
     let(:user) { create(:user) }
     let(:gl_id) { Gitlab::GlId.gl_id(user) }
 
@@ -48,7 +49,7 @@ describe Gitlab::Git::Hook do
 
           it "returns success with no errors" do
             create_hook(hook_name)
-            hook = described_class.new(hook_name, project)
+            hook = described_class.new(hook_name, repository)
             blank = Gitlab::Git::BLANK_SHA
             ref = Gitlab::Git::BRANCH_REF_PREFIX + 'new_branch'
 
@@ -66,7 +67,7 @@ describe Gitlab::Git::Hook do
         context "when the hook is unsuccessful" do
           it "returns failure with errors" do
             create_failing_hook(hook_name)
-            hook = described_class.new(hook_name, project)
+            hook = described_class.new(hook_name, repository)
             blank = Gitlab::Git::BLANK_SHA
             ref = Gitlab::Git::BRANCH_REF_PREFIX + 'new_branch'
 
@@ -80,7 +81,7 @@ describe Gitlab::Git::Hook do
 
     context "when the hook doesn't exist" do
       it "returns success with no errors" do
-        hook = described_class.new('unknown_hook', project)
+        hook = described_class.new('unknown_hook', repository)
         blank = Gitlab::Git::BLANK_SHA
         ref = Gitlab::Git::BRANCH_REF_PREFIX + 'new_branch'
 
diff --git a/spec/lib/gitlab/git/index_spec.rb b/spec/lib/gitlab/git/index_spec.rb
index 21b71654251..73fbc6a6afa 100644
--- a/spec/lib/gitlab/git/index_spec.rb
+++ b/spec/lib/gitlab/git/index_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Gitlab::Git::Index, seed_helper: true do
-  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
   let(:index) { described_class.new(repository) }
 
   before do
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 8ec8dfe6acf..06fcdd7aa57 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -17,7 +17,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
-  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
 
   describe "Respond to" do
     subject { repository }
@@ -56,14 +56,14 @@ describe Gitlab::Git::Repository, seed_helper: true do
   describe "#rugged" do
     describe 'when storage is broken', broken_storage: true  do
       it 'raises a storage exception when storage is not available' do
-        broken_repo = described_class.new('broken', 'a/path.git')
+        broken_repo = described_class.new('broken', 'a/path.git', '')
 
         expect { broken_repo.rugged }.to raise_error(Gitlab::Git::Storage::Inaccessible)
       end
     end
 
     it 'raises a no repository exception when there is no repo' do
-      broken_repo = described_class.new('default', 'a/path.git')
+      broken_repo = described_class.new('default', 'a/path.git', '')
 
       expect { broken_repo.rugged }.to raise_error(Gitlab::Git::Repository::NoRepository)
     end
@@ -257,7 +257,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#submodule_url_for' do
-    let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+    let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
     let(:ref) { 'master' }
 
     def submodule_url(path)
@@ -295,7 +295,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   context '#submodules' do
-    let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+    let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
 
     context 'where repo has submodules' do
       let(:submodules) { repository.send(:submodules, 'master') }
@@ -391,7 +391,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
   describe "#delete_branch" do
     before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH)
+      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
       @repo.delete_branch("feature")
     end
 
@@ -407,7 +407,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
   describe "#create_branch" do
     before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH)
+      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
     end
 
     it "should create a new branch" do
@@ -445,7 +445,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
   describe "#remote_delete" do
     before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH)
+      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
       @repo.remote_delete("expendable")
     end
 
@@ -461,7 +461,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
   describe "#remote_add" do
     before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH)
+      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
       @repo.remote_add("new_remote", SeedHelper::GITLAB_GIT_TEST_REPO_URL)
     end
 
@@ -477,7 +477,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
   describe "#remote_update" do
     before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH)
+      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
       @repo.remote_update("expendable", url: TEST_NORMAL_REPO_PATH)
     end
 
@@ -506,7 +506,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
     before(:context) do
       # Add new commits so that there's a renamed file in the commit history
-      repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH).rugged
+      repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
       @commit_with_old_name_id = new_commit_edit_old_file(repo)
       @rename_commit_id = new_commit_move_file(repo)
       @commit_with_new_name_id = new_commit_edit_new_file(repo)
@@ -514,7 +514,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
     after(:context) do
       # Erase our commits so other tests get the original repo
-      repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH).rugged
+      repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
       repo.references.update("refs/heads/master", SeedRepo::LastCommit::ID)
     end
 
@@ -849,7 +849,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
   describe '#autocrlf' do
     before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH)
+      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
       @repo.rugged.config['core.autocrlf'] = true
     end
 
@@ -864,7 +864,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
   describe '#autocrlf=' do
     before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH)
+      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
       @repo.rugged.config['core.autocrlf'] = false
     end
 
@@ -933,7 +933,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
     context 'with local and remote branches' do
       let(:repository) do
-        Gitlab::Git::Repository.new('default', File.join(TEST_MUTABLE_REPO_PATH, '.git'))
+        Gitlab::Git::Repository.new('default', File.join(TEST_MUTABLE_REPO_PATH, '.git'), '')
       end
 
       before do
@@ -1128,7 +1128,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
   describe '#local_branches' do
     before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', File.join(TEST_MUTABLE_REPO_PATH, '.git'))
+      @repo = Gitlab::Git::Repository.new('default', File.join(TEST_MUTABLE_REPO_PATH, '.git'), '')
     end
 
     after(:all) do
diff --git a/spec/lib/gitlab/git/tag_spec.rb b/spec/lib/gitlab/git/tag_spec.rb
index 78d1e120013..cc10679ef1e 100644
--- a/spec/lib/gitlab/git/tag_spec.rb
+++ b/spec/lib/gitlab/git/tag_spec.rb
@@ -1,7 +1,7 @@
 require "spec_helper"
 
 describe Gitlab::Git::Tag, seed_helper: true do
-  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
 
   shared_examples 'Gitlab::Git::Repository#tags' do
     describe 'first tag' do
diff --git a/spec/lib/gitlab/git/tree_spec.rb b/spec/lib/gitlab/git/tree_spec.rb
index 98ddd3c3664..c07a2d91768 100644
--- a/spec/lib/gitlab/git/tree_spec.rb
+++ b/spec/lib/gitlab/git/tree_spec.rb
@@ -1,7 +1,7 @@
 require "spec_helper"
 
 describe Gitlab::Git::Tree, seed_helper: true do
-  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
 
   context :repo do
     let(:tree) { Gitlab::Git::Tree.where(repository, SeedRepo::Commit::ID) }
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 0b1a35d1920..62585fdb35f 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -886,7 +886,7 @@ describe Repository, models: true do
     context 'when pre hooks were successful' do
       it 'runs without errors' do
         expect_any_instance_of(GitHooksService).to receive(:execute)
-          .with(committer, project, old_rev, blank_sha, 'refs/heads/feature')
+          .with(committer, repository, old_rev, blank_sha, 'refs/heads/feature')
 
         expect { repository.rm_branch(user, 'feature') }.not_to raise_error
       end
@@ -929,7 +929,7 @@ describe Repository, models: true do
         service = GitHooksService.new
         expect(GitHooksService).to receive(:new).and_return(service)
         expect(service).to receive(:execute)
-          .with(committer, project, old_rev, new_rev, 'refs/heads/feature')
+          .with(committer, repository, old_rev, new_rev, 'refs/heads/feature')
           .and_yield(service).and_return(true)
       end
 
-- 
cgit v1.2.1


From dc7c6bede27abd4507072276ef23b40a74ee297a Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 22 Aug 2017 14:18:09 +0200
Subject: Move GitHooksService to Gitlab::Git

---
 app/services/commits/create_service.rb             |  2 +-
 app/services/create_branch_service.rb              |  2 +-
 app/services/delete_branch_service.rb              |  2 +-
 app/services/git_hooks_service.rb                  | 32 -------------------
 app/services/git_operation_service.rb              |  2 +-
 app/services/merge_requests/merge_service.rb       |  2 +-
 app/services/tags/create_service.rb                |  2 +-
 app/services/tags/destroy_service.rb               |  2 +-
 app/services/validate_new_branch_service.rb        |  2 +-
 db/fixtures/development/17_cycle_analytics.rb      |  2 +-
 lib/gitlab/git/hook.rb                             |  4 +--
 lib/gitlab/git/hooks_service.rb                    | 37 ++++++++++++++++++++++
 spec/features/tags/master_deletes_tag_spec.rb      |  4 +--
 spec/lib/gitlab/git_access_spec.rb                 |  2 +-
 spec/models/repository_spec.rb                     | 16 +++++-----
 spec/services/files/update_service_spec.rb         |  2 +-
 spec/services/git_hooks_service_spec.rb            |  6 ++--
 spec/services/merge_requests/merge_service_spec.rb |  2 +-
 spec/services/tags/create_service_spec.rb          |  2 +-
 19 files changed, 65 insertions(+), 60 deletions(-)
 delete mode 100644 app/services/git_hooks_service.rb
 create mode 100644 lib/gitlab/git/hooks_service.rb

diff --git a/app/services/commits/create_service.rb b/app/services/commits/create_service.rb
index c58f04a252b..dbd0b9ef43a 100644
--- a/app/services/commits/create_service.rb
+++ b/app/services/commits/create_service.rb
@@ -17,7 +17,7 @@ module Commits
       new_commit = create_commit!
 
       success(result: new_commit)
-    rescue ValidationError, ChangeError, Gitlab::Git::Index::IndexError, Repository::CommitError, GitHooksService::PreReceiveError => ex
+    rescue ValidationError, ChangeError, Gitlab::Git::Index::IndexError, Repository::CommitError, Gitlab::Git::HooksService::PreReceiveError => ex
       error(ex.message)
     end
 
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index 673ed02f952..0ba6a0ac6b5 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -14,7 +14,7 @@ class CreateBranchService < BaseService
     else
       error('Invalid reference name')
     end
-  rescue GitHooksService::PreReceiveError => ex
+  rescue Gitlab::Git::HooksService::PreReceiveError => ex
     error(ex.message)
   end
 
diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb
index 64b3c0118fb..1f059c31944 100644
--- a/app/services/delete_branch_service.rb
+++ b/app/services/delete_branch_service.rb
@@ -16,7 +16,7 @@ class DeleteBranchService < BaseService
     else
       error('Failed to remove branch')
     end
-  rescue GitHooksService::PreReceiveError => ex
+  rescue Gitlab::Git::HooksService::PreReceiveError => ex
     error(ex.message)
   end
 
diff --git a/app/services/git_hooks_service.rb b/app/services/git_hooks_service.rb
deleted file mode 100644
index c14133230da..00000000000
--- a/app/services/git_hooks_service.rb
+++ /dev/null
@@ -1,32 +0,0 @@
-class GitHooksService
-  PreReceiveError = Class.new(StandardError)
-
-  attr_accessor :oldrev, :newrev, :ref
-
-  def execute(committer, repository, oldrev, newrev, ref)
-    @repository = repository
-    @gl_id      = committer.gl_id
-    @oldrev     = oldrev
-    @newrev     = newrev
-    @ref        = ref
-
-    %w(pre-receive update).each do |hook_name|
-      status, message = run_hook(hook_name)
-
-      unless status
-        raise PreReceiveError, message
-      end
-    end
-
-    yield(self).tap do
-      run_hook('post-receive')
-    end
-  end
-
-  private
-
-  def run_hook(name)
-    hook = Gitlab::Git::Hook.new(name, @repository)
-    hook.trigger(@gl_id, oldrev, newrev, ref)
-  end
-end
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index d20e4ed9e88..6a983566526 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -121,7 +121,7 @@ class GitOperationService
   end
 
   def with_hooks(ref, newrev, oldrev)
-    GitHooksService.new.execute(
+    Gitlab::Git::HooksService.new.execute(
       committer,
       repository,
       oldrev,
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index bc846e07f24..5be749cd6a0 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -49,7 +49,7 @@ module MergeRequests
       raise MergeError, 'Conflicts detected during merge' unless commit_id
 
       merge_request.update(merge_commit_sha: commit_id)
-    rescue GitHooksService::PreReceiveError => e
+    rescue Gitlab::Git::HooksService::PreReceiveError => e
       raise MergeError, e.message
     rescue StandardError => e
       raise MergeError, "Something went wrong during merge: #{e.message}"
diff --git a/app/services/tags/create_service.rb b/app/services/tags/create_service.rb
index 674792f6138..b3f4a72d6fe 100644
--- a/app/services/tags/create_service.rb
+++ b/app/services/tags/create_service.rb
@@ -13,7 +13,7 @@ module Tags
         new_tag = repository.add_tag(current_user, tag_name, target, message)
       rescue Rugged::TagError
         return error("Tag #{tag_name} already exists")
-      rescue GitHooksService::PreReceiveError => ex
+      rescue Gitlab::Git::HooksService::PreReceiveError => ex
         return error(ex.message)
       end
 
diff --git a/app/services/tags/destroy_service.rb b/app/services/tags/destroy_service.rb
index a368f4f5b61..d3d46064729 100644
--- a/app/services/tags/destroy_service.rb
+++ b/app/services/tags/destroy_service.rb
@@ -21,7 +21,7 @@ module Tags
       else
         error('Failed to remove tag')
       end
-    rescue GitHooksService::PreReceiveError => ex
+    rescue Gitlab::Git::HooksService::PreReceiveError => ex
       error(ex.message)
     end
 
diff --git a/app/services/validate_new_branch_service.rb b/app/services/validate_new_branch_service.rb
index d232e85cd33..7d1ed768ee8 100644
--- a/app/services/validate_new_branch_service.rb
+++ b/app/services/validate_new_branch_service.rb
@@ -13,7 +13,7 @@ class ValidateNewBranchService < BaseService
     end
 
     success
-  rescue GitHooksService::PreReceiveError => ex
+  rescue Gitlab::Git::HooksService::PreReceiveError => ex
     error(ex.message)
   end
 end
diff --git a/db/fixtures/development/17_cycle_analytics.rb b/db/fixtures/development/17_cycle_analytics.rb
index 7c1d758dada..383782112a8 100644
--- a/db/fixtures/development/17_cycle_analytics.rb
+++ b/db/fixtures/development/17_cycle_analytics.rb
@@ -15,7 +15,7 @@ class Gitlab::Seeder::CycleAnalytics
   # to disable the `pre_receive` hook in order to remove this
   # dependency on the GitLab API.
   def stub_git_pre_receive!
-    GitHooksService.class_eval do
+    Gitlab::Git::HooksService.class_eval do
       def run_hook(name)
         [true, '']
       end
diff --git a/lib/gitlab/git/hook.rb b/lib/gitlab/git/hook.rb
index 08cede42ba2..cc35d77c6e4 100644
--- a/lib/gitlab/git/hook.rb
+++ b/lib/gitlab/git/hook.rb
@@ -1,6 +1,6 @@
-# Gitaly note: JV: looks like this is only used by GitHooksService in
+# Gitaly note: JV: looks like this is only used by Gitlab::Git::HooksService in
 # app/services. We shouldn't bother migrating this until we know how
-# GitHooksService will be migrated.
+# Gitlab::Git::HooksService will be migrated.
 
 module Gitlab
   module Git
diff --git a/lib/gitlab/git/hooks_service.rb b/lib/gitlab/git/hooks_service.rb
new file mode 100644
index 00000000000..1da92fcc0e2
--- /dev/null
+++ b/lib/gitlab/git/hooks_service.rb
@@ -0,0 +1,37 @@
+module Gitlab
+  module Git
+    class HooksService
+      PreReceiveError = Class.new(StandardError)
+    
+      attr_accessor :oldrev, :newrev, :ref
+    
+      def execute(committer, repository, oldrev, newrev, ref)
+        @repository = repository
+        @gl_id      = committer.gl_id
+        @oldrev     = oldrev
+        @newrev     = newrev
+        @ref        = ref
+    
+        %w(pre-receive update).each do |hook_name|
+          status, message = run_hook(hook_name)
+    
+          unless status
+            raise PreReceiveError, message
+          end
+        end
+    
+        yield(self).tap do
+          run_hook('post-receive')
+        end
+      end
+    
+      private
+    
+      def run_hook(name)
+        hook = Gitlab::Git::Hook.new(name, @repository)
+        hook.trigger(@gl_id, oldrev, newrev, ref)
+      end
+    end
+  end
+end
+
diff --git a/spec/features/tags/master_deletes_tag_spec.rb b/spec/features/tags/master_deletes_tag_spec.rb
index 4d6fc13557f..d6a6b8fc7d5 100644
--- a/spec/features/tags/master_deletes_tag_spec.rb
+++ b/spec/features/tags/master_deletes_tag_spec.rb
@@ -36,8 +36,8 @@ feature 'Master deletes tag' do
 
   context 'when pre-receive hook fails', js: true do
     before do
-      allow_any_instance_of(GitHooksService).to receive(:execute)
-        .and_raise(GitHooksService::PreReceiveError, 'Do not delete tags')
+      allow_any_instance_of(Gitlab::Git::HooksService).to receive(:execute)
+        .and_raise(Gitlab::Git::HooksService::PreReceiveError, 'Do not delete tags')
     end
 
     scenario 'shows the error message' do
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 80dc49e99cb..295a979da76 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -384,7 +384,7 @@ describe Gitlab::GitAccess do
 
     def stub_git_hooks
       # Running the `pre-receive` hook is expensive, and not necessary for this test.
-      allow_any_instance_of(GitHooksService).to receive(:execute) do |service, &block|
+      allow_any_instance_of(Gitlab::Git::HooksService).to receive(:execute) do |service, &block|
         block.call(service)
       end
     end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 62585fdb35f..462e92b8b62 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -847,7 +847,7 @@ describe Repository, models: true do
 
         expect do
           repository.add_branch(user, 'new_feature', 'master')
-        end.to raise_error(GitHooksService::PreReceiveError)
+        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
       end
 
       it 'does not create the branch' do
@@ -855,7 +855,7 @@ describe Repository, models: true do
 
         expect do
           repository.add_branch(user, 'new_feature', 'master')
-        end.to raise_error(GitHooksService::PreReceiveError)
+        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
         expect(repository.find_branch('new_feature')).to be_nil
       end
     end
@@ -885,7 +885,7 @@ describe Repository, models: true do
 
     context 'when pre hooks were successful' do
       it 'runs without errors' do
-        expect_any_instance_of(GitHooksService).to receive(:execute)
+        expect_any_instance_of(Gitlab::Git::HooksService).to receive(:execute)
           .with(committer, repository, old_rev, blank_sha, 'refs/heads/feature')
 
         expect { repository.rm_branch(user, 'feature') }.not_to raise_error
@@ -906,7 +906,7 @@ describe Repository, models: true do
 
         expect do
           repository.rm_branch(user, 'feature')
-        end.to raise_error(GitHooksService::PreReceiveError)
+        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
       end
 
       it 'does not delete the branch' do
@@ -914,7 +914,7 @@ describe Repository, models: true do
 
         expect do
           repository.rm_branch(user, 'feature')
-        end.to raise_error(GitHooksService::PreReceiveError)
+        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
         expect(repository.find_branch('feature')).not_to be_nil
       end
     end
@@ -926,8 +926,8 @@ describe Repository, models: true do
 
     context 'when pre hooks were successful' do
       before do
-        service = GitHooksService.new
-        expect(GitHooksService).to receive(:new).and_return(service)
+        service = Gitlab::Git::HooksService.new
+        expect(Gitlab::Git::HooksService).to receive(:new).and_return(service)
         expect(service).to receive(:execute)
           .with(committer, repository, old_rev, new_rev, 'refs/heads/feature')
           .and_yield(service).and_return(true)
@@ -1030,7 +1030,7 @@ describe Repository, models: true do
           GitOperationService.new(committer, repository).with_branch('feature') do
             new_rev
           end
-        end.to raise_error(GitHooksService::PreReceiveError)
+        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
       end
     end
 
diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb
index cc950ae6bb3..2b4f8cd42ee 100644
--- a/spec/services/files/update_service_spec.rb
+++ b/spec/services/files/update_service_spec.rb
@@ -76,7 +76,7 @@ describe Files::UpdateService do
       let(:branch_name) { "#{project.default_branch}-new" }
 
       it 'fires hooks only once' do
-        expect(GitHooksService).to receive(:new).once.and_call_original
+        expect(Gitlab::Git::HooksService).to receive(:new).once.and_call_original
 
         subject.execute
       end
diff --git a/spec/services/git_hooks_service_spec.rb b/spec/services/git_hooks_service_spec.rb
index 3ce01a995b4..ac7be531526 100644
--- a/spec/services/git_hooks_service_spec.rb
+++ b/spec/services/git_hooks_service_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe GitHooksService do
+describe Gitlab::Git::HooksService do
   include RepoHelpers
 
   let(:user)    { create(:user) }
@@ -31,7 +31,7 @@ describe GitHooksService do
 
         expect do
           service.execute(user, project, @blankrev, @newrev, @ref)
-        end.to raise_error(GitHooksService::PreReceiveError)
+        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
       end
     end
 
@@ -43,7 +43,7 @@ describe GitHooksService do
 
         expect do
           service.execute(user, project, @blankrev, @newrev, @ref)
-        end.to raise_error(GitHooksService::PreReceiveError)
+        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
       end
     end
   end
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index e593bfeeaf7..5cfdb5372f3 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -217,7 +217,7 @@ describe MergeRequests::MergeService do
       it 'logs and saves error if there is an PreReceiveError exception' do
         error_message = 'error message'
 
-        allow(service).to receive(:repository).and_raise(GitHooksService::PreReceiveError, error_message)
+        allow(service).to receive(:repository).and_raise(Gitlab::Git::HooksService::PreReceiveError, error_message)
         allow(service).to receive(:execute_hooks)
 
         service.execute(merge_request)
diff --git a/spec/services/tags/create_service_spec.rb b/spec/services/tags/create_service_spec.rb
index 1b31ce29f7a..57013b54560 100644
--- a/spec/services/tags/create_service_spec.rb
+++ b/spec/services/tags/create_service_spec.rb
@@ -41,7 +41,7 @@ describe Tags::CreateService do
       it 'returns an error' do
         expect(repository).to receive(:add_tag)
           .with(user, 'v1.1.0', 'master', 'Foo')
-          .and_raise(GitHooksService::PreReceiveError, 'something went wrong')
+          .and_raise(Gitlab::Git::HooksService::PreReceiveError, 'something went wrong')
 
         response = service.execute('v1.1.0', 'master', 'Foo')
 
-- 
cgit v1.2.1


From c47b947a7360f21cc1438462587c6e284b40e230 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 22 Aug 2017 16:24:41 +0200
Subject: Move GitHooksService tests

---
 spec/lib/gitlab/git/hooks_service_spec.rb | 48 +++++++++++++++++++++++++++++
 spec/services/git_hooks_service_spec.rb   | 50 -------------------------------
 2 files changed, 48 insertions(+), 50 deletions(-)
 create mode 100644 spec/lib/gitlab/git/hooks_service_spec.rb
 delete mode 100644 spec/services/git_hooks_service_spec.rb

diff --git a/spec/lib/gitlab/git/hooks_service_spec.rb b/spec/lib/gitlab/git/hooks_service_spec.rb
new file mode 100644
index 00000000000..e9c0209fe3b
--- /dev/null
+++ b/spec/lib/gitlab/git/hooks_service_spec.rb
@@ -0,0 +1,48 @@
+require 'spec_helper'
+
+describe Gitlab::Git::HooksService, seed_helper: true do
+  let(:committer) { Gitlab::Git::Committer.new('Jane Doe', 'janedoe@example.com', 'user-456') }
+  let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, 'project-123') }
+  let(:service) { described_class.new }
+
+  before do
+    @blankrev = Gitlab::Git::BLANK_SHA
+    @oldrev = SeedRepo::Commit::PARENT_ID
+    @newrev = SeedRepo::Commit::ID
+    @ref = 'refs/heads/feature'
+  end
+
+  describe '#execute' do
+    context 'when receive hooks were successful' do
+      it 'calls post-receive hook' do
+        hook = double(trigger: [true, nil])
+        expect(Gitlab::Git::Hook).to receive(:new).exactly(3).times.and_return(hook)
+
+        service.execute(committer, repository, @blankrev, @newrev, @ref) { }
+      end
+    end
+
+    context 'when pre-receive hook failed' do
+      it 'does not call post-receive hook' do
+        expect(service).to receive(:run_hook).with('pre-receive').and_return([false, ''])
+        expect(service).not_to receive(:run_hook).with('post-receive')
+
+        expect do
+          service.execute(committer, repository, @blankrev, @newrev, @ref)
+        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
+      end
+    end
+
+    context 'when update hook failed' do
+      it 'does not call post-receive hook' do
+        expect(service).to receive(:run_hook).with('pre-receive').and_return([true, nil])
+        expect(service).to receive(:run_hook).with('update').and_return([false, ''])
+        expect(service).not_to receive(:run_hook).with('post-receive')
+
+        expect do
+          service.execute(committer, repository, @blankrev, @newrev, @ref)
+        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
+      end
+    end
+  end
+end
diff --git a/spec/services/git_hooks_service_spec.rb b/spec/services/git_hooks_service_spec.rb
deleted file mode 100644
index ac7be531526..00000000000
--- a/spec/services/git_hooks_service_spec.rb
+++ /dev/null
@@ -1,50 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::Git::HooksService do
-  include RepoHelpers
-
-  let(:user)    { create(:user) }
-  let(:project) { create(:project, :repository) }
-  let(:service) { described_class.new }
-
-  before do
-    @blankrev = Gitlab::Git::BLANK_SHA
-    @oldrev = sample_commit.parent_id
-    @newrev = sample_commit.id
-    @ref = 'refs/heads/feature'
-  end
-
-  describe '#execute' do
-    context 'when receive hooks were successful' do
-      it 'calls post-receive hook' do
-        hook = double(trigger: [true, nil])
-        expect(Gitlab::Git::Hook).to receive(:new).exactly(3).times.and_return(hook)
-
-        service.execute(user, project, @blankrev, @newrev, @ref) { }
-      end
-    end
-
-    context 'when pre-receive hook failed' do
-      it 'does not call post-receive hook' do
-        expect(service).to receive(:run_hook).with('pre-receive').and_return([false, ''])
-        expect(service).not_to receive(:run_hook).with('post-receive')
-
-        expect do
-          service.execute(user, project, @blankrev, @newrev, @ref)
-        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
-      end
-    end
-
-    context 'when update hook failed' do
-      it 'does not call post-receive hook' do
-        expect(service).to receive(:run_hook).with('pre-receive').and_return([true, nil])
-        expect(service).to receive(:run_hook).with('update').and_return([false, ''])
-        expect(service).not_to receive(:run_hook).with('post-receive')
-
-        expect do
-          service.execute(user, project, @blankrev, @newrev, @ref)
-        end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
-      end
-    end
-  end
-end
-- 
cgit v1.2.1


From da769135fede9dececee8ab3e1f6951de7361e7f Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 22 Aug 2017 16:27:15 +0200
Subject: Rubocop whitespace

---
 lib/gitlab/git/committer.rb     |  2 +-
 lib/gitlab/git/hooks_service.rb | 15 +++++++--------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/lib/gitlab/git/committer.rb b/lib/gitlab/git/committer.rb
index ef3576eaa1c..1f4bcf7a3a0 100644
--- a/lib/gitlab/git/committer.rb
+++ b/lib/gitlab/git/committer.rb
@@ -2,7 +2,7 @@ module Gitlab
   module Git
     class Committer
       attr_reader :name, :email, :gl_id
-      
+
       def self.from_user(user)
         new(user.name, user.email, Gitlab::GlId.gl_id(user))
       end
diff --git a/lib/gitlab/git/hooks_service.rb b/lib/gitlab/git/hooks_service.rb
index 1da92fcc0e2..ea8a87a1290 100644
--- a/lib/gitlab/git/hooks_service.rb
+++ b/lib/gitlab/git/hooks_service.rb
@@ -2,31 +2,31 @@ module Gitlab
   module Git
     class HooksService
       PreReceiveError = Class.new(StandardError)
-    
+
       attr_accessor :oldrev, :newrev, :ref
-    
+
       def execute(committer, repository, oldrev, newrev, ref)
         @repository = repository
         @gl_id      = committer.gl_id
         @oldrev     = oldrev
         @newrev     = newrev
         @ref        = ref
-    
+
         %w(pre-receive update).each do |hook_name|
           status, message = run_hook(hook_name)
-    
+
           unless status
             raise PreReceiveError, message
           end
         end
-    
+
         yield(self).tap do
           run_hook('post-receive')
         end
       end
-    
+
       private
-    
+
       def run_hook(name)
         hook = Gitlab::Git::Hook.new(name, @repository)
         hook.trigger(@gl_id, oldrev, newrev, ref)
@@ -34,4 +34,3 @@ module Gitlab
     end
   end
 end
-
-- 
cgit v1.2.1


From bc97931e2def4efee8418ce62ebc4818a31cf163 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Tue, 22 Aug 2017 16:27:18 +0200
Subject: Fix Hook.new call sites in tests

---
 spec/features/projects/import_export/import_file_spec.rb | 2 +-
 spec/lib/gitlab/import_export/repo_restorer_spec.rb      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb
index 9f2c86923b7..2eb6fab129d 100644
--- a/spec/features/projects/import_export/import_file_spec.rb
+++ b/spec/features/projects/import_export/import_file_spec.rb
@@ -99,6 +99,6 @@ feature 'Import/Export - project import integration test', js: true do
   end
 
   def project_hook_exists?(project)
-    Gitlab::Git::Hook.new('post-receive', project).exists?
+    Gitlab::Git::Hook.new('post-receive', project.repository.raw_repository).exists?
   end
 end
diff --git a/spec/lib/gitlab/import_export/repo_restorer_spec.rb b/spec/lib/gitlab/import_export/repo_restorer_spec.rb
index 2786bc92fe5..c49af602a01 100644
--- a/spec/lib/gitlab/import_export/repo_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/repo_restorer_spec.rb
@@ -34,7 +34,7 @@ describe Gitlab::ImportExport::RepoRestorer do
     it 'has the webhooks' do
       restorer.restore
 
-      expect(Gitlab::Git::Hook.new('post-receive', project)).to exist
+      expect(Gitlab::Git::Hook.new('post-receive', project.repository.raw_repository)).to exist
     end
   end
 end
-- 
cgit v1.2.1


From 961da7d0a7024628ec87c02c158a147dd64b3317 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Wed, 23 Aug 2017 17:23:28 +0200
Subject: Add tests for Committer#==

---
 spec/lib/gitlab/git/committer_spec.rb | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 spec/lib/gitlab/git/committer_spec.rb

diff --git a/spec/lib/gitlab/git/committer_spec.rb b/spec/lib/gitlab/git/committer_spec.rb
new file mode 100644
index 00000000000..b0ddbb51449
--- /dev/null
+++ b/spec/lib/gitlab/git/committer_spec.rb
@@ -0,0 +1,22 @@
+require 'spec_helper'
+
+describe Gitlab::Git::Committer do
+  let(:name) { 'Jane Doe' }
+  let(:email) { 'janedoe@example.com' }
+  let(:gl_id) { 'user-123' }
+
+  subject { described_class.new(name, email, gl_id) }
+
+  describe '#==' do
+    def eq_other(name, email, gl_id)
+      eq(described_class.new(name, email, gl_id))
+    end
+
+    it { expect(subject).to eq_other(name, email, gl_id) }
+
+    it { expect(subject).not_to eq_other(nil, nil, nil) }
+    it { expect(subject).not_to eq_other(name + 'x', email, gl_id) }
+    it { expect(subject).not_to eq_other(name, email + 'x', gl_id) }
+    it { expect(subject).not_to eq_other(name, email, gl_id + 'x') }
+  end
+end
-- 
cgit v1.2.1


From a1a914994f6376b03d95e9d4b05dea422e8610f9 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Thu, 24 Aug 2017 14:05:24 +0200
Subject: Avoid committer = lines

---
 app/models/repository.rb              | 27 +++++++++------------------
 app/services/git_operation_service.rb |  5 ++---
 2 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 062f532233f..59f913c7fb4 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -164,8 +164,7 @@ class Repository
 
     return false unless newrev
 
-    committer = Gitlab::Git::Committer.from_user(user)
-    GitOperationService.new(committer, self).add_branch(branch_name, newrev)
+    GitOperationService.new(user, self).add_branch(branch_name, newrev)
 
     after_create_branch
     find_branch(branch_name)
@@ -177,8 +176,7 @@ class Repository
 
     return false unless newrev
 
-    committer = Gitlab::Git::Committer.from_user(user)
-    GitOperationService.new(committer, self).add_tag(tag_name, newrev, options)
+    GitOperationService.new(user, self).add_tag(tag_name, newrev, options)
 
     find_tag(tag_name)
   end
@@ -187,8 +185,7 @@ class Repository
     before_remove_branch
     branch = find_branch(branch_name)
 
-    committer = Gitlab::Git::Committer.from_user(user)
-    GitOperationService.new(committer, self).rm_branch(branch)
+    GitOperationService.new(user, self).rm_branch(branch)
 
     after_remove_branch
     true
@@ -198,8 +195,7 @@ class Repository
     before_remove_tag
     tag = find_tag(tag_name)
 
-    committer = Gitlab::Git::Committer.from_user(user)
-    GitOperationService.new(committer, self).rm_tag(tag)
+    GitOperationService.new(user, self).rm_tag(tag)
 
     after_remove_tag
     true
@@ -767,8 +763,7 @@ class Repository
     author_email: nil, author_name: nil,
     start_branch_name: nil, start_project: project)
 
-    committer = Gitlab::Git::Committer.from_user(user)
-    GitOperationService.new(committer, self).with_branch(
+    GitOperationService.new(user, self).with_branch(
       branch_name,
       start_branch_name: start_branch_name,
       start_project: start_project) do |start_commit|
@@ -824,8 +819,7 @@ class Repository
   end
 
   def merge(user, source, merge_request, options = {})
-    committer = Gitlab::Git::Committer.from_user(user)
-    GitOperationService.new(committer, self).with_branch(
+    GitOperationService.new(user, self).with_branch(
       merge_request.target_branch) do |start_commit|
       our_commit = start_commit.sha
       their_commit = source
@@ -852,8 +846,7 @@ class Repository
   def revert(
     user, commit, branch_name,
     start_branch_name: nil, start_project: project)
-    committer = Gitlab::Git::Committer.from_user(user)
-    GitOperationService.new(committer, self).with_branch(
+    GitOperationService.new(user, self).with_branch(
       branch_name,
       start_branch_name: start_branch_name,
       start_project: start_project) do |start_commit|
@@ -876,8 +869,7 @@ class Repository
   def cherry_pick(
     user, commit, branch_name,
     start_branch_name: nil, start_project: project)
-    committer = Gitlab::Git::Committer.from_user(user)
-    GitOperationService.new(committer, self).with_branch(
+    GitOperationService.new(user, self).with_branch(
       branch_name,
       start_branch_name: start_branch_name,
       start_project: start_project) do |start_commit|
@@ -902,8 +894,7 @@ class Repository
   end
 
   def resolve_conflicts(user, branch_name, params)
-    committer = Gitlab::Git::Committer.from_user(user)
-    GitOperationService.new(committer, self).with_branch(branch_name) do
+    GitOperationService.new(user, self).with_branch(branch_name) do
       committer = user_to_committer(user)
 
       create_commit(params.merge(author: committer, committer: committer))
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 6a983566526..6b7a56e6922 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -2,10 +2,9 @@ class GitOperationService
   attr_reader :committer, :repository
 
   def initialize(committer, new_repository)
-    if committer && !committer.is_a?(Gitlab::Git::Committer)
-      raise "expected Gitlab::Git::Committer, got #{committer.inspect}"
-    end
+    committer = Gitlab::Git::Committer.from_user(committer) if committer.is_a?(User)
     @committer = committer
+
     @repository = new_repository
   end
 
-- 
cgit v1.2.1