Merge branch 'single-file-diffs' into 'master'

Single file diffs

Closes #14103. Related (not part of this MR): #19553.

This adds a `diff_for_path` action to each place we render diffs (commit, compare, new MR, existing MR) which renders the diff for a single path. The action is always available with the same params as the 'parent' action, to make it simpler to generate the URIs.

If a diff is bigger than 10 KB, it will be collapsed by default and have a data attribute added. You can then click the message or the filename to expand that diff. For expanded files, you can collapse and expand them, but they won't make any AJAX requests.

![Expand_and_collapse_diffs](/uploads/a4072029085082b85c47006f67ac531c/Expand_and_collapse_diffs.gif)

See merge request !4990

CHANGELOG

@@ -36,6 +36,8 @@ v 8.10.0 (unreleased)
   - Only show New Snippet button to users that can create snippets.
   - PipelinesFinder uses git cache data
   - Throttle the update of `project.pushes_since_gc` to 1 minute.
+  - Allow expanding and collapsing files in diff view (!4990)
+  - Collapse large diffs by default (!4990)
   - Check for conflicts with existing Project's wiki path when creating a new project.
   - Show last push widget in upstream after push to fork
   - Don't instantiate a git tree on Projects show default view

app/assets/javascripts/diff.js.coffee

 class @Diff
   UNFOLD_COUNT = 20
   constructor: ->
+    $('.files .diff-file').singleFileDiff()
+
     $(document).off('click', '.js-unfold')
     $(document).on('click', '.js-unfold', (event) =>
       target = $(event.target)

app/assets/javascripts/merge_request_tabs.js.coffee

@@ -160,6 +160,7 @@ class @MergeRequestTabs
         $('#diffs').html data.html
         gl.utils.localTimeAgo($('.js-timeago', 'div#diffs'))
         $('#diffs .js-syntax-highlight').syntaxHighlight()
+        $('#diffs .diff-file').singleFileDiff()
         @expandViewContainer() if @diffViewType() is 'parallel'
         @diffsLoaded = true
         @scrollToElement("#diffs")

app/assets/javascripts/single_file_diff.js.coffee

+class @SingleFileDiff
+
+  WRAPPER = '<div class="diff-content diff-wrap-lines"></div>'
+  LOADING_HTML = '<i class="fa fa-spinner fa-spin"></i>'
+  ERROR_HTML = '<div class="nothing-here-block"><i class="fa fa-warning"></i> Could not load diff</div>'
+  COLLAPSED_HTML = '<div class="nothing-here-block diff-collapsed">This diff is collapsed. Click to expand it.</div>'
+
+  constructor: (@file) ->
+    @content = $('.diff-content', @file)
+    @diffForPath = @content.find('[data-diff-for-path]').data 'diff-for-path'
+    @isOpen = !@diffForPath
+
+    if @diffForPath
+      @collapsedContent = @content
+      @loadingContent = $(WRAPPER).addClass('loading').html(LOADING_HTML).hide()
+      @content = null
+      @collapsedContent.after(@loadingContent)
+    else
+      @collapsedContent = $(WRAPPER).html(COLLAPSED_HTML).hide()
+      @content.after(@collapsedContent)
+
+    @collapsedContent.on 'click', @toggleDiff
+
+    $('.file-title > a', @file).on 'click', @toggleDiff
+
+  toggleDiff: (e) =>
+    @isOpen = !@isOpen
+    if not @isOpen and not @hasError
+      @content.hide()
+      @collapsedContent.show()
+    else if @content
+      @collapsedContent.hide()
+      @content.show()
+    else
+      @getContentHTML()
+
+  getContentHTML: ->
+    @collapsedContent.hide()
+    @loadingContent.show()
+    $.get @diffForPath, (data) =>
+      @loadingContent.hide()
+      if data.html
+        @content = $(data.html)
+        @content.syntaxHighlight()
+      else
+        @hasError = true
+        @content = $(ERROR_HTML)
+      @collapsedContent.after(@content)
+    return
+
+$.fn.singleFileDiff = ->
+  return @each ->
+    if not $.data this, 'singleFileDiff'
+      $.data this, 'singleFileDiff', new SingleFileDiff this

app/assets/stylesheets/framework/blocks.scss

@@ -16,6 +16,9 @@
   font-weight: normal;
   font-size: 16px;
   line-height: 36px;
+  &.diff-collapsed {
+    cursor: pointer;
+  }
 }
 
 .row-content-block {

app/controllers/concerns/diff_for_path.rb

+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).find do |diff|
+      diff.old_path == params[:old_path] && diff.new_path == params[:new_path]
+    end
+
+    return render_404 unless diff_file
+
+    diff_commit = commit_for_diff(diff_file)
+    blob = diff_file.blob(diff_commit)
+    @expand_all_diffs = 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

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 }
@@ -41,6 +27,10 @@ class Projects::CommitController < Projects::ApplicationController
     end
   end
 
+  def diff_for_path
+    render_diff_for_path(@diffs, @commit.diff_refs, @project)
+  end
+
   def builds
   end
 
