Collapse large diffs by default

When rendering a list of diff files, skip those where the diff is over
10 KB and provide an endpoint to render individually instead.

CHANGELOG

@@ -32,6 +32,8 @@ v 8.10.0 (unreleased)
   - Fix user creation with stronger minimum password requirements !4054 (nathan-pmt)
   - 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/controllers/projects/commit_controller.rb

@@ -19,7 +19,7 @@ class Projects::CommitController < Projects::ApplicationController
 
     @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,
@@ -41,6 +41,24 @@ class Projects::CommitController < Projects::ApplicationController
     end
   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)
+  end
+
   def builds
   end
 

app/controllers/projects/compare_controller.rb

@@ -6,7 +6,7 @@ class Projects::CompareController < Projects::ApplicationController
   # Authorize
   before_action :require_non_empty_project
   before_action :authorize_download_code!
-  before_action :assign_ref_vars, only: [:index, :show]
+  before_action :assign_ref_vars, only: [:index, :show, :diff_for_path]
   before_action :merge_request, only: [:index, :show]
 
   def index
@@ -35,6 +35,22 @@ class Projects::CompareController < Projects::ApplicationController
     end
   end
 
+  def diff_for_path
+    compare = CompareService.new.
+      execute(@project, @head_ref, @project, @base_ref, diff_options)
+
+    return render_404 unless compare
+
+    @commit = @project.commit(@head_ref)
+    @base_commit = @project.merge_base_commit(@base_ref, @head_ref)
+    diffs = compare.diffs(diff_options.merge(paths: [params[:path]]))
+
+    @diff_notes_disabled = true
+    @grouped_diff_notes = {}
+
+    render_diff_for_path(diffs, [@base_commit, @commit], @project)
+  end
+
   def create
     redirect_to namespace_project_compare_path(@project.namespace, @project,
                                                params[:from], params[:to])

app/controllers/projects/merge_requests_controller.rb

@@ -13,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
@@ -58,7 +60,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     respond_to do |format|
       format.html
-      
+
       format.json do
         render json: @merge_request
       end
@@ -80,32 +82,32 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def diffs
     apply_diff_view_cookie!
 
-    @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.merge(paths: [params[:path]]))
+    diff_refs = @merge_request.diff_refs
+
+    render_diff_for_path(diffs, diff_refs, @merge_request.project)
+  end
+
   def commits
     respond_to do |format|
       format.html { render 'show' }
@@ -121,8 +123,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
@@ -380,6 +381,30 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     closes_issues
   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'
@@ -408,4 +433,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,25 @@ module DiffHelper
     [marked_old_line, marked_new_line]
   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)
+
+    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)
 

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

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

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_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/_file.html.haml

@@ -16,28 +16,9 @@
 
         = 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
+  - if diff_file.collapsed_by_default?
+    - url = url_for(params.merge(action: :diff_for_path, path: diff_file.file_path, format: nil))
+    .diff-content.diff-wrap-lines{data: { diff_for_path: url }}
+      .nothing-here-block File hidden by default; content for this element available at #{url}
+  - else
+    = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, diff_refs: diff_refs, blob: blob, project: project

config/routes.rb

@@ -615,10 +615,18 @@ Rails.application.routes.draw do
             post :retry_builds
             post :revert
             post :cherry_pick
+            get '/diffs/*path', action: :diff_for_path, constraints: { format: false }
           end
         end
 
-        resources :compare, only: [:index, :create]
+        resources :compare, only: [:index, :create] do
+          collection do
+            get '/diffs/*path', action: :diff_for_path, constraints: { format: false }
+          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 '/diffs/*path', action: :diff_for_path, constraints: { format: false }
           end
 
           collection do
             get :branch_from
             get :branch_to
             get :update_branches
+            get '/diffs/*path', action: :diff_for_path, constraints: { format: false }
           end
         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/commit_controller_spec.rb

-require 'rails_helper'
+require 'spec_helper'
 
 describe Projects::CommitController do
-  describe 'GET show' 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 'GET #show' do
     render_views
 
+    def go(extra_params = {})
+      params = {
+        namespace_id: project.namespace.to_param,
+        project_id: project.to_param
+      }
+
+      get :show, params.merge(extra_params)
+    end
+
     let(:project) { create(:project) }
 
     before do
@@ -15,7 +35,7 @@ describe Projects::CommitController do
 
     context 'with valid id' do
       it 'responds with 200' do
-        go id: project.commit.id
+        go(id: commit.id)
 
         expect(response).to be_ok
       end
@@ -23,27 +43,274 @@ describe Projects::CommitController do
 
     context 'with invalid id' do
       it 'responds with 404' do
-        go id: project.commit.id.reverse
+        go(id: commit.id.reverse)
 
         expect(response).to be_not_found
       end
     end
 
     it 'handles binary files' do
-      get(:show,
+      go(id: TestEnv::BRANCH_SHA['binary-encoding'], format: 'html')
+
+      expect(response).to be_success
+    end
+
+    shared_examples "export as" do |format|
+      it "should generally work" do
+        go(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}")
+
+        go(id: commit.id, format: format)
+      end
+
+      it "should render it" do
+        go(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 &<>" ')
+
+        go(id: commit.id, format: format)
+