Merge branch 'merge-request-deleted-file' into 'master'

Correctly find last known blob for file deleted in MR.

Fixes #3092.

When building a new MR, `@merge_request.commits.last` would fail because this delegates to `merge_request_diff` which is still `nil` at that point. I fixed that, and changed some of the logic because showing deleted blob contents didn't previously work for the Compare page, and the UI would show the wrong commit sha for "View File @...".

See merge request !1647

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