Correctly find last known blob for file deleted in MR.merge-request-deleted-file

9 files changed, 38 insertions, 19 deletions

diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index d15004f93a6..71aaad1fad6 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -17,9 +17,10 @@ class Projects::CompareController < Projects::ApplicationController
       execute(@project, head_ref, @project, base_ref)
 
     if compare_result
-      @commits = compare_result.commits
+      @commits = Commit.decorate(compare_result.commits, @project)
       @diffs = compare_result.diffs
       @commit = @commits.last
+      @first_commit = @commits.first
       @line_notes = []
     end
   end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 0d9c5461959..16c42386623 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -56,6 +56,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def diffs
     @commit = @merge_request.last_commit
+    @first_commit = @merge_request.first_commit
+
     @comments_allowed = @reply_allowed = true
     @comments_target = {
       noteable_type: 'MergeRequest',
@@ -89,7 +91,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @target_project = merge_request.target_project
     @source_project = merge_request.source_project
     @commits = @merge_request.compare_commits
-    @commit = @merge_request.compare_commits.last
+    @commit = @merge_request.last_commit
+    @first_commit = @merge_request.first_commit
     @diffs = @merge_request.compare_diffs
     @note_counts = Note.where(commit_id: @commits.map(&:id)).
       group(:commit_id).count
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index b896fba3704..e65e37211c4 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -170,7 +170,8 @@ module DiffHelper
 
   def commit_for_diff(diff)
     if diff.deleted_file
-      @merge_request ? @merge_request.commits.last : @commit.parents.first
+      first_commit = @first_commit || @commit
+      first_commit.parent
     else
       @commit
     end
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 23b5e38336c..492f6be1ce3 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -164,6 +164,14 @@ class Commit
     @committer ||= User.find_by_any_email(committer_email)
   end
 
+  def parents
+    @parents ||= parent_ids.map { |id| project.commit(id) }
+  end
+
+  def parent
+    @parent ||= project.commit(self.parent_id) if self.parent_id
+  end
+
   def notes
     project.notes.for_commit_id(self.id)
   end
@@ -181,10 +189,6 @@ class Commit
     @raw.short_id(7)
   end
 
-  def parents
-    @parents ||= Commit.decorate(super, project)
-  end
-
   def ci_commit
     project.ci_commit(sha)
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 0042b95c4f1..21861a46a84 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -40,7 +40,7 @@ class MergeRequest < ActiveRecord::Base
   after_create :create_merge_request_diff
   after_update :update_merge_request_diff
 
-  delegate :commits, :diffs, :last_commit, :last_commit_short_sha, to: :merge_request_diff, prefix: nil
+  delegate :commits, :diffs, to: :merge_request_diff, prefix: nil
 
   # When this attribute is true some MR validation is ignored
   # It allows us to close or modify broken merge requests
@@ -157,6 +157,18 @@ class MergeRequest < ActiveRecord::Base
     reference
   end
 
+  def last_commit
+    merge_request_diff ? merge_request_diff.last_commit : compare_commits.last
+  end 
+
+  def first_commit
+    merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
+  end 
+
+  def last_commit_short_sha
+    last_commit.short_id
+  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"
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index bc2d691ece0..6575d0bc81f 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -55,6 +55,10 @@ class MergeRequestDiff < ActiveRecord::Base
     commits.first
   end
 
+  def first_commit
+    commits.last
+  end
+
   def last_commit_short_sha
     @last_commit_short_sha ||= last_commit.short_id
   end
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 921e1a9e426..e2d4f74407f 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -312,13 +312,7 @@ class Repository
   end
 
   def blob_for_diff(commit, diff)
-    file = blob_at(commit.id, diff.new_path)
-
-    unless file
-      file = prev_blob_for_diff(commit, diff)
-    end
-
-    file
+    blob_at(commit.id, diff.file_path)
   end
 
   def prev_blob_for_diff(commit, diff)
diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml
index 4f1965bfb39..56b51f038ba 100644
--- a/app/views/projects/diffs/_diffs.html.haml
+++ b/app/views/projects/diffs/_diffs.html.haml
@@ -15,8 +15,8 @@
 
 .files
   - diff_files.each_with_index do |diff_file, index|
-    - diff_commit = commit_for_diff(diff_file.diff)
-    - blob = project.repository.blob_for_diff(diff_commit, diff_file.diff)
+    - diff_commit = commit_for_diff(diff_file)
+    - blob = project.repository.blob_for_diff(diff_commit, diff_file)
     - next unless blob
 
     = render 'projects/diffs/file', i: index, project: project,
diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml
index 9698921f6da..410ff6abb43 100644
--- a/app/views/projects/diffs/_file.html.haml
+++ b/app/views/projects/diffs/_file.html.haml
@@ -1,5 +1,5 @@
 .diff-file{id: "diff-#{i}", data: diff_file_html_data(project, diff_commit, diff_file)}
-  .diff-header{id: "file-path-#{hexdigest(diff_file.new_path || diff_file.old_path)}"}
+  .diff-header{id: "file-path-#{hexdigest(diff_file.file_path)}"}
     - if diff_file.diff.submodule?
       %span
         - submodule_item = project.repository.blob_at(@commit.id, diff_file.file_path)
@@ -38,7 +38,7 @@
       - else
         = render "projects/diffs/text_file", diff_file: diff_file, index: i
     - elsif blob.image?
-      - old_file = project.repository.prev_blob_for_diff(@commit, diff_file)
+      - old_file = project.repository.prev_blob_for_diff(diff_commit, diff_file)
       = render "projects/diffs/image", diff_file: diff_file, old_file: old_file, file: blob, index: i
     - else
       .nothing-here-block No preview for this file type