Unify parameters and callback after hooks

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19747856

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