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

6 files changed, 48 insertions, 6 deletions

diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml
index c3d2f80544b..6120b2191dd 100644
--- a/app/views/projects/diffs/_content.html.haml
+++ b/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
diff --git a/changelogs/unreleased/stanhu-gitlab-ce-fix-error-500-with-mr-images.yml b/changelogs/unreleased/stanhu-gitlab-ce-fix-error-500-with-mr-images.yml
new file mode 100644
index 00000000000..7ca0d5fb19e
--- /dev/null
+++ b/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:
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index 5110bfbf898..c6bf25b5874 100644
--- a/lib/gitlab/diff/file.rb
+++ b/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)
diff --git a/spec/features/merge_requests/create_new_mr_spec.rb b/spec/features/merge_requests/create_new_mr_spec.rb
index c68e1ea4af9..584574cc91a 100644
--- a/spec/features/merge_requests/create_new_mr_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb
index 0650cb291e5..38475792d93 100644
--- a/spec/lib/gitlab/diff/file_spec.rb
+++ b/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
diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb
index 103f7542286..4cf81be3adc 100644
--- a/spec/support/test_env.rb
+++ b/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