From 7c479d88a92233790bc0fb63146fe004f8b9b5d7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 15 May 2017 13:19:49 -0500 Subject: [PATCH] Pass fallback_diff_refs to Diff::File instead of using view helpers --- .../projects/compare_controller.rb | 1 - .../projects/merge_requests_controller.rb | 2 - app/helpers/diff_helper.rb | 24 +----- app/models/concerns/note_on_diff.rb | 10 --- app/models/diff_note.rb | 4 +- app/models/legacy_diff_note.rb | 2 +- app/models/merge_request.rb | 34 ++------ app/models/merge_request_diff.rb | 18 ++++ .../discussions/_diff_with_notes.html.haml | 2 +- app/views/projects/diffs/_content.html.haml | 9 +- app/views/projects/diffs/_file.html.haml | 13 ++- .../projects/diffs/_file_header.html.haml | 3 +- .../projects/diffs/viewers/_image.html.haml | 6 +- .../projects/diffs/viewers/_text.html.haml | 1 + lib/gitlab/diff/file.rb | 82 ++++++++++++------- lib/gitlab/diff/file_collection/base.rb | 15 ++-- .../file_collection/merge_request_diff.rb | 3 +- spec/models/diff_note_spec.rb | 4 +- spec/models/merge_request_spec.rb | 6 +- 19 files changed, 117 insertions(+), 122 deletions(-) diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 008d2f5815f..7ec542bb2df 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -53,7 +53,6 @@ class Projects::CompareController < Projects::ApplicationController @commits = @compare.commits @start_commit = @compare.start_commit @commit = @compare.commit - @base_commit = @compare.base_commit @diffs = @compare.diffs(diff_options) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 0352065998b..c8f68192f24 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -502,7 +502,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController def define_commit_vars @commit = @merge_request.diff_head_commit - @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit end def define_diff_vars @@ -569,7 +568,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController @source_project = merge_request.source_project @commits = @merge_request.compare_commits.reverse @commit = @merge_request.diff_head_commit - @base_commit = @merge_request.diff_base_commit @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 4cfaa103741..4c4fbdd4d39 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -102,30 +102,14 @@ module DiffHelper ].join(' ').html_safe end - def diff_content_commit(diff_file) - content_commit = diff_file.content_commit - return content_commit if content_commit - - if diff_file.deleted_file? - diff_old_content_commit(diff_file) - else - @commit - end - end - - def diff_old_content_commit(diff_file) - return if diff_file.new_file? - - diff_file.old_content_commit || @base_commit || @commit.parent || @commit - end - def diff_file_blob_raw_path(diff_file) - namespace_project_raw_path(@project.namespace, @project, tree_join(diff_content_commit(diff_file).sha, diff_file.file_path)) + namespace_project_raw_path(@project.namespace, @project, tree_join(diff_file.content_sha, diff_file.file_path)) end def diff_file_old_blob_raw_path(diff_file) - return if diff_file.new_file? - namespace_project_raw_path(@project.namespace, @project, tree_join(diff_old_content_commit(diff_file).sha, diff_file.old_path)) + sha = diff_file.old_content_sha + return unless sha + namespace_project_raw_path(@project.namespace, @project, tree_join(diff_file.old_content_sha, diff_file.old_path)) end def diff_file_html_data(project, diff_file_path, diff_commit_id) diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb index 6359f7596b1..f734952fa6c 100644 --- a/app/models/concerns/note_on_diff.rb +++ b/app/models/concerns/note_on_diff.rb @@ -33,14 +33,4 @@ module NoteOnDiff def created_at_diff?(diff_refs) false end - - private - - def noteable_diff_refs - if noteable.respond_to?(:diff_sha_refs) - noteable.diff_sha_refs - else - noteable.diff_refs - end - end end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 76c59199afd..c6b770a180f 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -60,7 +60,7 @@ class DiffNote < Note return false unless supported? return true if for_commit? - diff_refs ||= noteable_diff_refs + diff_refs ||= noteable.diff_refs self.position.diff_refs == diff_refs end @@ -96,7 +96,7 @@ class DiffNote < Note self.project, nil, old_diff_refs: self.position.diff_refs, - new_diff_refs: noteable_diff_refs, + new_diff_refs: noteable.diff_refs, paths: self.position.paths ).execute(self) end diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index d7c627432d2..ebf8fb92ab5 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -61,7 +61,7 @@ class LegacyDiffNote < Note return true if for_commit? return true unless diff_line return false unless noteable - return false if diff_refs && diff_refs != noteable_diff_refs + return false if diff_refs && diff_refs != noteable.diff_refs noteable_diff = find_noteable_diff diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 9be00880438..08c72edc728 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -245,19 +245,6 @@ class MergeRequest < ActiveRecord::Base end end - # MRs created before 8.4 don't store a MergeRequestDiff#base_commit_sha, - # but we need to get a commit for the "View file @ ..." link by deleted files, - # so we find the likely one if we can't get the actual one. - # This will not be the actual base commit if the target branch was merged into - # the source branch after the merge request was created, but it is good enough - # for the specific purpose of linking to a commit. - # It is not good enough for use in `Gitlab::Git::DiffRefs`, which needs the - # true base commit, so we can't simply have `#diff_base_commit` fall back on - # this method. - def likely_diff_base_commit - first_commit.try(:parent) || first_commit - end - def diff_start_commit if persisted? merge_request_diff.start_commit @@ -322,21 +309,14 @@ class MergeRequest < ActiveRecord::Base end def diff_refs - return unless diff_start_commit || diff_base_commit - - Gitlab::Diff::DiffRefs.new( - base_sha: diff_base_sha, - start_sha: diff_start_sha, - head_sha: diff_head_sha - ) - end - - # Return diff_refs instance trying to not touch the git repository - def diff_sha_refs - if merge_request_diff && merge_request_diff.diff_refs_by_sha? + if persisted? merge_request_diff.diff_refs else - diff_refs + Gitlab::Diff::DiffRefs.new( + base_sha: diff_base_sha, + start_sha: diff_start_sha, + head_sha: diff_head_sha + ) end end @@ -858,7 +838,7 @@ class MergeRequest < ActiveRecord::Base end def has_complete_diff_refs? - diff_sha_refs && diff_sha_refs.complete? + diff_refs && diff_refs.complete? end def update_diff_notes_positions(old_diff_refs:, new_diff_refs:) diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index f0a3c30ea74..a6f3994166b 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -150,6 +150,24 @@ class MergeRequestDiff < ActiveRecord::Base ) end + # MRs created before 8.4 don't store their true diff refs (start and base), + # but we need to get a commit SHA for the "View file @ ..." link by a file, + # so we find use an approximation of the diff refs if we can't get the actual one. + # These will not be the actual diff refs if the target branch was merged into + # the source branch after the merge request was created, but it is good enough + # for the specific purpose of linking to a commit. + # It is not good enough for highlighting diffs, so we can't simply pass + # these as `diff_refs.` + def fallback_diff_refs + likely_base_commit_sha = (first_commit&.parent || first_commit)&.sha + + Gitlab::Diff::DiffRefs.new( + base_sha: likely_base_commit_sha, + start_sha: safe_start_commit_sha, + head_sha: head_commit_sha + ) + end + def diff_refs_by_sha? base_commit_sha? && head_commit_sha? && start_commit_sha? end diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index c3f55ff821f..70042dee20f 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -3,7 +3,7 @@ .diff-file.file-holder .js-file-title.file-title - = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_file.content_commit, project: discussion.project, url: discussion_path(discussion), show_toggle: false + = render "projects/diffs/file_header", diff_file: diff_file, url: discussion_path(discussion), show_toggle: false .diff-content.code.js-syntax-highlight %table diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml index b361619fc79..c7e22a0b4ec 100644 --- a/app/views/projects/diffs/_content.html.haml +++ b/app/views/projects/diffs/_content.html.haml @@ -1,7 +1,4 @@ -- diff_commit = local_assigns.fetch(:diff_commit) { diff_content_commit(diff_file) } -- diff_old_commit = local_assigns.fetch(:diff_old_commit) { diff_old_content_commit(diff_file) } -- blob = local_assigns.fetch(:blob) { diff_file.blob(diff_commit) } -- old_blob = local_assigns.fetch(:old_blob) { diff_file.old_blob(diff_old_commit) } +- blob = diff_file.blob .diff-content - if diff_file.too_large? @@ -18,13 +15,13 @@ %a.click-to-expand Click to expand it. - elsif diff_file.diff_lines.length > 0 - = render "projects/diffs/viewers/text", diff_file: diff_file, blob: blob + = render "projects/diffs/viewers/text", diff_file: diff_file - else - if diff_file.mode_changed? .nothing-here-block File mode changed - elsif diff_file.renamed_file? .nothing-here-block File moved - elsif blob.image? - = render "projects/diffs/viewers/image", diff_file: diff_file, blob: blob, old_blob: old_blob + = render "projects/diffs/viewers/image", diff_file: diff_file - else .nothing-here-block No preview for this file type diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index 35650dee68e..b5aea217384 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -1,13 +1,12 @@ - environment = local_assigns.fetch(:environment, nil) -- diff_commit = diff_content_commit(diff_file) -- blob = diff_file.blob(diff_commit) - file_hash = hexdigest(diff_file.file_path) -.diff-file.file-holder{ id: file_hash, data: diff_file_html_data(project, diff_file.file_path, diff_commit.id) } +.diff-file.file-holder{ id: file_hash, data: diff_file_html_data(project, diff_file.file_path, diff_file.content_sha) } .js-file-title.file-title-flex-parent .file-header-content - = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_commit, url: "##{file_hash}" + = render "projects/diffs/file_header", diff_file: diff_file, url: "##{file_hash}" - unless diff_file.submodule? + - blob = diff_file.blob .file-actions.hidden-xs - if blob.readable_text? = link_to '#', class: 'js-toggle-diff-comments btn active has-tooltip', title: "Toggle comments for this file", disabled: @diff_notes_disabled do @@ -18,9 +17,9 @@ = edit_blob_link(@merge_request.source_project, @merge_request.source_branch, diff_file.new_path, blob: blob, link_opts: link_opts) - = view_file_button(diff_commit.id, diff_file.file_path, project) - = view_on_environment_button(diff_commit.id, diff_file.file_path, environment) if environment + = view_file_button(diff_file.content_sha, diff_file.file_path, project) + = view_on_environment_button(diff_file.content_sha, diff_file.file_path, environment) if environment = render 'projects/fork_suggestion' - = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, blob: blob + = render 'projects/diffs/content', diff_file: diff_file diff --git a/app/views/projects/diffs/_file_header.html.haml b/app/views/projects/diffs/_file_header.html.haml index 3ce963e06ba..73c316472e3 100644 --- a/app/views/projects/diffs/_file_header.html.haml +++ b/app/views/projects/diffs/_file_header.html.haml @@ -4,11 +4,12 @@ %i.fa.diff-toggle-caret.fa-fw - if diff_file.submodule? + - blob = diff_file.blob %span = icon('archive fw') %strong.file-title-name - = submodule_link(blob, diff_commit.id, diff_file.repository) + = submodule_link(blob, diff_file.content_sha, diff_file.repository) = copy_file_path_button(blob.path) - else diff --git a/app/views/projects/diffs/viewers/_image.html.haml b/app/views/projects/diffs/viewers/_image.html.haml index b42904a6139..ea75373581e 100644 --- a/app/views/projects/diffs/viewers/_image.html.haml +++ b/app/views/projects/diffs/viewers/_image.html.haml @@ -1,3 +1,5 @@ +- blob = diff_file.blob +- old_blob = diff_file.old_blob - blob_raw_path = diff_file_blob_raw_path(diff_file) - old_blob_raw_path = diff_file_old_blob_raw_path(diff_file) @@ -12,7 +14,7 @@ .two-up.view %span.wrap .frame.deleted - %a{ href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_old_content_commit(diff_file).sha, diff_file.old_path)) } + %a{ href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_file.old_content_sha, diff_file.old_path)) } %img{ src: old_blob_raw_path, alt: diff_file.old_path } %p.image-info.hide %span.meta-filesize= number_to_human_size(old_blob.size) @@ -24,7 +26,7 @@ %span.meta-height %span.wrap .frame.added - %a{ href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_content_commit(diff_file).sha, diff_file.new_path)) } + %a{ href: namespace_project_blob_path(@project.namespace, @project, tree_join(diff_file.content_sha, diff_file.new_path)) } %img{ src: blob_raw_path, alt: diff_file.new_path } %p.image-info.hide %span.meta-filesize= number_to_human_size(blob.size) diff --git a/app/views/projects/diffs/viewers/_text.html.haml b/app/views/projects/diffs/viewers/_text.html.haml index 248fa52269d..e4b89671724 100644 --- a/app/views/projects/diffs/viewers/_text.html.haml +++ b/app/views/projects/diffs/viewers/_text.html.haml @@ -1,3 +1,4 @@ +- blob = diff_file.blob - blob.load_all_data!(diff_file.repository) - total_lines = blob.lines.size - total_lines -= 1 if total_lines > 0 && blob.lines.last.blank? diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index a812d549397..2aef7fdaa35 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -1,16 +1,17 @@ module Gitlab module Diff class File - attr_reader :diff, :repository, :diff_refs + attr_reader :diff, :repository, :diff_refs, :fallback_diff_refs delegate :new_file?, :deleted_file?, :renamed_file?, :old_path, :new_path, :a_mode, :b_mode, :mode_changed?, :submodule?, :too_large?, :collapsed?, to: :diff, prefix: false - def initialize(diff, repository:, diff_refs: nil) + def initialize(diff, repository:, diff_refs: nil, fallback_diff_refs: nil) @diff = diff @repository = repository @diff_refs = diff_refs + @fallback_diff_refs = fallback_diff_refs end def position(line) @@ -49,24 +50,60 @@ module Gitlab line_code(line) if line end - def content_commit - return unless diff_refs - - repository.commit(deleted_file? ? old_sha : new_sha) - end - - def old_content_commit - return unless diff_refs - - repository.commit(old_sha) - end - def old_sha - diff_refs.try(:base_sha) + diff_refs&.base_sha end def new_sha - diff_refs.try(:head_sha) + diff_refs&.head_sha + end + + def content_sha + return old_content_sha if deleted_file? + return @content_sha if defined?(@content_sha) + + refs = diff_refs || fallback_diff_refs + @content_sha = refs&.head_sha + end + + def content_commit + return @content_commit if defined?(@content_commit) + + sha = content_sha + @content_commit = repository.commit(sha) if sha + end + + def old_content_sha + return if new_file? + return @old_content_sha if defined?(@old_content_sha) + + refs = diff_refs || fallback_diff_refs + @old_content_sha = refs&.base_sha + end + + def old_content_commit + return @old_content_commit if defined?(@old_content_commit) + + sha = old_content_sha + @old_content_commit = repository.commit(sha) if sha + end + + def blob + return @blob if defined?(@blob) + + sha = content_sha + return @blob = nil unless sha + + repository.blob_at(sha, file_path) + end + + def old_blob + return @old_blob if defined?(@old_blob) + + sha = old_content_sha + return @old_blob = nil unless sha + + @old_blob = repository.blob_at(sha, old_path) end attr_writer :highlighted_diff_lines @@ -113,19 +150,6 @@ module Gitlab diff_lines.count(&:removed?) end - def old_blob(commit = old_content_commit) - return unless commit - return if new_file? - - repository.blob_at(commit.id, old_path) - end - - def blob(commit = content_commit) - return unless commit - - repository.blob_at(commit.id, file_path) - end - def file_identifier "#{file_path}-#{new_file?}-#{deleted_file?}-#{renamed_file?}" end diff --git a/lib/gitlab/diff/file_collection/base.rb b/lib/gitlab/diff/file_collection/base.rb index 2b9fc65b985..7330f3377e1 100644 --- a/lib/gitlab/diff/file_collection/base.rb +++ b/lib/gitlab/diff/file_collection/base.rb @@ -2,7 +2,7 @@ module Gitlab module Diff module FileCollection class Base - attr_reader :project, :diff_options, :diff_view, :diff_refs + attr_reader :project, :diff_options, :diff_view, :diff_refs, :fallback_diff_refs delegate :count, :size, :real_size, to: :diff_files @@ -10,14 +10,15 @@ module Gitlab ::Commit.max_diff_options.merge(ignore_whitespace_change: false, no_collapse: false) end - def initialize(diffable, project:, diff_options: nil, diff_refs: nil) + def initialize(diffable, project:, diff_options: nil, diff_refs: nil, fallback_diff_refs: nil) diff_options = self.class.default_options.merge(diff_options || {}) - @diffable = diffable - @diffs = diffable.raw_diffs(diff_options) - @project = project + @diffable = diffable + @diffs = diffable.raw_diffs(diff_options) + @project = project @diff_options = diff_options - @diff_refs = diff_refs + @diff_refs = diff_refs + @fallback_diff_refs = fallback_diff_refs end def diff_files @@ -27,7 +28,7 @@ module Gitlab private def decorate_diff!(diff) - Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs) + Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs, fallback_diff_refs: fallback_diff_refs) end end end diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb index 0bd226ef050..9a58b500a2c 100644 --- a/lib/gitlab/diff/file_collection/merge_request_diff.rb +++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb @@ -8,7 +8,8 @@ module Gitlab super(merge_request_diff, project: merge_request_diff.project, diff_options: diff_options, - diff_refs: merge_request_diff.diff_refs) + diff_refs: merge_request_diff.diff_refs, + fallback_diff_refs: merge_request_diff.fallback_diff_refs) end def diff_files diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index ab4c51a87b0..96f075d4f7d 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -145,7 +145,7 @@ describe DiffNote, models: true do context "when the merge request's diff refs don't match that of the diff note" do before do - allow(subject.noteable).to receive(:diff_sha_refs).and_return(commit.diff_refs) + allow(subject.noteable).to receive(:diff_refs).and_return(commit.diff_refs) end it "returns false" do @@ -194,7 +194,7 @@ describe DiffNote, models: true do context "when the note is outdated" do before do - allow(merge_request).to receive(:diff_sha_refs).and_return(commit.diff_refs) + allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs) end it "uses the DiffPositionUpdateService" do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 0e05f719499..6892f7bfc5f 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1243,7 +1243,7 @@ describe MergeRequest, models: true do end end - describe "#diff_sha_refs" do + describe "#diff_refs" do context "with diffs" do subject { create(:merge_request, :with_diffs) } @@ -1252,7 +1252,7 @@ describe MergeRequest, models: true do expect_any_instance_of(Repository).not_to receive(:commit) - subject.diff_sha_refs + subject.diff_refs end it "returns expected diff_refs" do @@ -1262,7 +1262,7 @@ describe MergeRequest, models: true do head_sha: subject.merge_request_diff.head_commit_sha ) - expect(subject.diff_sha_refs).to eq(expected_diff_refs) + expect(subject.diff_refs).to eq(expected_diff_refs) end end end