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.

5 files changed, 84 insertions, 98 deletions

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)