Fix Error 500 when creating a merge request that contains an image that was deleted and added

Steps to reproduce:

1. Start with a repo with an image
2. Add a commit to delete the image
3. Add another commit to replace the image with another image

In a diff comparison, we really just compare about what the image was before the diff, not
the direct parent of the last commit. This MR fixes that.

Closes #3893, gitlab-org/gitlab-ee#678

Signed-off-by: Rémy Coutable <remy@rymai.me>

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

@@ -25,7 +25,7 @@
       - elsif diff_file.renamed_file
         .nothing-here-block File moved
   - elsif blob.image?
-    - old_blob = diff_file.old_blob(diff_commit)
+    - old_blob = diff_file.old_blob(diff_file.old_content_commit || @base_commit)
     = render "projects/diffs/image", diff_file: diff_file, old_file: old_blob, file: blob
   - else
     .nothing-here-block No preview for this file type

changelogs/unreleased/stanhu-gitlab-ce-fix-error-500-with-mr-images.yml

@@ -0,0 +1,4 @@
+---
+title: Fix Error 500 when creating a merge request that contains an image that was deleted and added
+merge_request: 7457
+author:

lib/gitlab/diff/file.rb

@@ -55,6 +55,12 @@ module Gitlab
         repository.commit(deleted_file ? old_ref : new_ref)
       end
 
+      def old_content_commit
+        return unless diff_refs
+
+        repository.commit(old_ref)
+      end
+
       def old_ref
         diff_refs.try(:base_sha)
       end
@@ -111,13 +117,10 @@ module Gitlab
         diff_lines.count(&:removed?)
       end
 
-      def old_blob(commit = content_commit)
+      def old_blob(commit = old_content_commit)
         return unless commit
 
-        parent_id = commit.parent_id
-        return unless parent_id
-
-        repository.blob_at(parent_id, old_path)
+        repository.blob_at(commit.id, old_path)
       end
 
       def blob(commit = content_commit)

spec/features/merge_requests/create_new_mr_spec.rb

@@ -67,4 +67,14 @@ feature 'Create New Merge Request', feature: true, js: true do
     expect(page).to have_content('Source branch "non-exist-source" does not exist')
     expect(page).to have_content('Target branch "non-exist-target" does not exist')
   end
+
+  context 'when a branch contains commits that both delete and add the same image' do
+    it 'renders the diff successfully' do
+      visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { target_branch: 'master', source_branch: 'deleted-image-test' })
+
+      click_link "Changes"
+
+      expect(page).to have_content "6049019_460s.jpg"
+    end
+  end
 end

spec/lib/gitlab/diff/file_spec.rb

@@ -46,4 +46,28 @@ describe Gitlab::Diff::File, lib: true do
       expect(diff_file.collapsed?).to eq(false)
     end
   end
+
+  describe '#old_content_commit' do
+    it 'returns base commit' do
+      old_content_commit = diff_file.old_content_commit
+
+      expect(old_content_commit.id).to eq('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9')
+    end
+  end
+
+  describe '#old_blob' do
+    it 'returns blob of commit of base commit' do
+      old_data = diff_file.old_blob.data
+
+      expect(old_data).to include('raise "System commands must be given as an array of strings"')
+    end
+  end
+
+  describe '#blob' do
+    it 'returns blob of new commit' do
+      data = diff_file.blob.data
+
+      expect(data).to include('raise RuntimeError, "System commands must be given as an array of strings"')
+    end
+  end
 end

spec/support/test_env.rb

@@ -35,6 +35,7 @@ module TestEnv
     'conflict-missing-side'              => 'eb227b3',
     'conflict-non-utf8'                  => 'd0a293c',
     'conflict-too-large'                 => '39fa04f',
+    'deleted-image-test'                 => '6c17798'
   }
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily