Commit f16bfa40 authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

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
parents d01fb5d8 8710739e
...@@ -17,9 +17,10 @@ class Projects::CompareController < Projects::ApplicationController ...@@ -17,9 +17,10 @@ class Projects::CompareController < Projects::ApplicationController
execute(@project, head_ref, @project, base_ref) execute(@project, head_ref, @project, base_ref)
if compare_result if compare_result
@commits = compare_result.commits @commits = Commit.decorate(compare_result.commits, @project)
@diffs = compare_result.diffs @diffs = compare_result.diffs
@commit = @commits.last @commit = @commits.last
@first_commit = @commits.first
@line_notes = [] @line_notes = []
end end
end end
......
...@@ -56,6 +56,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -56,6 +56,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
def diffs def diffs
@commit = @merge_request.last_commit @commit = @merge_request.last_commit
@first_commit = @merge_request.first_commit
@comments_allowed = @reply_allowed = true @comments_allowed = @reply_allowed = true
@comments_target = { @comments_target = {
noteable_type: 'MergeRequest', noteable_type: 'MergeRequest',
...@@ -89,7 +91,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -89,7 +91,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@target_project = merge_request.target_project @target_project = merge_request.target_project
@source_project = merge_request.source_project @source_project = merge_request.source_project
@commits = @merge_request.compare_commits @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 @diffs = @merge_request.compare_diffs
@note_counts = Note.where(commit_id: @commits.map(&:id)). @note_counts = Note.where(commit_id: @commits.map(&:id)).
group(:commit_id).count group(:commit_id).count
......
...@@ -170,7 +170,8 @@ module DiffHelper ...@@ -170,7 +170,8 @@ module DiffHelper
def commit_for_diff(diff) def commit_for_diff(diff)
if diff.deleted_file if diff.deleted_file
@merge_request ? @merge_request.commits.last : @commit.parents.first first_commit = @first_commit || @commit
first_commit.parent
else else
@commit @commit
end end
......
...@@ -164,6 +164,14 @@ class Commit ...@@ -164,6 +164,14 @@ class Commit
@committer ||= User.find_by_any_email(committer_email) @committer ||= User.find_by_any_email(committer_email)
end 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 def notes
project.notes.for_commit_id(self.id) project.notes.for_commit_id(self.id)
end end
...@@ -181,10 +189,6 @@ class Commit ...@@ -181,10 +189,6 @@ class Commit
@raw.short_id(7) @raw.short_id(7)
end end
def parents
@parents ||= Commit.decorate(super, project)
end
def ci_commit def ci_commit
project.ci_commit(sha) project.ci_commit(sha)
end end
......
...@@ -40,7 +40,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -40,7 +40,7 @@ class MergeRequest < ActiveRecord::Base
after_create :create_merge_request_diff after_create :create_merge_request_diff
after_update :update_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 # When this attribute is true some MR validation is ignored
# It allows us to close or modify broken merge requests # It allows us to close or modify broken merge requests
...@@ -157,6 +157,18 @@ class MergeRequest < ActiveRecord::Base ...@@ -157,6 +157,18 @@ class MergeRequest < ActiveRecord::Base
reference reference
end 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 def validate_branches
if target_project == source_project && target_branch == source_branch if target_project == source_project && target_branch == source_branch
errors.add :branch_conflict, "You can not use same project/branch for source and target" errors.add :branch_conflict, "You can not use same project/branch for source and target"
......
...@@ -55,6 +55,10 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -55,6 +55,10 @@ class MergeRequestDiff < ActiveRecord::Base
commits.first commits.first
end end
def first_commit
commits.last
end
def last_commit_short_sha def last_commit_short_sha
@last_commit_short_sha ||= last_commit.short_id @last_commit_short_sha ||= last_commit.short_id
end end
......
...@@ -312,13 +312,7 @@ class Repository ...@@ -312,13 +312,7 @@ class Repository
end end
def blob_for_diff(commit, diff) def blob_for_diff(commit, diff)
file = blob_at(commit.id, diff.new_path) blob_at(commit.id, diff.file_path)
unless file
file = prev_blob_for_diff(commit, diff)
end
file
end end
def prev_blob_for_diff(commit, diff) def prev_blob_for_diff(commit, diff)
......
...@@ -15,8 +15,8 @@ ...@@ -15,8 +15,8 @@
.files .files
- diff_files.each_with_index do |diff_file, index| - diff_files.each_with_index do |diff_file, index|
- diff_commit = commit_for_diff(diff_file.diff) - diff_commit = commit_for_diff(diff_file)
- blob = project.repository.blob_for_diff(diff_commit, diff_file.diff) - blob = project.repository.blob_for_diff(diff_commit, diff_file)
- next unless blob - next unless blob
= render 'projects/diffs/file', i: index, project: project, = render 'projects/diffs/file', i: index, project: project,
......
.diff-file{id: "diff-#{i}", data: diff_file_html_data(project, diff_commit, diff_file)} .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? - if diff_file.diff.submodule?
%span %span
- submodule_item = project.repository.blob_at(@commit.id, diff_file.file_path) - submodule_item = project.repository.blob_at(@commit.id, diff_file.file_path)
...@@ -38,7 +38,7 @@ ...@@ -38,7 +38,7 @@
- else - else
= render "projects/diffs/text_file", diff_file: diff_file, index: i = render "projects/diffs/text_file", diff_file: diff_file, index: i
- elsif blob.image? - 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 = render "projects/diffs/image", diff_file: diff_file, old_file: old_file, file: blob, index: i
- else - else
.nothing-here-block No preview for this file type .nothing-here-block No preview for this file type
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment