From ff55398aafa2feccaba4ed470becabc526b4df48 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Wed, 6 Jul 2016 18:15:27 +0100
Subject: [PATCH] DRY up diff_for_path actions
1. Move render method to a concern, not a helper.
2. Let DiffHelper#diff_options automatically add the path option.
3. Move more instance var definitions to before filters.
---
app/controllers/concerns/diff_for_path.rb | 23 +++++++
app/controllers/projects/commit_controller.rb | 61 ++++++++---------
.../projects/compare_controller.rb | 68 ++++++++-----------
.../projects/merge_requests_controller.rb | 6 +-
app/helpers/diff_helper.rb | 24 +------
5 files changed, 84 insertions(+), 98 deletions(-)
create mode 100644 app/controllers/concerns/diff_for_path.rb
diff --git a/app/controllers/concerns/diff_for_path.rb b/app/controllers/concerns/diff_for_path.rb
new file mode 100644
index 00000000000..b9b5d136bd9
--- /dev/null
+++ b/app/controllers/concerns/diff_for_path.rb
@@ -0,0 +1,23 @@
+module DiffForPath
+ extend ActiveSupport::Concern
+
+ def render_diff_for_path(diffs, diff_refs, project)
+ diff_file = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository).first
+
+ return render_404 unless diff_file
+
+ diff_commit = commit_for_diff(diff_file)
+ blob = diff_file.blob(diff_commit)
+ @expand_all = true
+
+ locals = {
+ diff_file: diff_file,
+ diff_commit: diff_commit,
+ diff_refs: diff_refs,
+ blob: blob,
+ project: project
+ }
+
+ render json: { html: view_to_html_string('projects/diffs/_content', locals) }
+ end
+end
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index 810653b4264..727e84b40a1 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -3,6 +3,7 @@
# Not to be confused with CommitsController, plural.
class Projects::CommitController < Projects::ApplicationController
include CreatesCommit
+ include DiffForPath
include DiffHelper
# Authorize
@@ -11,29 +12,14 @@ class Projects::CommitController < Projects::ApplicationController
before_action :authorize_update_build!, only: [:cancel_builds, :retry_builds]
before_action :authorize_read_commit_status!, only: [:builds]
before_action :commit
- before_action :define_show_vars, only: [:show, :builds]
+ before_action :define_commit_vars, only: [:show, :diff_for_path, :builds]
+ before_action :define_status_vars, only: [:show, :builds]
+ before_action :define_note_vars, only: [:show, :diff_for_path]
before_action :authorize_edit_tree!, only: [:revert, :cherry_pick]
def show
apply_diff_view_cookie!
- @grouped_diff_notes = commit.notes.grouped_diff_notes
- @notes = commit.notes.non_diff_notes.fresh
-
- Banzai::NoteRenderer.render(
- @grouped_diff_notes.values.flatten + @notes,
- @project,
- current_user,
- )
-
- @note = @project.build_commit_note(commit)
-
- @noteable = @commit
- @comments_target = {
- noteable_type: 'Commit',
- commit_id: @commit.id
- }
-
respond_to do |format|
format.html
format.diff { render text: @commit.to_diff }
@@ -42,21 +28,7 @@ class Projects::CommitController < Projects::ApplicationController
end
def diff_for_path
- return git_not_found! unless commit
-
- opts = diff_options
- opts[:ignore_whitespace_change] = true if params[:format] == 'diff'
-
- diffs = commit.diffs(opts.merge(paths: [params[:path]]))
- diff_refs = [commit.parent || commit, commit]
-
- @comments_target = {
- noteable_type: 'Commit',
- commit_id: @commit.id
- }
- @grouped_diff_notes = {}
-
- render_diff_for_path(diffs, diff_refs, @project)
+ render_diff_for_path(@diffs, @commit.diff_refs, @project)
end
def builds
@@ -132,7 +104,7 @@ class Projects::CommitController < Projects::ApplicationController
@ci_builds ||= Ci::Build.where(pipeline: pipelines)
end
- def define_show_vars
+ def define_commit_vars
return git_not_found! unless commit
opts = diff_options
@@ -140,7 +112,28 @@ class Projects::CommitController < Projects::ApplicationController
@diffs = commit.diffs(opts)
@notes_count = commit.notes.count
+ end
+
+ def define_note_vars
+ @grouped_diff_notes = commit.notes.grouped_diff_notes
+ @notes = commit.notes.non_diff_notes.fresh
+
+ Banzai::NoteRenderer.render(
+ @grouped_diff_notes.values.flatten + @notes,
+ @project,
+ current_user,
+ )
+
+ @note = @project.build_commit_note(commit)
+
+ @noteable = @commit
+ @comments_target = {
+ noteable_type: 'Commit',
+ commit_id: @commit.id
+ }
+ end
+ def define_status_vars
@statuses = CommitStatus.where(pipeline: pipelines)
@builds = Ci::Build.where(pipeline: pipelines)
end
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index 5e00d2d5aff..5f3ee71444d 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -1,60 +1,26 @@
require 'addressable/uri'
class Projects::CompareController < Projects::ApplicationController
+ include DiffForPath
include DiffHelper
# Authorize
before_action :require_non_empty_project
before_action :authorize_download_code!
- before_action :assign_ref_vars, only: [:index, :show, :diff_for_path]
+ before_action :define_ref_vars, only: [:index, :show, :diff_for_path]
+ before_action :define_diff_vars, only: [:show, :diff_for_path]
before_action :merge_request, only: [:index, :show]
def index
end
def show
- compare = CompareService.new.
- execute(@project, @head_ref, @project, @start_ref)
-
- if compare
- @commits = Commit.decorate(compare.commits, @project)
-
- @start_commit = @project.commit(@start_ref)
- @commit = @project.commit(@head_ref)
- @base_commit = @project.merge_base_commit(@start_ref, @head_ref)
-
- @diffs = compare.diffs(diff_options)
- @diff_refs = Gitlab::Diff::DiffRefs.new(
- base_sha: @base_commit.try(:sha),
- start_sha: @start_commit.try(:sha),
- head_sha: @commit.try(:sha)
- )
-
- @diff_notes_disabled = true
- @grouped_diff_notes = {}
- end
end
def diff_for_path
- compare = CompareService.new.
- execute(@project, @head_ref, @project, @start_ref)
-
- return render_404 unless compare
-
- @start_commit = @project.commit(@start_ref)
- @commit = @project.commit(@head_ref)
- @base_commit = @project.merge_base_commit(@start_ref, @head_ref)
- diffs = compare.diffs(diff_options.merge(paths: [params[:path]]))
- diff_refs = Gitlab::Diff::DiffRefs.new(
- base_sha: @base_commit.try(:sha),
- start_sha: @start_commit.try(:sha),
- head_sha: @commit.try(:sha)
- )
-
- @diff_notes_disabled = true
- @grouped_diff_notes = {}
+ return render_404 unless @compare
- render_diff_for_path(diffs, diff_refs, @project)
+ render_diff_for_path(@diffs, @diff_refs, @project)
end
def create
@@ -64,11 +30,33 @@ class Projects::CompareController < Projects::ApplicationController
private
- def assign_ref_vars
+ def define_ref_vars
@start_ref = Addressable::URI.unescape(params[:from])
@ref = @head_ref = Addressable::URI.unescape(params[:to])
end
+ def define_diff_vars
+ @compare = CompareService.new.execute(@project, @head_ref, @project, @start_ref)
+
+ if @compare
+ @commits = Commit.decorate(@compare.commits, @project)
+
+ @start_commit = @project.commit(@start_ref)
+ @commit = @project.commit(@head_ref)
+ @base_commit = @project.merge_base_commit(@start_ref, @head_ref)
+
+ @diffs = @compare.diffs(diff_options)
+ @diff_refs = Gitlab::Diff::DiffRefs.new(
+ base_sha: @base_commit.try(:sha),
+ start_sha: @start_commit.try(:sha),
+ head_sha: @commit.try(:sha)
+ )
+
+ @diff_notes_disabled = true
+ @grouped_diff_notes = {}
+ end
+ end
+
def merge_request
@merge_request ||= @project.merge_requests.opened.
find_by(source_project: @project, source_branch: @head_ref, target_branch: @start_ref)
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 3fc5a319c9b..31d7c324e55 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -1,5 +1,6 @@
class Projects::MergeRequestsController < Projects::ApplicationController
include ToggleSubscriptionAction
+ include DiffForPath
include DiffHelper
include IssuableActions
include ToggleAwardEmoji
@@ -102,10 +103,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
define_commit_vars
- diffs = @merge_request.diffs(diff_options.merge(paths: [params[:path]]))
- diff_refs = @merge_request.diff_refs
+ diffs = @merge_request.diffs(diff_options)
- render_diff_for_path(diffs, diff_refs, @merge_request.project)
+ render_diff_for_path(diffs, @merge_request.diff_refs, @merge_request.project)
end
def commits
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index fd7b71407f3..ebfe4e27b78 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -12,26 +12,6 @@ module DiffHelper
@expand_all || params[:expand].present?
end
- def render_diff_for_path(diffs, diff_refs, project)
- diff_file = safe_diff_files(diffs, diff_refs).first
-
- return render_404 unless diff_file
-
- diff_commit = commit_for_diff(diff_file)
- blob = project.repository.blob_for_diff(diff_commit, diff_file)
- @expand_all = true
-
- locals = {
- diff_file: diff_file,
- diff_commit: diff_commit,
- diff_refs: diff_refs,
- blob: blob,
- project: project
- }
-
- render json: { html: view_to_html_string('projects/diffs/_content', locals) }
- end
-
def diff_view
diff_views = %w(inline parallel)
@@ -43,7 +23,9 @@ module DiffHelper
end
def diff_options
- Commit.max_diff_options.merge(ignore_whitespace_change: hide_whitespace?)
+ default_options = Commit.max_diff_options
+ default_options[:paths] = [params[:path]] if params[:path]
+ default_options.merge(ignore_whitespace_change: hide_whitespace?)
end
def safe_diff_files(diffs, diff_refs: nil, repository: nil)
--
GitLab