Refactor merge request diff

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

app/models/merge_request_diff.rb

@@ -8,7 +8,8 @@ class MergeRequestDiff < ActiveRecord::Base
 
   belongs_to :merge_request
 
-  delegate :source_branch_sha, :target_branch_sha, :target_branch, :source_branch, to: :merge_request, prefix: nil
+  delegate :source_branch_sha, :target_branch_sha,
+    :target_branch, :source_branch, to: :merge_request, prefix: nil
 
   state_machine :state, initial: :empty do
     state :collected
@@ -24,9 +25,16 @@ class MergeRequestDiff < ActiveRecord::Base
   serialize :st_commits
   serialize :st_diffs
 
+  after_initialize :set_diff_range
   after_create :reload_content, unless: :importing?
   after_save :keep_around_commits, unless: :importing?
 
+  def set_diff_range
+    self.start_commit_sha ||= target_branch_sha
+    self.head_commit_sha  ||= source_branch_sha
+    self.base_commit_sha  ||= branch_base_sha
+  end
+
   def reload_content
     reload_commits
     reload_diffs
@@ -41,8 +49,8 @@ class MergeRequestDiff < ActiveRecord::Base
       @diffs_no_whitespace ||= begin
         compare = Gitlab::Git::Compare.new(
           repository.raw_repository,
-          self.start_commit_sha || self.target_branch_sha,
-          self.head_commit_sha || self.source_branch_sha,
+          start_commit_sha,
+          head_commit_sha
         )
         compare.diffs(options)
       end
@@ -65,35 +73,21 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def base_commit
-    return unless self.base_commit_sha
+    return unless base_commit_sha
 
-    project.commit(self.base_commit_sha)
+    project.commit(base_commit_sha)
   end
 
   def start_commit
-    return unless self.start_commit_sha
+    return unless start_commit_sha
 
-    project.commit(self.start_commit_sha)
+    project.commit(start_commit_sha)
   end
 
   def head_commit
-    return last_commit unless self.head_commit_sha
-
-    project.commit(self.head_commit_sha)
-  end
-
-  def compare
-    @compare ||=
-      begin
-        # Update ref for merge request
-        merge_request.fetch_ref
+    return last_commit unless head_commit_sha
 
-        Gitlab::Git::Compare.new(
-          repository.raw_repository,
-          self.target_branch_sha,
-          self.source_branch_sha
-        )
-      end
+    project.commit(head_commit_sha)
   end
 
   def diff_refs
@@ -108,16 +102,18 @@ class MergeRequestDiff < ActiveRecord::Base
 
   private
 
-  # Collect array of Git::Commit objects
-  # between target and source branches
-  def unmerged_commits
-    commits = compare.commits
+  def compare
+    @compare ||=
+      begin
+        # Update ref for merge request
+        merge_request.fetch_ref
 
-    if commits.present?
-      commits = Commit.decorate(commits, merge_request.source_project).reverse
+        Gitlab::Git::Compare.new(
+          repository.raw_repository,
+          start_commit_sha,
+          head_commit_sha
+        )
       end
-
-    commits
   end
 
   def dump_commits(commits)
@@ -133,21 +129,16 @@ class MergeRequestDiff < ActiveRecord::Base
   def reload_commits
     new_attributes = {}
 
-    commit_objects = unmerged_commits
+    commits = compare.commits
 
-    if commit_objects.present?
-      new_attributes[:st_commits] = dump_commits(commit_objects)
+    if commits.present?
+      commits = Commit.decorate(commits, merge_request.source_project).reverse
+      new_attributes[:st_commits] = dump_commits(commits)
     end
 
     update_columns_serialized(new_attributes)
   end
 
-  # Collect array of Git::Diff objects
-  # between target and source branches
-  def unmerged_diffs
-    compare.diffs(Commit.max_diff_options)
-  end
-
   def dump_diffs(diffs)
     if diffs.respond_to?(:map)
       diffs.map(&:to_hash)
@@ -177,7 +168,7 @@ class MergeRequestDiff < ActiveRecord::Base
     if commits.size.zero?
       new_attributes[:state] = :empty
     else
-      diff_collection = unmerged_diffs
+      diff_collection = compare.diffs(Commit.max_diff_options)
 
       if diff_collection.overflow?
         # Set our state to 'overflow' to make the #empty? and #collected?
@@ -195,10 +186,6 @@ class MergeRequestDiff < ActiveRecord::Base
 
     new_attributes[:st_diffs] = new_diffs
 
-    new_attributes[:start_commit_sha] = self.target_branch_sha
-    new_attributes[:head_commit_sha] = self.source_branch_sha
-    new_attributes[:base_commit_sha] = branch_base_sha
-
     update_columns_serialized(new_attributes)
 
     keep_around_commits
@@ -213,9 +200,9 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def branch_base_commit
-    return unless self.source_branch_sha && self.target_branch_sha
+    return unless source_branch_sha && target_branch_sha
 
-    project.merge_base_commit(self.source_branch_sha, self.target_branch_sha)
+    project.merge_base_commit(source_branch_sha, target_branch_sha)
   end
 
   def branch_base_sha
@@ -254,8 +241,8 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def keep_around_commits
-    repository.keep_around(target_branch_sha)
-    repository.keep_around(source_branch_sha)
-    repository.keep_around(branch_base_sha)
+    repository.keep_around(start_commit_sha)
+    repository.keep_around(head_commit_sha)
+    repository.keep_around(base_commit_sha)
   end
 end