@@ -114,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
@@ -122,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

app/controllers/projects/compare_controller.rb

 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]
+  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, diff_options)
+  end
+
+  def diff_for_path
+    return render_404 unless @compare
+
+    render_diff_for_path(@diffs, @diff_refs, @project)
+  end
+
+  def create
+    redirect_to namespace_project_compare_path(@project.namespace, @project,
+                                               params[:from], params[:to])
+  end
+
+  private
 
-    if compare
-      @commits = Commit.decorate(compare.commits, @project)
+  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)
+      @diffs = @compare.diffs(diff_options)
       @diff_refs = Gitlab::Diff::DiffRefs.new(
         base_sha: @base_commit.try(:sha),
         start_sha: @start_commit.try(:sha),
@@ -35,18 +57,6 @@ class Projects::CompareController < Projects::ApplicationController
     end
   end
 
-  def create
-    redirect_to namespace_project_compare_path(@project.namespace, @project,
-                                               params[:from], params[:to])
-  end
-
-  private
-
-  def assign_ref_vars
-    @start_ref = Addressable::URI.unescape(params[:from])
-    @ref = @head_ref = Addressable::URI.unescape(params[:to])
-  end
-
   def merge_request
     @merge_request ||= @project.merge_requests.opened.
       find_by(source_project: @project, source_branch: @head_ref, target_branch: @start_ref)

app/controllers/projects/merge_requests_controller.rb

 class Projects::MergeRequestsController < Projects::ApplicationController
   include ToggleSubscriptionAction
+  include DiffForPath
   include DiffHelper
   include IssuableActions
   include ToggleAwardEmoji
@@ -12,6 +13,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :validates_merge_request, only: [:show, :diffs, :commits, :builds]
   before_action :define_show_vars, only: [:show, :diffs, :commits, :builds]
   before_action :define_widget_vars, only: [:merge, :cancel_merge_when_build_succeeds, :merge_check]
+  before_action :define_commit_vars, only: [:diffs]
+  before_action :define_diff_comment_vars, only: [:diffs]
   before_action :ensure_ref_fetched, only: [:show, :diffs, :commits, :builds]
 
   # Allow read any merge_request
@@ -54,7 +57,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def show
     respond_to do |format|
       format.html
-      
+
       format.json do
         render json: @merge_request
       end
@@ -78,32 +81,31 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     @merge_request_diff = @merge_request.merge_request_diff
 
-    @commit = @merge_request.diff_head_commit
-    @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit
-
-    @comments_target = {
-      noteable_type: 'MergeRequest',
-      noteable_id: @merge_request.id
-    }
-
-    @use_legacy_diff_notes = !@merge_request.support_new_diff_notes?
-    @grouped_diff_notes = @merge_request.notes.grouped_diff_notes
-
-    Banzai::NoteRenderer.render(
-      @grouped_diff_notes.values.flatten,
-      @project,
-      current_user,
-      @path,
-      @project_wiki,
-      @ref
-    )
-
     respond_to do |format|
       format.html
       format.json { render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } }
     end
   end
 
+  # With an ID param, loads the MR at that ID. Otherwise, accepts the same params as #new
+  # and uses that (unsaved) MR.
+  #
+  def diff_for_path
+    if params[:id]
+      merge_request
+      define_diff_comment_vars
+    else
+      build_merge_request
+      @diff_notes_disabled = true
+      @grouped_diff_notes = {}
+    end
+
+    define_commit_vars
+    diffs = @merge_request.diffs(diff_options)
+
+    render_diff_for_path(diffs, @merge_request.diff_refs, @merge_request.project)
+  end
+
   def commits
     respond_to do |format|
       format.html { render 'show' }
@@ -127,8 +129,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def new
-    params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
-    @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute
+    build_merge_request
     @noteable = @merge_request
 
     @target_branches = if @merge_request.target_project
@@ -384,6 +385,30 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @pipelines = [@pipeline].compact
   end
 
+  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_comment_vars
+    @comments_target = {
+      noteable_type: 'MergeRequest',
+      noteable_id: @merge_request.id
+    }
+
+    @use_legacy_diff_notes = !@merge_request.support_new_diff_notes?
+    @grouped_diff_notes = @merge_request.notes.grouped_diff_notes
+
+    Banzai::NoteRenderer.render(
+      @grouped_diff_notes.values.flatten,
+      @project,
+      current_user,
+      @path,
+      @project_wiki,
+      @ref
+    )
+  end
+
   def invalid_mr
     # Render special view for MR with removed source or target branch
     render 'invalid'
@@ -412,4 +437,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     params[:merge_when_build_succeeds].present? &&
       @merge_request.pipeline && @merge_request.pipeline.active?
   end
+
+  def build_merge_request
+    params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
+    @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute
+  end
 end

app/helpers/diff_helper.rb

@@ -8,6 +8,10 @@ module DiffHelper
     [marked_old_line, marked_new_line]
   end
 
+  def expand_all_diffs?
+    @expand_all_diffs || params[:expand_all_diffs].present?
+  end
+
   def diff_view
     diff_views = %w(inline parallel)
 
@@ -18,16 +22,14 @@ module DiffHelper
     end
   end
 
-  def diff_hard_limit_enabled?
-    params[:force_show_diff].present?
-  end
-
   def diff_options
-    options = { ignore_whitespace_change: hide_whitespace? }
-    if diff_hard_limit_enabled?
-      options.merge!(Commit.max_diff_options)
+    default_options = Commit.max_diff_options
+
+    if action_name == 'diff_for_path'
+      default_options[:paths] = params.values_at(:old_path, :new_path)
     end
-    options
+
+    default_options.merge(ignore_whitespace_change: hide_whitespace?)
   end
 
   def safe_diff_files(diffs, diff_refs: nil, repository: nil)
@@ -90,7 +92,7 @@ module DiffHelper
 
   def commit_for_diff(diff_file)
     return diff_file.content_commit if diff_file.content_commit
-    
+
     if diff_file.deleted_file
       @base_commit || @commit.parent || @commit
     else

app/models/merge_request.rb

@@ -19,7 +19,7 @@ class MergeRequest < ActiveRecord::Base
   after_create :create_merge_request_diff, unless: :importing?
   after_update :update_merge_request_diff
 
-  delegate :commits, :diffs, :real_size, to: :merge_request_diff, prefix: nil
+  delegate :commits, :real_size, 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
@@ -164,6 +164,10 @@ class MergeRequest < ActiveRecord::Base
     merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
   end
 
+  def diffs(*args)
+    merge_request_diff ? merge_request_diff.diffs(*args) : compare.diffs(*args)
+  end
+
   def diff_size
     merge_request_diff.size
   end

app/models/merge_request_diff.rb

@@ -46,7 +46,8 @@ class MergeRequestDiff < ActiveRecord::Base
         compare.diffs(options)
       end
     else
-      @diffs ||= load_diffs(st_diffs, options)
+      @diffs ||= {}
+      @diffs[options] ||= load_diffs(st_diffs, options)
     end
   end
 
@@ -144,6 +145,12 @@ class MergeRequestDiff < ActiveRecord::Base
 
   def load_diffs(raw, options)
     if raw.respond_to?(:each)
+      if paths = options[:paths]
+        raw = raw.select do |diff|
+          paths.include?(diff[:old_path]) || paths.include?(diff[:new_path])
+        end
+      end
+
       Gitlab::Git::DiffCollection.new(raw, options)
     else
       Gitlab::Git::DiffCollection.new([])

app/services/compare_service.rb

@@ -3,7 +3,7 @@ require 'securerandom'
 # Compare 2 branches for one repo or between repositories
 # and return Gitlab::Git::Compare object that responds to commits and diffs
 class CompareService
-  def execute(source_project, source_branch, target_project, target_branch, diff_options = {})
+  def execute(source_project, source_branch, target_project, target_branch)
     source_commit = source_project.commit(source_branch)
     return unless source_commit
 

app/views/projects/diffs/_content.html.haml

+.diff-content.diff-wrap-lines
+  - # Skip all non non-supported blobs
+  - return unless blob.respond_to?(:text?)
+  - if diff_file.too_large?
+    .nothing-here-block This diff could not be displayed because it is too large.
+  - elsif blob.only_display_raw?
+    .nothing-here-block This file is too large to display.
+  - elsif blob_text_viewable?(blob)
+    - if !project.repository.diffable?(blob)
+      .nothing-here-block This diff was suppressed by a .gitattributes entry.
+    - elsif diff_file.diff_lines.length > 0
+      - if diff_file.collapsed_by_default? && !expand_all_diffs?
+        - url = url_for(params.merge(action: :diff_for_path, old_path: diff_file.old_path, new_path: diff_file.new_path))
+        .nothing-here-block.diff-collapsed{data: { diff_for_path: url } }
+          This diff is collapsed. Click to expand it.
+      - elsif diff_view == 'parallel'
+        = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob
+      - else
+        = render "projects/diffs/text_file", 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?
+    - old_blob = diff_file.old_blob(diff_commit)
+    = render "projects/diffs/image", diff_file: diff_file, old_file: old_blob, file: blob
+  - else
+    .nothing-here-block No preview for this file type

app/views/projects/diffs/_diffs.html.haml

@@ -6,6 +6,8 @@
 
 .content-block.oneline-block.files-changed
   .inline-parallel-buttons
+    - unless expand_all_diffs?
+      = link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: 'html')), class: 'btn btn-default'
     - if show_whitespace_toggle
       - if current_controller?(:commit)
         = commit_diff_whitespace_link(@project, @commit, class: 'hidden-xs')

app/views/projects/diffs/_file.html.haml

@@ -16,28 +16,4 @@
 
         = view_file_btn(diff_commit.id, diff_file, project)
 
