Represent DiffRefs as proper class instead of tuple array

Author: DouweM

app/controllers/projects/blob_controller.rb

@@ -57,7 +57,7 @@ def preview
     diffy = Diffy::Diff.new(@blob.data, @content, diff: '-U 3', include_diff_info: true)
     diff_lines = diffy.diff.scan(/.*\n/)[2..-1]
     diff_lines = Gitlab::Diff::Parser.new.parse(diff_lines)
-    @diff_lines = Gitlab::Diff::Highlight.new(diff_lines).highlight
+    @diff_lines = Gitlab::Diff::Highlight.new(diff_lines, repository: @repository).highlight
 
     render layout: false
   end

app/controllers/projects/commit_controller.rb

@@ -121,7 +121,6 @@ def define_show_vars
     opts[:ignore_whitespace_change] = true if params[:format] == 'diff'
 
     @diffs = commit.diffs(opts)
-    @diff_refs = [commit.parent || commit, commit]
     @notes_count = commit.notes.count
 
     @statuses = CommitStatus.where(pipeline: pipelines)

app/controllers/projects/compare_controller.rb

@@ -14,14 +14,22 @@ def index
 
   def show
     compare = CompareService.new.
-      execute(@project, @head_ref, @project, @base_ref, diff_options)
+      execute(@project, @head_ref, @project, @start_ref, diff_options)
 
     if compare
       @commits = Commit.decorate(compare.commits, @project)
+
+      @start_commit = @project.commit(@start_ref)
       @commit = @project.commit(@head_ref)
-      @base_commit = @project.merge_base_commit(@base_ref, @head_ref)
+      @base_commit = @project.merge_base_commit(@start_ref, @head_ref)
+
       @diffs = compare.diffs(diff_options)
-      @diff_refs = [@base_commit, @commit]
+      @diff_refs = Gitlab::Diff::DiffRefs.new(
+        base_sha: @base_commit.try(:sha),
+        start_sha: @start_commit.try(:sha),
+        head_sha: @commit.try(:sha)
+      )
+
       @diff_notes_disabled = true
       @grouped_diff_notes = {}
     end
@@ -35,12 +43,12 @@ def create
   private
 
   def assign_ref_vars
-    @base_ref = Addressable::URI.unescape(params[:from])
+    @start_ref = Addressable::URI.unescape(params[:from])
     @ref = @head_ref = Addressable::URI.unescape(params[:to])
   end
 
   def merge_request
     @merge_request ||= @project.merge_requests.opened.
-      find_by(source_project: @project, source_branch: @head_ref, target_branch: @base_ref)
+      find_by(source_project: @project, source_branch: @head_ref, target_branch: @start_ref)
   end
 end

app/helpers/diff_helper.rb

@@ -30,8 +30,8 @@ def diff_options
     options
   end
 
-  def safe_diff_files(diffs, diff_refs)
-    diffs.decorate! { |diff| Gitlab::Diff::File.new(diff, diff_refs) }
+  def safe_diff_files(diffs, diff_refs: nil, repository: nil)
+    diffs.decorate! { |diff| Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
   end
 
   def generate_line_code(file_path, line)

app/models/commit.rb

@@ -214,6 +214,13 @@ def short_id
     @raw.short_id(7)
   end
 
+  def diff_refs
+    Gitlab::Diff::DiffRefs.new(
+      base_sha: self.parent_id || self.sha,
+      head_sha: self.sha
+    )
+  end
+
   def pipelines
     @pipeline ||= project.pipelines.where(sha: sha)
   end

app/models/merge_request.rb

@@ -249,6 +249,16 @@ def source_branch_sha
     source_branch_head.try(:sha)
   end
 
+  def diff_refs
+    return nil unless diff_start_commit || diff_base_commit
+
+    Gitlab::Diff::DiffRefs.new(
+      base_sha:  diff_base_sha,
+      start_sha: diff_start_sha,
+      head_sha:  diff_head_sha
+    )
+  end
+
   def validate_branches
     if target_project == source_project && target_branch == source_branch
       errors.add :branch_conflict, "You can not use same project/branch for source and target"
@@ -622,12 +632,6 @@ def diverged_from_target_branch?
   end
 
   def pipeline
-  end
-
-  def diff_refs
-    return nil unless diff_base_commit
-
-    [diff_base_commit, last_commit]
     @pipeline ||= source_project.pipeline(diff_head_sha, source_branch) if diff_head_sha && source_project
   end
 

app/views/projects/commit/show.html.haml

@@ -7,8 +7,7 @@
   = render "ci_menu"
 - else
   %div.block-connector
-= render "projects/diffs/diffs", diffs: @diffs, project: @project,
-         diff_refs: @diff_refs
+= render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @commit.diff_refs
 = render "projects/notes/notes_with_form"
 - if can_collaborate_with_project?
   - %w(revert cherry-pick).each do |type|

app/views/projects/diffs/_diffs.html.haml

@@ -2,7 +2,7 @@
 - if diff_view == 'parallel'
   - fluid_layout true
 
-- diff_files = safe_diff_files(diffs, diff_refs)
+- diff_files = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository)
 
 .content-block.oneline-block.files-changed
   .inline-parallel-buttons

