From 99ac0935271b1e99f4512e496104219045f1018e Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 5 Jan 2017 01:52:21 +0800
Subject: [PATCH] Introduce Repository#with_repo_branch_commit

We merge repository checks inside it so we don't have to
check it on the call site, and we could also load the commit
for the caller. This greatly reduce code duplication.

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_20572919
---
 app/models/repository.rb              | 25 ++++++++++++++++---------
 app/services/compare_service.rb       | 18 ++++++------------
 app/services/git_operation_service.rb | 18 ++++++------------
 3 files changed, 28 insertions(+), 33 deletions(-)

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 46995bdcb33..b1a789492d3 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1063,19 +1063,26 @@ class Repository
     Gitlab::Popen.popen(args, path_to_repo).first.lines.map(&:strip)
   end
 
-  def with_tmp_ref(source_repository, source_branch_name)
-    tmp_ref = "refs/tmp/#{SecureRandom.hex}/head"
+  def with_repo_branch_commit(source_repository, source_branch_name)
+    branch_name_or_sha =
+      if source_repository == self
+        source_branch_name
+      else
+        tmp_ref = "refs/tmp/#{SecureRandom.hex}/head"
 
-    fetch_ref(
-      source_repository.path_to_repo,
-      "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch_name}",
-      tmp_ref
-    )
+        fetch_ref(
+          source_repository.path_to_repo,
+          "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch_name}",
+          tmp_ref
+        )
 
-    yield
+        source_repository.commit(source_branch_name).sha
+      end
+
+    yield(commit(branch_name_or_sha))
 
   ensure
-    rugged.references.delete(tmp_ref)
+    rugged.references.delete(tmp_ref) if tmp_ref
   end
 
   def fetch_ref(source_path, source_ref, target_ref)
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index 31c371c4b34..d3d613661a6 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -11,19 +11,13 @@ class CompareService
   end
 
   def execute(target_project, target_branch, straight: false)
-    source_sha = source_project.repository.
-      commit(source_branch_name).try(:sha)
-
-    return unless source_sha
-
     # If compare with other project we need to fetch ref first
-    if target_project == source_project
-      compare(source_sha, target_project, target_branch, straight)
-    else
-      target_project.repository.with_tmp_ref(
-        source_project.repository, source_branch_name) do
-        compare(source_sha, target_project, target_branch, straight)
-      end
+    target_project.repository.with_repo_branch_commit(
+      source_project.repository,
+      source_branch_name) do |commit|
+      break unless commit
+
+      compare(commit.sha, target_project, target_branch, straight)
     end
   end
 
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 00c85112873..ed9822cfee6 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -43,23 +43,17 @@ class GitOperationService
   def with_branch(
     branch_name,
     source_branch_name: nil,
-    source_project: repository.project)
+    source_project: repository.project,
+    &block)
 
     check_with_branch_arguments!(
       branch_name, source_branch_name, source_project)
 
-    source_commit = source_project.repository.find_branch(
-      source_branch_name || branch_name).try(:dereferenced_target)
-
     update_branch_with_hooks(branch_name) do
-      if repository.project == source_project
-        yield(source_commit)
-      else
-        repository.with_tmp_ref(
-          source_project.repository, source_branch_name) do
-            yield(source_commit)
-          end
-      end
+      repository.with_repo_branch_commit(
+        source_project.repository,
+        source_branch_name || branch_name,
+        &block)
     end
   end