-  .diff-content.diff-wrap-lines
-    - # Skip all non non-supported blobs
-    - return unless blob.respond_to?(:text?)
-    - if diff_file.too_large?
-      .nothing-here-block This diff could not be displayed because it is too large.
-    - elsif blob.only_display_raw?
-      .nothing-here-block This file is too large to display.
-    - elsif blob_text_viewable?(blob)
-      - if !project.repository.diffable?(blob)
-        .nothing-here-block This diff was suppressed by a .gitattributes entry.
-      - elsif diff_file.diff_lines.length > 0
-        - if diff_view == 'parallel'
-          = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob, index: i
-        - else
-          = render "projects/diffs/text_file", diff_file: diff_file, index: i
-      - 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?
-      - old_blob = diff_file.old_blob(diff_commit)
-      = render "projects/diffs/image", diff_file: diff_file, old_file: old_blob, file: blob, index: i
-    - else
-      .nothing-here-block No preview for this file type
+  = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, diff_refs: diff_refs, blob: blob, project: project

app/views/projects/diffs/_text_file.html.haml

-- too_big = diff_file.diff_lines.count > Commit::DIFF_SAFE_LINES
-- if too_big
-  .suppressed-container
-    %a.show-suppressed-diff.js-show-suppressed-diff Changes suppressed. Click to show.
-
-%table.text-file.code.js-syntax-highlight{ class: too_big ? 'hide' : '' }
+%table.text-file.code.js-syntax-highlight
   - last_line = 0
   - diff_file.highlighted_diff_lines.each do |line|
     - last_line = line.new_pos

app/views/projects/diffs/_warning.html.haml

@@ -2,9 +2,6 @@
   %h4
     Too many changes to show.
     .pull-right
-      - unless diff_hard_limit_enabled?
-        = link_to "Reload with full diff", url_for(params.merge(force_show_diff: true, format: nil)), class: "btn btn-sm"
-
       - if current_controller?(:commit) or current_controller?(:merge_requests)
         - if current_controller?(:commit)
           = link_to "Plain diff", namespace_project_commit_path(@project.namespace, @project, @commit, format: :diff), class: "btn btn-sm"

config/routes.rb

@@ -615,10 +615,18 @@ Rails.application.routes.draw do
             post :retry_builds
             post :revert
             post :cherry_pick
+            get :diff_for_path
           end
         end
 
-        resources :compare, only: [:index, :create]
+        resources :compare, only: [:index, :create] do
+          collection do
+            get :diff_for_path
+          end
+        end
+
+        get '/compare/:from...:to', to: 'compare#show', as: 'compare', constraints: { from: /.+/, to: /.+/ }
+
         resources :network, only: [:show], constraints: { id: /(?:[^.]|\.(?!json$))+/, format: /json/ }
 
         resources :graphs, only: [:show], constraints: { id: /(?:[^.]|\.(?!json$))+/, format: /json/ } do
@@ -629,9 +637,6 @@ Rails.application.routes.draw do
           end
         end
 
-        get '/compare/:from...:to' => 'compare#show', :as => 'compare',
-            :constraints => { from: /.+/, to: /.+/ }
-
         resources :snippets, constraints: { id: /\d+/ } do
           member do
             get 'raw'
@@ -706,12 +711,14 @@ Rails.application.routes.draw do
             post :toggle_subscription
             post :toggle_award_emoji
             post :remove_wip
+            get :diff_for_path
           end
 
           collection do
             get :branch_from
             get :branch_to
             get :update_branches
+            get :diff_for_path
           end
         end
 

features/project/commits/commits.feature

@@ -83,11 +83,6 @@ Feature: Project Commits
     #Given I visit my project's commits stats page
     #Then I see commits stats
 
-  Scenario: I browse big commit
-    Given I visit big commit page
-    Then I see big commit warning
-    And I see "Reload with full diff" link
-
   Scenario: I browse a commit with an image
     Given I visit a commit with an image that changed
     Then The diff links to both the previous and current image

features/steps/project/commits/commits.rb

@@ -125,25 +125,6 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps
     expect(page).to have_content 'Authors'
   end
 
-  step 'I visit big commit page' do
-    # Create a temporary scope to ensure that the stub_const is removed after user
-    RSpec::Mocks.with_temporary_scope do
-      stub_const('Gitlab::Git::DiffCollection::DEFAULT_LIMITS', { max_lines: 1, max_files: 1 })
-      visit namespace_project_commit_path(@project.namespace, @project, sample_big_commit.id)
-    end
-  end
-
-  step 'I see big commit warning' do
-    expect(page).to have_content sample_big_commit.message
-    expect(page).to have_content "Too many changes"
-  end
-
-  step 'I see "Reload with full diff" link' do
-    link = find_link('Reload with full diff')
-    expect(link[:href]).to end_with('?force_show_diff=true')
-    expect(link[:href]).not_to include('.html')
-  end
-
   step 'I visit a commit with an image that changed' do
     visit namespace_project_commit_path(@project.namespace, @project, sample_image_commit.id)
   end

lib/gitlab/diff/file.rb

@@ -68,6 +68,10 @@ module Gitlab
         @lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a
       end
 
+      def collapsed_by_default?
+        diff.diff.bytesize > 10240 # 10 KB
+      end
+
       def highlighted_diff_lines
         @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight
       end

spec/controllers/commit_controller_spec.rb

-require 'spec_helper'
-
-describe Projects::CommitController do
-  let(:project) { create(:project) }
-  let(:user)    { create(:user) }
-  let(:commit)  { project.commit("master") }
-  let(:master_pickable_sha) { '7d3b0f7cff5f37573aea97cebfd5692ea1689924' }
-  let(:master_pickable_commit)  { project.commit(master_pickable_sha) }
-
-  before do
-    sign_in(user)
-    project.team << [user, :master]
-  end
-
-  describe "#show" do
-    shared_examples "export as" do |format|
-      it "should generally work" do
-        get(:show,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            id: commit.id,
-            format: format)
-
-        expect(response).to be_success
-      end
-
-      it "should generate it" do
-        expect_any_instance_of(Commit).to receive(:"to_#{format}")
-
-        get(:show,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            id: commit.id, format: format)
-      end
-
-      it "should render it" do
-        get(:show,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            id: commit.id, format: format)
-
-        expect(response.body).to eq(commit.send(:"to_#{format}"))
-      end
-
-      it "should not escape Html" do
-        allow_any_instance_of(Commit).to receive(:"to_#{format}").
-          and_return('HTML entities &<>" ')
-
-        get(:show,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            id: commit.id, format: format)
-
-        expect(response.body).not_to include('&amp;')
-        expect(response.body).not_to include('&gt;')
-        expect(response.body).not_to include('&lt;')
-        expect(response.body).not_to include('&quot;')
-      end
-    end
-
-    describe "as diff" do
-      include_examples "export as", :diff
-      let(:format) { :diff }
-
-      it "should really only be a git diff" do
-        get(:show,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            id: commit.id,
-            format: format)
-
-        expect(response.body).to start_with("diff --git")
-      end
-      
-      it "should really only be a git diff without whitespace changes" do
-        get(:show,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            id: '66eceea0db202bb39c4e445e8ca28689645366c5',
-            # id: commit.id,
-            format: format,
-            w: 1)
-
-        expect(response.body).to start_with("diff --git")
-        # without whitespace option, there are more than 2 diff_splits
-        diff_splits = assigns(:diffs).first.diff.split("\n")
-        expect(diff_splits.length).to be <= 2
-      end
-    end
-
-    describe "as patch" do
-      include_examples "export as", :patch
-      let(:format) { :patch }
-
-      it "should really be a git email patch" do
-        get(:show,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            id: commit.id,
-            format: format)
-
-        expect(response.body).to start_with("From #{commit.id}")
-      end
-
-      it "should contain a git diff" do
-        get(:show,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            id: commit.id,
-            format: format)
-
-        expect(response.body).to match(/^diff --git/)
-      end
-    end
-
-    context 'commit that removes a submodule' do
-      render_views
-
-      let(:fork_project) { create(:forked_project_with_submodules) }
-      let(:commit) { fork_project.commit('remove-submodule') }
-
-      before do
-        fork_project.team << [user, :master]
-      end
-
-      it 'renders it' do
-        get(:show,
-            namespace_id: fork_project.namespace.to_param,
-            project_id: fork_project.to_param,
-            id: commit.id)
-
-        expect(response).to be_success
-      end
-    end
-  end
-
-  describe "#branches" do
-    it "contains branch and tags information" do
-      get(:branches,
-          namespace_id: project.namespace.to_param,
-          project_id: project.to_param,
-          id: commit.id)
-
-      expect(assigns(:branches)).to include("master", "feature_conflict")
-      expect(assigns(:tags)).to include("v1.1.0")
-    end
-  end
-
-  describe '#revert' do
-    context 'when target branch is not provided' do
-      it 'should render the 404 page' do
-        post(:revert,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            id: commit.id)
-
-        expect(response).not_to be_success
-        expect(response).to have_http_status(404)
-      end
-    end
-
-    context 'when the revert was successful' do
-      it 'should redirect to the commits page' do
-        post(:revert,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            target_branch: 'master',
-            id: commit.id)
-
-        expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master')
-        expect(flash[:notice]).to eq('The commit has been successfully reverted.')
-      end
-    end
-
-    context 'when the revert failed' do
-      before do
-        post(:revert,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            target_branch: 'master',
-            id: commit.id)
-      end
-
-      it 'should redirect to the commit page' do
-        # Reverting a commit that has been already reverted.
-        post(:revert,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            target_branch: 'master',
-            id: commit.id)
-
-        expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, commit.id)
-        expect(flash[:alert]).to match('Sorry, we cannot revert this commit automatically.')
-      end
-    end
-  end
-
-  describe '#cherry_pick' do
-    context 'when target branch is not provided' do
-      it 'should render the 404 page' do
-        post(:cherry_pick,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            id: master_pickable_commit.id)
-
-        expect(response).not_to be_success
-        expect(response).to have_http_status(404)
-      end
-    end
-
-    context 'when the cherry-pick was successful' do
-      it 'should redirect to the commits page' do
-        post(:cherry_pick,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            target_branch: 'master',
-            id: master_pickable_commit.id)
-
-        expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master')
-        expect(flash[:notice]).to eq('The commit has been successfully cherry-picked.')
-      end
-    end
-
-    context 'when the cherry_pick failed' do
-      before do
-        post(:cherry_pick,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            target_branch: 'master',
-            id: master_pickable_commit.id)
-      end
-
-      it 'should redirect to the commit page' do
-        # Cherry-picking a commit that has been already cherry-picked.
-        post(:cherry_pick,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            target_branch: 'master',
-            id: master_pickable_commit.id)
-
-        expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, master_pickable_commit.id)
-        expect(flash[:alert]).to match('Sorry, we cannot cherry-pick this commit automatically.')
-      end
-    end
-  end
-end

spec/controllers/projects/compare_controller_spec.rb

@@ -64,4 +64,73 @@ describe Projects::CompareController do
       expect(assigns(:commits)).to eq(nil)
     end
   end
+
+  describe 'GET diff_for_path' do
+    def diff_for_path(extra_params = {})
+      params = {
+        namespace_id: project.namespace.to_param,
+        project_id: project.to_param
+      }
+
+      get :diff_for_path, params.merge(extra_params)
+    end
+
+    let(:existing_path) { 'files/ruby/feature.rb' }
+
+    context 'when the from and to refs exist' do
+      context 'when the user has access to the project' do
+        context 'when the path exists in the diff' do
+          it 'disables diff notes' do
+            diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path)
+
+            expect(assigns(:diff_notes_disabled)).to be_truthy
+          end
+
+          it 'only renders the diffs for the path given' do
+            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
+              expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
+              meth.call(diffs, diff_refs, project)
+            end
+
+            diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path)
+          end
+        end
+
+        context 'when the path does not exist in the diff' do
+          before { diff_for_path(from: ref_from, to: ref_to, old_path: existing_path.succ, new_path: existing_path.succ) }
+
+          it 'returns a 404' do
+            expect(response).to have_http_status(404)
+          end
+        end
+      end
+
+      context 'when the user does not have access to the project' do
+        before do
+          project.team.truncate
+          diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path)
+        end
+
+        it 'returns a 404' do
+          expect(response).to have_http_status(404)
+        end
+      end
+    end
+
+    context 'when the from ref does not exist' do
+      before { diff_for_path(from: ref_from.succ, to: ref_to, old_path: existing_path, new_path: existing_path) }
+
+      it 'returns a 404' do
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context 'when the to ref does not exist' do
+      before { diff_for_path(from: ref_from, to: ref_to.succ, old_path: existing_path, new_path: existing_path) }
+
+      it 'returns a 404' do
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
 end

spec/features/expand_collapse_diffs_spec.rb

+require 'spec_helper'
+
+feature 'Expand and collapse diffs', js: true, feature: true do
+  include WaitForAjax
+
+  before do
+    login_as :admin
+    project = create(:project)
+    branch = 'expand-collapse-diffs'
+
+    # Ensure that undiffable.md is in .gitattributes
+    project.repository.copy_gitattributes(branch)
+    visit namespace_project_commit_path(project.namespace, project, project.commit(branch))
+    execute_script('window.ajaxUris = []; $(document).ajaxSend(function(event, xhr, settings) { ajaxUris.push(settings.url) });')
+  end
+
+  def file_container(filename)
+    find("[data-blob-diff-path*='#{filename}']")
+  end
+
+  # Use define_method instead of let (which is memoized) so that this just works across a
+  # reload.
+  #
+  files = [
+    'small_diff.md', 'large_diff.md', 'large_diff_renamed.md', 'undiffable.md',
+    'too_large.md', 'too_large_image.jpg'
+  ]
+
+  files.each do |file|
+    define_method(file.split('.').first) { file_container(file) }
+  end
+
+  context 'visiting a commit with collapsed diffs' do
+    it 'shows small diffs immediately' do
+      expect(small_diff).to have_selector('.code')
+      expect(small_diff).not_to have_selector('.nothing-here-block')
+    end
+
+    it 'collapses large diffs by default' do
+      expect(large_diff).not_to have_selector('.code')
+      expect(large_diff).to have_selector('.nothing-here-block')
+    end
+
+    it 'collapses large diffs for renamed files by default' do
+      expect(large_diff_renamed).not_to have_selector('.code')
+      expect(large_diff_renamed).to have_selector('.nothing-here-block')
+      expect(large_diff_renamed).to have_selector('.file-title .deletion')
+      expect(large_diff_renamed).to have_selector('.file-title .addition')
+    end
+
+    it 'shows non-renderable diffs as such immediately, regardless of their size' do
+      expect(undiffable).not_to have_selector('.code')
+      expect(undiffable).to have_selector('.nothing-here-block')
+      expect(undiffable).to have_content('gitattributes')
+    end
+
+    it 'does not allow diffs that are larger than the maximum size to be expanded' do
+      expect(too_large).not_to have_selector('.code')
+      expect(too_large).to have_selector('.nothing-here-block')
+      expect(too_large).to have_content('too large')
+    end
+
+    it 'shows image diffs immediately, regardless of their size' do
+      expect(too_large_image).not_to have_selector('.nothing-here-block')
+      expect(too_large_image).to have_selector('.image')
+    end
+
+    context 'expanding a diff for a renamed file' do
+      before do
+        large_diff_renamed.find('.nothing-here-block').click
+        wait_for_ajax
+      end
+
+      it 'shows the old content' do
+        old_line = large_diff_renamed.find('.line_content.old')
+
+        expect(old_line).to have_content('two copies')
+      end
+
+      it 'shows the new content' do
+        new_line = large_diff_renamed.find('.line_content.new', match: :prefer_exact)
+
+        expect(new_line).to have_content('three copies')
+      end
+    end
+
+    context 'expanding a large diff' do
+      before do
+        click_link('large_diff.md')
+        wait_for_ajax
+      end
+
+      it 'makes a request to get the content' do
+        ajax_uris = evaluate_script('ajaxUris')
+
+        expect(ajax_uris).not_to be_empty
+        expect(ajax_uris.first).to include('large_diff.md')
+      end
+
+      it 'shows the diff content' do
+        expect(large_diff).to have_selector('.code')
+        expect(large_diff).not_to have_selector('.nothing-here-block')
+      end
+
+      context 'adding a comment to the expanded diff' do
+        let(:comment_text) { 'A comment' }
+
+        before do
+          large_diff.find('.line_holder', match: :prefer_exact).hover
+          large_diff.find('.add-diff-note').click
+          large_diff.find('.note-textarea').send_keys comment_text
+          large_diff.find_button('Comment').click
+          wait_for_ajax
+        end
+
+        it 'adds the comment' do
+          expect(large_diff.find('.notes')).to have_content comment_text
+        end
+
+        context 'reloading the page' do
+          before { refresh }
+
+          it 'collapses the large diff by default' do
+            expect(large_diff).not_to have_selector('.code')
+            expect(large_diff).to have_selector('.nothing-here-block')
+          end
+
+          context 'expanding the diff' do
+            before do
+              click_link('large_diff.md')
+              wait_for_ajax
+            end
+
+            it 'shows the diff content' do
+              expect(large_diff).to have_selector('.code')
+              expect(large_diff).not_to have_selector('.nothing-here-block')
+            end
+
+            it 'shows the diff comment' do
+              expect(large_diff.find('.notes')).to have_content comment_text
+            end
+          end
+        end
+      end
+    end
+
+    context 'collapsing an expanded diff' do
+      before { click_link('small_diff.md') }
+
+      it 'hides the diff content' do
+        expect(small_diff).not_to have_selector('.code')
+        expect(small_diff).to have_selector('.nothing-here-block')
+      end
+
+      context 're-expanding the same diff' do
+        before { click_link('small_diff.md') }
+
+        it 'shows the diff content' do
+          expect(small_diff).to have_selector('.code')
+          expect(small_diff).not_to have_selector('.nothing-here-block')
+        end
+
+        it 'does not make a new HTTP request' do
+          expect(evaluate_script('ajaxUris')).to be_empty
+        end
+      end
+    end
+  end
+
+  context 'expanding all diffs' do
+    before do
+      click_link('Expand all')
+      wait_for_ajax
+      execute_script('window.ajaxUris = []; $(document).ajaxSend(function(event, xhr, settings) { ajaxUris.push(settings.url) });')
+    end
+
+    it 'reloads the page with all diffs expanded' do
+      expect(small_diff).to have_selector('.code')
+      expect(small_diff).not_to have_selector('.nothing-here-block')
+
+      expect(large_diff).to have_selector('.code')
+      expect(large_diff).not_to have_selector('.nothing-here-block')
+    end
+
+    context 'collapsing an expanded diff' do
+      before { click_link('small_diff.md') }
+
+      it 'hides the diff content' do
+        expect(small_diff).not_to have_selector('.code')
+        expect(small_diff).to have_selector('.nothing-here-block')
+      end
+
+      context 're-expanding the same diff' do
+        before { click_link('small_diff.md') }
+
+        it 'shows the diff content' do
+          expect(small_diff).to have_selector('.code')
+          expect(small_diff).not_to have_selector('.nothing-here-block')
+        end
+
+        it 'does not make a new HTTP request' do
+          expect(evaluate_script('ajaxUris')).to be_empty
+        end
+      end
+    end
+  end
+end

spec/features/projects/labels/update_prioritization_spec.rb

@@ -76,7 +76,7 @@ feature 'Prioritize labels', feature: true do
         expect(page.all('li').last).to have_content('bug')
       end
 
-      visit current_url
+      refresh
       wait_for_ajax
 
       page.within('.prioritized-labels') do

spec/helpers/diff_helper_spec.rb

@@ -31,26 +31,11 @@ describe DiffHelper do
     end
   end
 
-  describe 'diff_hard_limit_enabled?' do
-    it 'should return true if param is provided' do
-      allow(controller).to receive(:params) { { force_show_diff: true } }
-      expect(diff_hard_limit_enabled?).to be_truthy
-    end
-
-    it 'should return false if param is not provided' do
-      expect(diff_hard_limit_enabled?).to be_falsey
-    end
-  end
-
   describe 'diff_options' do
