Decouple GitOperationService from User

5 files changed, 60 insertions, 26 deletions

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