Commit f0b446e5 authored by Douwe Maan's avatar Douwe Maan

Merge branch '19820-safer-diffs' into 'master'

Collapsed diffs lines/size don't accumulate to overflow diffs.

## What does this MR do?

Reduce the number of lines that we highlight on big diffs to try to reduce the degradation introduced by  !4990 as you can see on the issue description #19820 .

We collapse any files after the diff is over the number of safe files (100), or over the safe lines (500) or over the safe number of bytes (5KB x 100 files).

We still need to bump the gitlab_git and point this branch to the new gitlab_git version.

## What are the relevant issue numbers?

Closes #19820 
Closes #19885

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- ~~[ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
- ~~[ ] API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)


See merge request !5306
parents 3d9c7a0e a404ab38
...@@ -103,6 +103,7 @@ v 8.10.0 (unreleased) ...@@ -103,6 +103,7 @@ v 8.10.0 (unreleased)
- Add min value for project limit field on user's form !3622 (jastkand) - Add min value for project limit field on user's form !3622 (jastkand)
- Reset project pushes_since_gc when we enqueue the git gc call - Reset project pushes_since_gc when we enqueue the git gc call
- Add reminder to not paste private SSH keys !4399 (Ingo Blechschmidt) - Add reminder to not paste private SSH keys !4399 (Ingo Blechschmidt)
- Collapsed diffs lines/size don't acumulate to overflow diffs.
- Remove duplicate `description` field in `MergeRequest` entities (Ben Boeckel) - Remove duplicate `description` field in `MergeRequest` entities (Ben Boeckel)
- Style of import project buttons were fixed in the new project page. !5183 (rdemirbay) - Style of import project buttons were fixed in the new project page. !5183 (rdemirbay)
- Fix GitHub client requests when rate limit is disabled - Fix GitHub client requests when rate limit is disabled
......
...@@ -52,7 +52,7 @@ gem 'browser', '~> 2.2' ...@@ -52,7 +52,7 @@ gem 'browser', '~> 2.2'
# Extracting information from a git repository # Extracting information from a git repository
# Provide access to Gitlab::Git library # Provide access to Gitlab::Git library
gem 'gitlab_git', '~> 10.2' gem 'gitlab_git', '~> 10.3.2'
# LDAP Auth # LDAP Auth
# GitLab fork with several improvements to original library. For full list of changes # GitLab fork with several improvements to original library. For full list of changes
......
...@@ -274,7 +274,7 @@ GEM ...@@ -274,7 +274,7 @@ GEM
diff-lcs (~> 1.1) diff-lcs (~> 1.1)
mime-types (>= 1.16, < 3) mime-types (>= 1.16, < 3)
posix-spawn (~> 0.3) posix-spawn (~> 0.3)
gitlab_git (10.2.3) gitlab_git (10.3.2)
activesupport (~> 4.0) activesupport (~> 4.0)
charlock_holmes (~> 0.7.3) charlock_holmes (~> 0.7.3)
github-linguist (~> 4.7.0) github-linguist (~> 4.7.0)
...@@ -861,7 +861,7 @@ DEPENDENCIES ...@@ -861,7 +861,7 @@ DEPENDENCIES
github-linguist (~> 4.7.0) github-linguist (~> 4.7.0)
github-markup (~> 1.4) github-markup (~> 1.4)
gitlab-flowdock-git-hook (~> 1.0.1) gitlab-flowdock-git-hook (~> 1.0.1)
gitlab_git (~> 10.2) gitlab_git (~> 10.3.2)
gitlab_meta (= 7.0) gitlab_meta (= 7.0)
gitlab_omniauth-ldap (~> 1.2.1) gitlab_omniauth-ldap (~> 1.2.1)
gollum-lib (~> 4.2) gollum-lib (~> 4.2)
......
...@@ -10,7 +10,6 @@ module DiffForPath ...@@ -10,7 +10,6 @@ module DiffForPath
diff_commit = commit_for_diff(diff_file) diff_commit = commit_for_diff(diff_file)
blob = diff_file.blob(diff_commit) blob = diff_file.blob(diff_commit)
@expand_all_diffs = true
locals = { locals = {
diff_file: diff_file, diff_file: diff_file,
......
...@@ -9,7 +9,7 @@ module DiffHelper ...@@ -9,7 +9,7 @@ module DiffHelper
end end
def expand_all_diffs? def expand_all_diffs?
@expand_all_diffs || params[:expand_all_diffs].present? params[:expand_all_diffs].present?
end end
def diff_view def diff_view
...@@ -23,13 +23,14 @@ module DiffHelper ...@@ -23,13 +23,14 @@ module DiffHelper
end end
def diff_options def diff_options
default_options = Commit.max_diff_options options = { ignore_whitespace_change: hide_whitespace?, no_collapse: expand_all_diffs? }
if action_name == 'diff_for_path' if action_name == 'diff_for_path'
default_options[:paths] = params.values_at(:old_path, :new_path) options[:no_collapse] = true
options[:paths] = params.values_at(:old_path, :new_path)
end end
default_options.merge(ignore_whitespace_change: hide_whitespace?) Commit.max_diff_options.merge(options)
end end
def safe_diff_files(diffs, diff_refs: nil, repository: nil) def safe_diff_files(diffs, diff_refs: nil, repository: nil)
......
...@@ -8,12 +8,12 @@ ...@@ -8,12 +8,12 @@
- elsif blob_text_viewable?(blob) - elsif blob_text_viewable?(blob)
- if !project.repository.diffable?(blob) - if !project.repository.diffable?(blob)
.nothing-here-block This diff was suppressed by a .gitattributes entry. .nothing-here-block This diff was suppressed by a .gitattributes entry.
- elsif diff_file.diff_lines.length > 0 - elsif diff_file.collapsed?
- 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)) - 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 } } .nothing-here-block.diff-collapsed{data: { diff_for_path: url } }
This diff is collapsed. Click to expand it. This diff is collapsed. Click to expand it.
- elsif diff_view == 'parallel' - elsif diff_file.diff_lines.length > 0
- if diff_view == 'parallel'
= render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob
- else - else
= render "projects/diffs/text_file", diff_file: diff_file = render "projects/diffs/text_file", diff_file: diff_file
......
...@@ -6,7 +6,7 @@ ...@@ -6,7 +6,7 @@
.content-block.oneline-block.files-changed .content-block.oneline-block.files-changed
.inline-parallel-buttons .inline-parallel-buttons
- unless expand_all_diffs? - if !expand_all_diffs? && diff_files.any? { |diff_file| diff_file.collapsed? }
= link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: 'html')), class: 'btn btn-default' = link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: 'html')), class: 'btn btn-default'
- if show_whitespace_toggle - if show_whitespace_toggle
- if current_controller?(:commit) - if current_controller?(:commit)
......
...@@ -5,7 +5,7 @@ module Gitlab ...@@ -5,7 +5,7 @@ module Gitlab
delegate :new_file, :deleted_file, :renamed_file, delegate :new_file, :deleted_file, :renamed_file,
:old_path, :new_path, :a_mode, :b_mode, :old_path, :new_path, :a_mode, :b_mode,
:submodule?, :too_large?, to: :diff, prefix: false :submodule?, :too_large?, :collapsed?, to: :diff, prefix: false
def initialize(diff, repository:, diff_refs: nil) def initialize(diff, repository:, diff_refs: nil)
@diff = diff @diff = diff
...@@ -68,10 +68,6 @@ module Gitlab ...@@ -68,10 +68,6 @@ module Gitlab
@lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a @lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a
end end
def collapsed_by_default?
diff.diff.bytesize > 10240 # 10 KB
end
def highlighted_diff_lines def highlighted_diff_lines
@highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight
end end
......
...@@ -3,10 +3,11 @@ require 'spec_helper' ...@@ -3,10 +3,11 @@ require 'spec_helper'
feature 'Expand and collapse diffs', js: true, feature: true do feature 'Expand and collapse diffs', js: true, feature: true do
include WaitForAjax include WaitForAjax
let(:branch) { 'expand-collapse-diffs' }
before do before do
login_as :admin login_as :admin
project = create(:project) project = create(:project)
branch = 'expand-collapse-diffs'
# Ensure that undiffable.md is in .gitattributes # Ensure that undiffable.md is in .gitattributes
project.repository.copy_gitattributes(branch) project.repository.copy_gitattributes(branch)
...@@ -167,6 +168,46 @@ feature 'Expand and collapse diffs', js: true, feature: true do ...@@ -167,6 +168,46 @@ feature 'Expand and collapse diffs', js: true, feature: true do
end end
end end
context 'visiting a commit without collapsed diffs' do
let(:branch) { 'feature' }
it 'does not show Expand all button' do
expect(page).not_to have_link('Expand all')
end
end
context 'visiting a commit with more than safe files' do
let(:branch) { 'expand-collapse-files' }
# safe-files -> 100 | safe-lines -> 5000 | commit-files -> 105
it 'does collapsing from the safe number of files to the end on small files' do
expect(page).to have_link('Expand all')
expect(page).to have_selector('.diff-content', count: 105)
expect(page).to have_selector('.diff-collapsed', count: 5)
%w(file-95.txt file-96.txt file-97.txt file-98.txt file-99.txt).each do |filename|
expect(find("[data-blob-diff-path*='#{filename}']")).to have_selector('.diff-collapsed')
end
end
end
context 'visiting a commit with more than safe lines' do
let(:branch) { 'expand-collapse-lines' }
# safe-files -> 100 | safe-lines -> 5000 | commit_files -> 8 (each 1250 lines)
it 'does collapsing from the safe number of lines to the end' do
expect(page).to have_link('Expand all')
expect(page).to have_selector('.diff-content', count: 6)
expect(page).to have_selector('.diff-collapsed', count: 2)
%w(file-4.txt file-5.txt).each do |filename|
expect(find("[data-blob-diff-path*='#{filename}']")).to have_selector('.diff-collapsed')
end
end
end
context 'expanding all diffs' do context 'expanding all diffs' do
before do before do
click_link('Expand all') click_link('Expand all')
......
...@@ -32,10 +32,29 @@ describe DiffHelper do ...@@ -32,10 +32,29 @@ describe DiffHelper do
end end
describe 'diff_options' do describe 'diff_options' do
it 'should return hard limit for a diff' do it 'should return hard limit for a diff if force diff is true' do
allow(controller).to receive(:params) { { force_show_diff: true } } allow(controller).to receive(:params) { { force_show_diff: true } }
expect(diff_options).to include(Commit.max_diff_options) expect(diff_options).to include(Commit.max_diff_options)
end end
it 'should return hard limit for a diff if expand_all_diffs is true' do
allow(controller).to receive(:params) { { expand_all_diffs: true } }
expect(diff_options).to include(Commit.max_diff_options)
end
it 'should return no collapse false' do
expect(diff_options).to include(no_collapse: false)
end
it 'should return no collapse true if expand_all_diffs' do
allow(controller).to receive(:params) { { expand_all_diffs: true } }
expect(diff_options).to include(no_collapse: true)
end
it 'should return no collapse true if action name diff_for_path' do
allow(controller).to receive(:action_name) { 'diff_for_path' }
expect(diff_options).to include(no_collapse: true)
end
end end
describe 'unfold_bottom_class' do describe 'unfold_bottom_class' do
......
...@@ -32,4 +32,18 @@ describe Gitlab::Diff::File, lib: true do ...@@ -32,4 +32,18 @@ describe Gitlab::Diff::File, lib: true do
expect(diff_file.too_large?).to eq(false) expect(diff_file.too_large?).to eq(false)
end end
end end
describe '#collapsed?' do
it 'returns true for a file that is quite big' do
expect(diff).to receive(:collapsed?).and_return(true)
expect(diff_file.collapsed?).to eq(true)
end
it 'returns false for a file that is small enough' do
expect(diff).to receive(:collapsed?).and_return(false)
expect(diff_file.collapsed?).to eq(false)
end
end
end end
...@@ -18,7 +18,9 @@ module TestEnv ...@@ -18,7 +18,9 @@ module TestEnv
'orphaned-branch' => '45127a9', 'orphaned-branch' => '45127a9',
'binary-encoding' => '7b1cf43', 'binary-encoding' => '7b1cf43',
'gitattributes' => '5a62481', 'gitattributes' => '5a62481',
'expand-collapse-diffs' => '4842455' 'expand-collapse-diffs' => '4842455',
'expand-collapse-files' => '025db92',
'expand-collapse-lines' => '238e82d'
} }
# gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment