Merge branch 'stanhu/gitlab-ce-fix-error-500-with-mr-images' into 'master'

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

_Originally opened at !4816 by @stanhu._

- - -

## What does this MR do?

This MR fixes an Error 500 when creating a merge request that contains an image that was deleted and added. Before, when displaying the before and after image, the code would always retrieve the image from the parent commit. However, in a diff, this could cause two different problems:
The "before" image may not actually be the image you want to compare against (regression of #14327)
It may appear as though a file was modified when it was really just added during the diff

## Are there points in the code the reviewer needs to double check?

There may be a more elegant to fix this bug.

## What are the relevant issue numbers?

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

See merge request !7457

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

+---
+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