Commit a404ab38 authored by Paco Guzman's avatar Paco Guzman Committed by Douwe Maan

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

parent 65352b5b
...@@ -92,6 +92,7 @@ v 8.10.0 (unreleased) ...@@ -92,6 +92,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