From d03c605bd4a128d45179dd05f117a78aab7af6be Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 14 Dec 2016 02:29:35 +0800
Subject: [PATCH] Unify parameters and callback after hooks
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19747856
---
app/services/git_operation_service.rb | 34 +++++++++----------
lib/gitlab/git.rb | 2 +-
spec/models/repository_spec.rb | 7 ++--
.../git_garbage_collect_worker_spec.rb | 2 +-
4 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 9a052f952cf..68b28231595 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -11,7 +11,7 @@ class GitOperationService
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
oldrev = Gitlab::Git::BLANK_SHA
- with_hooks_and_update_ref(ref, oldrev, newrev)
+ update_ref_in_hooks(ref, newrev, oldrev)
end
def rm_branch(branch)
@@ -19,14 +19,14 @@ class GitOperationService
oldrev = branch.dereferenced_target.id
newrev = Gitlab::Git::BLANK_SHA
- with_hooks_and_update_ref(ref, oldrev, newrev)
+ update_ref_in_hooks(ref, newrev, oldrev)
end
def add_tag(tag_name, newrev, options = {})
ref = Gitlab::Git::TAG_REF_PREFIX + tag_name
oldrev = Gitlab::Git::BLANK_SHA
- with_hooks(ref, oldrev, newrev) do |service|
+ with_hooks(ref, newrev, oldrev) do |service|
raw_tag = repository.rugged.tags.create(tag_name, newrev, options)
service.newrev = raw_tag.target_id
end
@@ -82,25 +82,23 @@ class GitOperationService
end
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
- with_hooks_and_update_ref(ref, oldrev, newrev) do
- # If repo was empty expire cache
- repository.after_create if was_empty
- repository.after_create_branch if was_empty ||
- Gitlab::Git.blank_ref?(oldrev)
- end
+ update_ref_in_hooks(ref, newrev, oldrev)
+
+ # If repo was empty expire cache
+ repository.after_create if was_empty
+ repository.after_create_branch if was_empty ||
+ Gitlab::Git.blank_ref?(oldrev)
newrev
end
- def with_hooks_and_update_ref(ref, oldrev, newrev)
- with_hooks(ref, oldrev, newrev) do |service|
- update_ref!(ref, newrev, oldrev)
-
- yield(service) if block_given?
+ def update_ref_in_hooks(ref, newrev, oldrev)
+ with_hooks(ref, newrev, oldrev) do
+ update_ref(ref, newrev, oldrev)
end
end
- def with_hooks(ref, oldrev, newrev)
+ def with_hooks(ref, newrev, oldrev)
result = nil
GitHooksService.new.execute(
@@ -116,7 +114,7 @@ class GitOperationService
result
end
- def update_ref!(name, newrev, oldrev)
+ def update_ref(ref, newrev, oldrev)
# We use 'git update-ref' because libgit2/rugged currently does not
# offer 'compare and swap' ref updates. Without compare-and-swap we can
# (and have!) accidentally reset the ref to an earlier state, clobbering
@@ -125,12 +123,12 @@ class GitOperationService
_, status = Gitlab::Popen.popen(
command,
repository.path_to_repo) do |stdin|
- stdin.write("update #{name}\x00#{newrev}\x00#{oldrev}\x00")
+ stdin.write("update #{ref}\x00#{newrev}\x00#{oldrev}\x00")
end
unless status.zero?
raise Repository::CommitError.new(
- "Could not update branch #{name.sub('refs/heads/', '')}." \
+ "Could not update branch #{Gitlab::Git.branch_name(ref)}." \
" Please refresh and try again.")
end
end
diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb
index 3cd515e4a3a..d3df3f1bca1 100644
--- a/lib/gitlab/git.rb
+++ b/lib/gitlab/git.rb
@@ -6,7 +6,7 @@ module Gitlab
class << self
def ref_name(ref)
- ref.gsub(/\Arefs\/(tags|heads)\//, '')
+ ref.sub(/\Arefs\/(tags|heads)\//, '')
end
def branch_name(ref)
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index a61d7f0c76d..65e96351033 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -829,7 +829,6 @@ describe Repository, models: true do
context 'when target branch is different from source branch' do
before do
allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, ''])
- allow(repository).to receive(:update_ref!)
end
it 'expires branch cache' do
@@ -1474,16 +1473,16 @@ describe Repository, models: true do
end
end
- describe '#update_ref!' do
+ describe '#update_ref' do
it 'can create a ref' do
- GitOperationService.new(nil, repository).send(:update_ref!, 'refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+ GitOperationService.new(nil, repository).send(:update_ref, 'refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
expect(repository.find_branch('foobar')).not_to be_nil
end
it 'raises CommitError when the ref update fails' do
expect do
- GitOperationService.new(nil, repository).send(:update_ref!, 'refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+ GitOperationService.new(nil, repository).send(:update_ref, 'refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
end.to raise_error(Repository::CommitError)
end
end
diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb
index 2b31efbf631..5ef8cf1105b 100644
--- a/spec/workers/git_garbage_collect_worker_spec.rb
+++ b/spec/workers/git_garbage_collect_worker_spec.rb
@@ -108,7 +108,7 @@ describe GitGarbageCollectWorker do
parents: [old_commit],
)
GitOperationService.new(nil, project.repository).send(
- :update_ref!,
+ :update_ref,
"refs/heads/#{SecureRandom.hex(6)}",
new_commit_sha,
Gitlab::Git::BLANK_SHA
--
2.30.9