From 9b9309329207449ef022bdcf06bff5f8eae36032 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer 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