Fix Diff::Position#diff_file for positions on straight diffs

Author: DouweM

app/services/compare_service.rb

@@ -17,18 +17,18 @@ def execute(target_project, target_branch, straight: false)
       start_branch_name) do |commit|
       break unless commit
 
-      compare(commit.sha, target_project, target_branch, straight)
+      compare(commit.sha, target_project, target_branch, straight: straight)
     end
   end
 
   private
 
-  def compare(source_sha, target_project, target_branch, straight)
+  def compare(source_sha, target_project, target_branch, straight:)
     raw_compare = Gitlab::Git::Compare.new(
       target_project.repository.raw_repository,
       target_branch,
       source_sha,
-      straight
+      straight: straight
     )
 
     Compare.new(raw_compare, target_project, straight: straight)

lib/gitlab/diff/diff_refs.rb

@@ -37,6 +37,16 @@ def hash
       def complete?
         start_sha && head_sha
       end
+
+      def compare_in(project)
+        # We're at the initial commit, so just get that as we can't compare to anything.
+        if Gitlab::Git.blank_ref?(start_sha)
+          project.commit(head_sha)
+        else
+          straight = start_sha == base_sha
+          CompareService.new(project, head_sha).execute(project, start_sha, straight: straight)
+        end
+      end
     end
   end
 end

lib/gitlab/diff/position.rb

@@ -145,23 +145,9 @@ def line_code(repository)
       private
 
       def find_diff_file(repository)
-        # We're at the initial commit, so just get that as we can't compare to anything.
-        compare =
-          if Gitlab::Git.blank_ref?(start_sha)
-            Gitlab::Git::Commit.find(repository.raw_repository, head_sha)
-          else
-            Gitlab::Git::Compare.new(
-              repository.raw_repository,
-              start_sha,
-              head_sha
-            )
-          end
-
-        diff = compare.diffs(paths: paths).first
-
-        return unless diff
+        return unless diff_refs.complete?
 
-        Gitlab::Diff::File.new(diff, repository: repository, diff_refs: diff_refs)
+        diff_refs.compare_in(repository.project).diffs(paths: paths, expanded: true).diff_files.first
       end
     end
   end

lib/gitlab/diff/position_tracer.rb

@@ -216,7 +216,7 @@ def cd_diffs
 
       def compare(start_sha, head_sha, straight: false)
         compare = CompareService.new(project, head_sha).execute(project, start_sha, straight: straight)
-        compare.diffs(paths: paths)
+        compare.diffs(paths: paths, expanded: true)
       end
 
       def position(diff_file, old_line, new_line)

lib/gitlab/git/compare.rb

@@ -3,7 +3,7 @@ module Git
     class Compare
       attr_reader :head, :base, :straight
 
-      def initialize(repository, base, head, straight = false)
+      def initialize(repository, base, head, straight: false)
         @repository = repository
         @straight = straight
 

spec/lib/gitlab/diff/diff_refs_spec.rb

@@ -0,0 +1,61 @@
+require 'spec_helper'
+
+describe Gitlab::Diff::DiffRefs, lib: true do
+  let(:project) { create(:project, :repository) }
+
+  describe '#compare_in' do
+    context 'with diff refs for the initial commit' do
+      let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') }
+      subject { commit.diff_refs }
+
+      it 'returns an appropriate comparison' do
+        compare = subject.compare_in(project)
+
+        expect(compare.diff_refs).to eq(subject)
+      end
+    end
+
+    context 'with diff refs for a commit' do
+      let(:commit) { project.commit('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') }
+      subject { commit.diff_refs }
+
+      it 'returns an appropriate comparison' do
+        compare = subject.compare_in(project)
+
+        expect(compare.diff_refs).to eq(subject)
+      end
+    end
+
+    context 'with diff refs for a comparison through the base' do
+      subject do
+        described_class.new(
+          start_sha: '0b4bc9a49b562e85de7cc9e834518ea6828729b9', # feature
+          base_sha: 'ae73cb07c9eeaf35924a10f713b364d32b2dd34f',
+          head_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a' # master
+        )
+      end
+
+      it 'returns an appropriate comparison' do
+        compare = subject.compare_in(project)
+
+        expect(compare.diff_refs).to eq(subject)
+      end
+    end
+
+    context 'with diff refs for a straight comparison' do
+      subject do
+        described_class.new(
+          start_sha: '0b4bc9a49b562e85de7cc9e834518ea6828729b9', # feature
+          base_sha: '0b4bc9a49b562e85de7cc9e834518ea6828729b9',
+          head_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a' # master
+        )
+      end
+
+      it 'returns an appropriate comparison' do
+        compare = subject.compare_in(project)
+
+        expect(compare.diff_refs).to eq(subject)
+      end
+    end
+  end
+end

spec/lib/gitlab/diff/position_spec.rb

@@ -381,6 +381,54 @@
     end
   end
 
+  describe "position for a file in a straight comparison" do
+    let(:diff_refs) do
+      Gitlab::Diff::DiffRefs.new(
+        start_sha: '0b4bc9a49b562e85de7cc9e834518ea6828729b9', # feature
+        base_sha: '0b4bc9a49b562e85de7cc9e834518ea6828729b9',
+        head_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a' # master
+      )
+    end
+
+    subject do
+      described_class.new(
+        old_path: "files/ruby/feature.rb",
+        new_path: "files/ruby/feature.rb",
+        old_line: 3,
+        new_line: nil,
+        diff_refs: diff_refs
+      )
+    end
+
+    describe "#diff_file" do
+      it "returns the correct diff file" do
+        diff_file = subject.diff_file(project.repository)
+
+        expect(diff_file.deleted_file?).to be true
+        expect(diff_file.old_path).to eq(subject.old_path)
+        expect(diff_file.diff_refs).to eq(subject.diff_refs)
+      end
+    end
+
+    describe "#diff_line" do
+      it "returns the correct diff line" do
+        diff_line = subject.diff_line(project.repository)
+
+        expect(diff_line.removed?).to be true
+        expect(diff_line.old_line).to eq(subject.old_line)
+        expect(diff_line.text).to eq("-    puts 'bar'")
+      end
+    end
+
+    describe "#line_code" do
+      it "returns the correct line code" do
+        line_code = Gitlab::Diff::LineCode.generate(subject.file_path, 0, subject.old_line)
+
+        expect(subject.line_code(project.repository)).to eq(line_code)
+      end
+    end
+  end
+
   describe "#to_json" do
     let(:hash) do
       {

spec/lib/gitlab/git/compare_spec.rb

@@ -2,8 +2,8 @@
 
 describe Gitlab::Git::Compare, seed_helper: true do
   let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) }
-  let(:compare) { Gitlab::Git::Compare.new(repository, SeedRepo::BigCommit::ID, SeedRepo::Commit::ID, false) }
-  let(:compare_straight) { Gitlab::Git::Compare.new(repository, SeedRepo::BigCommit::ID, SeedRepo::Commit::ID, true) }
+  let(:compare) { Gitlab::Git::Compare.new(repository, SeedRepo::BigCommit::ID, SeedRepo::Commit::ID, straight: false) }
+  let(:compare_straight) { Gitlab::Git::Compare.new(repository, SeedRepo::BigCommit::ID, SeedRepo::Commit::ID, straight: true) }
 
   describe '#commits' do
     subject do