app/views/projects/diffs/_image.html.haml

@@ -1,9 +1,8 @@
 - diff = diff_file.diff
-- file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(@commit.id, diff.new_path))
+- file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(diff_file.new_ref, diff.new_path))
 // diff_refs will be nil for orphaned commits (e.g. first commit in repo)
-- if diff_refs
-  - old_commit_id = diff_refs.first.id
-  - old_file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(old_commit_id, diff.old_path))
+- if diff_file.old_ref
+  - old_file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(diff_file.old_ref, diff.old_path))
 
 - if diff.renamed_file || diff.new_file || diff.deleted_file
   .image
@@ -16,7 +15,7 @@
     %div.two-up.view
       %span.wrap
         .frame.deleted
-          %a{href: namespace_project_blob_path(@project.namespace, @project, tree_join(old_commit_id, diff.old_path))}
+          %a{href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_file.old_ref, diff.old_path))}
             %img{src: old_file_raw_path}
         %p.image-info.hide
           %span.meta-filesize= "#{number_to_human_size old_file.size}"
@@ -28,7 +27,7 @@
           %span.meta-height
       %span.wrap
         .frame.added
-          %a{href: namespace_project_blob_path(@project.namespace, @project, tree_join(@commit.id, diff.new_path))}
+          %a{href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_file.new_ref, diff.new_path))}
             %img{src: file_raw_path}
         %p.image-info.hide
           %span.meta-filesize= "#{number_to_human_size file.size}"

app/workers/emails_on_push_worker.rb

@@ -28,18 +28,30 @@ def perform(project_id, recipients, push_data, options = {})
         :push
       end
 
+    merge_base_sha = project.merge_base_commit(before_sha, after_sha).try(:sha)
+
     diff_refs = nil
     compare = nil
     reverse_compare = false
     if action == :push
       compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha)
-      diff_refs = [project.merge_base_commit(before_sha, after_sha), project.commit(after_sha)]
+
+      diff_refs = Gitlab::Diff::DiffRefs.new(
+        base_sha: merge_base_sha,
+        start_sha: before_sha,
+        head_sha: after_sha
+      )
 
       return false if compare.same
 
       if compare.commits.empty?
         compare = Gitlab::Git::Compare.new(project.repository.raw_repository, after_sha, before_sha)
-        diff_refs = [project.merge_base_commit(after_sha, before_sha), project.commit(before_sha)]
+
+        diff_refs = Gitlab::Diff::DiffRefs.new(
+          base_sha: merge_base_sha,
+          start_sha: after_sha,
+          head_sha: before_sha
+        )
 
         reverse_compare = true
 

lib/gitlab/diff/diff_refs.rb

@@ -0,0 +1,26 @@
+module Gitlab
+  module Diff
+    class DiffRefs
+      attr_reader :base_sha
+      attr_reader :start_sha
+      attr_reader :head_sha
+
+      def initialize(base_sha:, start_sha: base_sha, head_sha:)
+        @base_sha = base_sha
+        @start_sha = start_sha
+        @head_sha = head_sha
+      end
+
+      def ==(other)
+        other.is_a?(self.class) &&
+          base_sha == other.base_sha &&
+          start_sha == other.start_sha &&
+          head_sha == other.head_sha
+      end
+
+      def complete?
+        start_sha && head_sha
+      end
+    end
+  end
+end

lib/gitlab/diff/file.rb

@@ -1,39 +1,36 @@
 module Gitlab
   module Diff
     class File
-      attr_reader :diff, :diff_refs
+      attr_reader :diff, :repository, :diff_refs
 
       delegate :new_file, :deleted_file, :renamed_file,
         :old_path, :new_path, to: :diff, prefix: false
 
-      def initialize(diff, diff_refs)
+      def initialize(diff, repository:, diff_refs: nil)
         @diff = diff
+        @repository = repository
         @diff_refs = diff_refs
       end
 
       def old_ref
-        diff_refs[0] if diff_refs
+        diff_refs.try(:base_sha)
       end
 
       def new_ref
-        diff_refs[1] if diff_refs
+        diff_refs.try(:head_sha)
       end
 
-      # Array of Gitlab::DIff::Line objects
+      # Array of Gitlab::Diff::Line objects
       def diff_lines
-        @lines ||= parser.parse(raw_diff.each_line).to_a
-      end
-
-      def too_large?
-        diff.too_large?
+        @lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a
       end
 
       def highlighted_diff_lines
-        Gitlab::Diff::Highlight.new(self).highlight
+        @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight
       end
 
       def parallel_diff_lines
-        Gitlab::Diff::ParallelDiff.new(self).parallelize
+        @parallel_diff_lines ||= Gitlab::Diff::ParallelDiff.new(self).parallelize
       end
 
       def mode_changed?

lib/gitlab/diff/highlight.rb

@@ -1,11 +1,13 @@
 module Gitlab
   module Diff
     class Highlight
-      attr_reader :diff_file, :diff_lines, :raw_lines
+      attr_reader :diff_file, :diff_lines, :raw_lines, :repository
 
       delegate :old_path, :new_path, :old_ref, :new_ref, to: :diff_file, prefix: :diff
 
-      def initialize(diff_lines)
+      def initialize(diff_lines, repository: nil)
+        @repository = repository
+
         if diff_lines.is_a?(Gitlab::Diff::File)
           @diff_file = diff_lines
           @diff_lines = @diff_file.diff_lines
@@ -19,7 +21,7 @@ def highlight
         @diff_lines.map.with_index do |diff_line, i|
           diff_line = diff_line.dup
           # ignore highlighting for "match" lines
-          next diff_line if diff_line.type == 'match' || diff_line.type == 'nonewline'
+          next diff_line if diff_line.meta?
 
           rich_line = highlight_line(diff_line) || diff_line.text
 
@@ -70,7 +72,7 @@ def processing_args(version)
         ref  = send("diff_#{version}_ref")
         path = send("diff_#{version}_path")
 
-        [ref.project.repository, ref.id, path]
+        [self.repository, ref, path]
       end
     end
   end

lib/gitlab/email/message/repository_push.rb

@@ -33,11 +33,15 @@ def author
         end
 
         def commits
-          @commits ||= (Commit.decorate(compare.commits, project) if compare)
+          return unless compare
+
+          @commits ||= Commit.decorate(compare.commits, project)
         end
 
         def diffs
-          @diffs ||= (safe_diff_files(compare.diffs(max_files: 30), diff_refs) if compare)
+          return unless compare
+          
+          @diffs ||= safe_diff_files(compare.diffs(max_files: 30), diff_refs: diff_refs, repository: project.repository)
         end
 
         def diffs_count

lib/gitlab/workhorse.rb

@@ -38,12 +38,10 @@ def send_git_archive(repository, ref:, format:)
       end
 
       def send_git_diff(repository, diff_refs)
-        from, to = diff_refs
-
         params = {
           'RepoPath'  => repository.path_to_repo,
-          'ShaFrom'   => from.sha,
-          'ShaTo'     => to.sha
+          'ShaFrom'   => diff_refs.start_sha,
+          'ShaTo'     => diff_refs.head_sha
         }
 
         [

spec/helpers/diff_helper_spec.rb

@@ -9,7 +9,7 @@
   let(:diffs) { commit.diffs }
   let(:diff) { diffs.first }
   let(:diff_refs) { [commit.parent, commit] }
-  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs) }
+  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
 
   describe 'diff_view' do
     it 'returns a valid value when cookie is set' do

spec/lib/gitlab/diff/file_spec.rb

@@ -6,7 +6,7 @@
   let(:project) { create(:project) }
   let(:commit) { project.commit(sample_commit.id) }
   let(:diff) { commit.diffs.first }
-  let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent, commit]) }
+  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
 
   describe :diff_lines do
     let(:diff_lines) { diff_file.diff_lines }

spec/lib/gitlab/diff/highlight_spec.rb

@@ -6,11 +6,11 @@
   let(:project) { create(:project) }
   let(:commit) { project.commit(sample_commit.id) }
   let(:diff) { commit.diffs.first }
-  let(:diff_file) { Gitlab::Diff::File.new(diff, [commit.parent, commit]) }
+  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
 
   describe '#highlight' do
     context "with a diff file" do
-      let(:subject) { Gitlab::Diff::Highlight.new(diff_file).highlight }
+      let(:subject) { Gitlab::Diff::Highlight.new(diff_file, repository: project.repository).highlight }
 
       it 'should return Gitlab::Diff::Line elements' do
         expect(subject.first).to be_an_instance_of(Gitlab::Diff::Line)
@@ -41,7 +41,7 @@
     end
 
     context "with diff lines" do
-      let(:subject) { Gitlab::Diff::Highlight.new(diff_file.diff_lines).highlight }
+      let(:subject) { Gitlab::Diff::Highlight.new(diff_file.diff_lines, repository: project.repository).highlight }
 
       it 'should return Gitlab::Diff::Line elements' do
         expect(subject.first).to be_an_instance_of(Gitlab::Diff::Line)

spec/lib/gitlab/diff/parallel_diff_spec.rb

@@ -8,8 +8,7 @@
   let(:commit) { project.commit(sample_commit.id) }
   let(:diffs) { commit.diffs }
   let(:diff) { diffs.first }
-  let(:diff_refs) { [commit.parent, commit] }
-  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs) }
+  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) }
   subject { described_class.new(diff_file) }
 
   let(:parallel_diff_result_array) { YAML.load_file("#{Rails.root}/spec/fixtures/parallel_diff_result.yml") }