-    it 'should return hard limit for a diff if force diff is true' do
+    it 'should return hard limit for a diff' do
       allow(controller).to receive(:params) { { force_show_diff: true } }
       expect(diff_options).to include(Commit.max_diff_options)
     end
-
-    it 'should return safe limit for a diff if force diff is false' do
-      expect(diff_options).not_to include(:max_lines, :max_files)
-    end
   end
 
   describe 'unfold_bottom_class' do

spec/models/merge_request_diff_spec.rb

+require 'spec_helper'
+
+describe MergeRequestDiff, models: true do
+  describe '#diffs' do
+    let(:mr) { create(:merge_request, :with_diffs) }
+    let(:mr_diff) { mr.merge_request_diff }
+
+    context 'when the :ignore_whitespace_change option is set' do
+      it 'creates a new compare object instead of loading from the DB' do
+        expect(mr_diff).not_to receive(:load_diffs)
+        expect(Gitlab::Git::Compare).to receive(:new).and_call_original
+
+        mr_diff.diffs(ignore_whitespace_change: true)
+      end
+    end
+
+    context 'when the raw diffs are empty' do
+      before { mr_diff.update_attributes(st_diffs: '') }
+
+      it 'returns an empty DiffCollection' do
+        expect(mr_diff.diffs).to be_a(Gitlab::Git::DiffCollection)
+        expect(mr_diff.diffs).to be_empty
+      end
+    end
+
+    context 'when the raw diffs exist' do
+      it 'returns the diffs' do
+        expect(mr_diff.diffs).to be_a(Gitlab::Git::DiffCollection)
+        expect(mr_diff.diffs).not_to be_empty
+      end
+
+      context 'when the :paths option is set' do
+        let(:diffs) { mr_diff.diffs(paths: ['files/ruby/popen.rb', 'files/ruby/popen.rb']) }
+
+        it 'only returns diffs that match the (old path, new path) given' do
+          expect(diffs.map(&:new_path)).to contain_exactly('files/ruby/popen.rb')
+        end
+
+        it 'uses the diffs from the DB' do
+          expect(mr_diff).to receive(:load_diffs)
+
+          diffs
+        end
+      end
+    end
+  end
+end

spec/models/merge_request_spec.rb

@@ -116,6 +116,31 @@ describe MergeRequest, models: true do
     end
   end
 
+  describe '#diffs' do
+    let(:merge_request) { build(:merge_request) }
+    let(:options) { { paths: ['a/b', 'b/a', 'c/*'] } }
+
+    context 'when there are MR diffs' do
+      it 'delegates to the MR diffs' do
+        merge_request.merge_request_diff = MergeRequestDiff.new
+
+        expect(merge_request.merge_request_diff).to receive(:diffs).with(options)
+
+        merge_request.diffs(options)
+      end
+    end
+
+    context 'when there are no MR diffs' do
+      it 'delegates to the compare object' do
+        merge_request.compare = double(:compare)
+
+        expect(merge_request.compare).to receive(:diffs).with(options)
+
+        merge_request.diffs(options)
+      end
+    end
+  end
+
   describe "#mr_and_commit_notes" do
     let!(:merge_request) { create(:merge_request) }
 

spec/support/capybara_helpers.rb

@@ -27,6 +27,14 @@ module CapybaraHelpers
       end
     end
   end
+
+  # Refresh the page. Calling `visit current_url` doesn't seem to work consistently.
+  #
+  def refresh
+    url = current_url
+    visit 'about:blank'
+    visit url
+  end
 end
 
 RSpec.configure do |config|

spec/support/test_env.rb

@@ -5,19 +5,20 @@ module TestEnv
 
   # When developing the seed repository, comment out the branch you will modify.
   BRANCH_SHA = {
-    'empty-branch'     => '7efb185',
-    'flatten-dir'      => 'e56497b',
-    'feature'          => '0b4bc9a',
-    'feature_conflict' => 'bb5206f',
-    'fix'              => '48f0be4',
-    'improve/awesome'  => '5937ac0',
-    'markdown'         => '0ed8c6c',
-    'lfs'              => 'be93687',
-    'master'           => '5937ac0',
-    "'test'"           => 'e56497b',
-    'orphaned-branch'  => '45127a9',
-    'binary-encoding'  => '7b1cf43',
-    'gitattributes'    => '5a62481',
+    'empty-branch'          => '7efb185',
+    'flatten-dir'           => 'e56497b',
+    'feature'               => '0b4bc9a',
+    'feature_conflict'      => 'bb5206f',
+    'fix'                   => '48f0be4',
+    'improve/awesome'       => '5937ac0',
+    'markdown'              => '0ed8c6c',
+    'lfs'                   => 'be93687',
+    'master'                => '5937ac0',
+    "'test'"                => 'e56497b',
+    'orphaned-branch'       => '45127a9',
+    'binary-encoding'       => '7b1cf43',
+    'gitattributes'         => '5a62481',
+    'expand-collapse-diffs' => '4842455'
   }
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily