From 72a42e81db13f23cd491945496b4678707723479 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 4 Feb 2016 11:26:42 +0100 Subject: [PATCH 01/33] Lazily create new Diff instances --- lib/gitlab_git/commit.rb | 6 +++++- lib/gitlab_git/compare.rb | 2 +- lib/gitlab_git/repository.rb | 6 ++++-- spec/commit_spec.rb | 4 ++-- spec/diff_spec.rb | 4 ++-- 5 files changed, 14 insertions(+), 8 deletions(-) diff --git a/lib/gitlab_git/commit.rb b/lib/gitlab_git/commit.rb index 47edd14..03073b5 100644 --- a/lib/gitlab_git/commit.rb +++ b/lib/gitlab_git/commit.rb @@ -205,7 +205,11 @@ module Gitlab end def diffs(options = {}) - diff_from_parent(options).map { |diff| Gitlab::Git::Diff.new(diff) } + Enumerator.new do |y| + diff_from_parent(options).each do |diff| + y << Gitlab::Git::Diff.new(diff) + end + end end def parents diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index 0c52c01..1cb9684 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -47,7 +47,7 @@ module Gitlab # Check if diff is empty because it is actually empty # and not because its impossible to get it def empty_diff? - diffs.empty? && timeout == false + diffs.first.nil? && timeout == false end end end diff --git a/lib/gitlab_git/repository.rb b/lib/gitlab_git/repository.rb index 79bddde..31672bc 100644 --- a/lib/gitlab_git/repository.rb +++ b/lib/gitlab_git/repository.rb @@ -315,8 +315,10 @@ module Gitlab # diff options. The +options+ hash can also include :break_rewrites to # split larger rewrites into delete/add pairs. def diff(from, to, options = {}, *paths) - diff_patches(from, to, options, *paths).map do |p| - Gitlab::Git::Diff.new(p) + Enumerator.new do |y| + diff_patches(from, to, options, *paths).each do |p| + y << Gitlab::Git::Diff.new(p) + end end end diff --git a/spec/commit_spec.rb b/spec/commit_spec.rb index 83c7186..e2a1435 100644 --- a/spec/commit_spec.rb +++ b/spec/commit_spec.rb @@ -320,8 +320,8 @@ describe Gitlab::Git::Commit do describe :diffs do subject { commit.diffs } - it { should be_kind_of Array } - its(:size) { should eq(2) } + it { should be_kind_of Enumerator } + it { subject.to_a.size.should eq(2) } its(:first) { should be_kind_of Gitlab::Git::Diff } end diff --git a/spec/diff_spec.rb b/spec/diff_spec.rb index 46419f3..d15d10e 100644 --- a/spec/diff_spec.rb +++ b/spec/diff_spec.rb @@ -52,8 +52,8 @@ EOT let(:diffs) { Gitlab::Git::Diff.between(repository, 'feature', 'master') } subject { diffs } - it { should be_kind_of Array } - its(:size) { should eq(1) } + it { should be_kind_of Enumerator } + it { subject.to_a.size.should eq(1) } context :diff do subject { diffs.first } -- GitLab From e6a9e089186fecddf1d9d49cc348ad1bb639ec75 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 4 Feb 2016 18:25:35 +0100 Subject: [PATCH 02/33] Safe emptiness check --- lib/gitlab_git/compare.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index 1cb9684..c396f5c 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -31,7 +31,7 @@ module Gitlab # Try to collect diff only if diffs is empty # Otherwise return cached version - if @diffs.empty? && @timeout == false + if @diffs.first.nil? && @timeout == false begin @diffs = Gitlab::Git::Diff.between(@repository, @head.id, @base.id, options, *paths) -- GitLab From a6a3cbdc554529dd4d714dfde2ed30e7477d9b1f Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 5 Feb 2016 14:55:55 +0100 Subject: [PATCH 03/33] Add #size method to Diff --- lib/gitlab_git/diff.rb | 4 ++++ spec/diff_spec.rb | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/lib/gitlab_git/diff.rb b/lib/gitlab_git/diff.rb index 3879404..04154f3 100644 --- a/lib/gitlab_git/diff.rb +++ b/lib/gitlab_git/diff.rb @@ -187,6 +187,10 @@ module Gitlab a_mode == '160000' || b_mode == '160000' end + def size + @size ||= @diff.scan(/\n/).count + end + private def init_from_rugged(rugged) diff --git a/spec/diff_spec.rb b/spec/diff_spec.rb index d15d10e..c4ad421 100644 --- a/spec/diff_spec.rb +++ b/spec/diff_spec.rb @@ -106,4 +106,10 @@ EOT it { Gitlab::Git::Diff.new(@diffs[0]).submodule?.should == false } it { Gitlab::Git::Diff.new(@diffs[1]).submodule?.should == true } end + + describe :size do + subject { Gitlab::Git::Diff.new(@rugged_diff) } + + its(:size) { should eq(9) } + end end -- GitLab From c992385ca323c1bd27dd1c47bc814afc6ad28bcd Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 8 Feb 2016 16:10:47 +0100 Subject: [PATCH 04/33] Add efficient line counting helper --- lib/gitlab_git.rb | 1 + lib/gitlab_git/count.rb | 18 ++++++++++++++++++ spec/count_spec.rb | 16 ++++++++++++++++ 3 files changed, 35 insertions(+) create mode 100644 lib/gitlab_git/count.rb create mode 100644 spec/count_spec.rb diff --git a/lib/gitlab_git.rb b/lib/gitlab_git.rb index 89bd83b..6be9293 100644 --- a/lib/gitlab_git.rb +++ b/lib/gitlab_git.rb @@ -16,6 +16,7 @@ require_relative "gitlab_git/blob" require_relative "gitlab_git/commit" require_relative "gitlab_git/commit_stats" require_relative "gitlab_git/compare" +require_relative "gitlab_git/count" require_relative "gitlab_git/diff" require_relative "gitlab_git/repository" require_relative "gitlab_git/tree" diff --git a/lib/gitlab_git/count.rb b/lib/gitlab_git/count.rb new file mode 100644 index 0000000..c10debe --- /dev/null +++ b/lib/gitlab_git/count.rb @@ -0,0 +1,18 @@ +module Gitlab + module Git + module Count + LINE_SEP = "\n" + + def self.lines(string) + case string[-1] + when nil + 0 + when LINE_SEP + string.count(LINE_SEP) + else + string.count(LINE_SEP) + 1 + end + end + end + end +end diff --git a/spec/count_spec.rb b/spec/count_spec.rb new file mode 100644 index 0000000..b272c5c --- /dev/null +++ b/spec/count_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +describe Gitlab::Git::Count do + describe :lines do + [ + ["", 0], + ["foo", 1], + ["foo\n", 1], + ["foo\n\n", 2], + ].each do |string, line_count| + it "counts #{line_count} lines in #{string.inspect}" do + Gitlab::Git::Count.lines(string).should eq(line_count) + end + end + end +end -- GitLab From 1a3e68545ec29b521bc59c44e299f2bda24ba19a Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 8 Feb 2016 18:38:44 +0100 Subject: [PATCH 05/33] Return DiffCollections instead of Enumerators --- lib/gitlab_git.rb | 1 + lib/gitlab_git/commit.rb | 9 +- lib/gitlab_git/compare.rb | 21 ++--- lib/gitlab_git/diff.rb | 5 +- lib/gitlab_git/diff_collection.rb | 58 +++++++++++++ lib/gitlab_git/repository.rb | 9 +- spec/commit_spec.rb | 6 +- spec/compare_spec.rb | 7 +- spec/diff_collection_spec.rb | 138 ++++++++++++++++++++++++++++++ spec/diff_spec.rb | 6 +- spec/repository_spec.rb | 2 +- 11 files changed, 228 insertions(+), 34 deletions(-) create mode 100644 lib/gitlab_git/diff_collection.rb create mode 100644 spec/diff_collection_spec.rb diff --git a/lib/gitlab_git.rb b/lib/gitlab_git.rb index 6be9293..79759d1 100644 --- a/lib/gitlab_git.rb +++ b/lib/gitlab_git.rb @@ -18,6 +18,7 @@ require_relative "gitlab_git/commit_stats" require_relative "gitlab_git/compare" require_relative "gitlab_git/count" require_relative "gitlab_git/diff" +require_relative "gitlab_git/diff_collection" require_relative "gitlab_git/repository" require_relative "gitlab_git/tree" require_relative "gitlab_git/blob_snippet" diff --git a/lib/gitlab_git/commit.rb b/lib/gitlab_git/commit.rb index 03073b5..ca2535d 100644 --- a/lib/gitlab_git/commit.rb +++ b/lib/gitlab_git/commit.rb @@ -205,11 +205,12 @@ module Gitlab end def diffs(options = {}) - Enumerator.new do |y| - diff_from_parent(options).each do |diff| - y << Gitlab::Git::Diff.new(diff) - end + result = DiffCollection.new(options) + diff_from_parent(options).each do |diff| + result.add(Gitlab::Git::Diff.new(diff)) + break if result.full? end + result end def parents diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index c396f5c..2799589 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -1,13 +1,12 @@ module Gitlab module Git class Compare - attr_reader :commits, :diffs, :same, :timeout, :head, :base + attr_reader :commits, :same, :head, :base def initialize(repository, base, head) - @commits, @diffs = [], [] + @commits= [] @same = false @repository = repository - @timeout = false return unless base && head @@ -24,21 +23,17 @@ module Gitlab @commits = Gitlab::Git::Commit.between(repository, @base.id, @head.id) end - def diffs(paths = nil, options = {}) + def diffs(options = {}) unless @head && @base - return [] + return Gitlab::Git::DiffCollection.empty end # Try to collect diff only if diffs is empty # Otherwise return cached version - if @diffs.first.nil? && @timeout == false - begin + if @diffs.nil? + paths = options.delete(:paths) || [] @diffs = Gitlab::Git::Diff.between(@repository, @head.id, @base.id, - options, *paths) - rescue Gitlab::Git::Diff::TimeoutError => ex - @diffs = [] - @timeout = true - end + options, *paths) end @diffs @@ -47,7 +42,7 @@ module Gitlab # Check if diff is empty because it is actually empty # and not because its impossible to get it def empty_diff? - diffs.first.nil? && timeout == false + diffs.to_a.empty? end end end diff --git a/lib/gitlab_git/diff.rb b/lib/gitlab_git/diff.rb index 04154f3..da2344b 100644 --- a/lib/gitlab_git/diff.rb +++ b/lib/gitlab_git/diff.rb @@ -130,7 +130,8 @@ module Gitlab :disable_pathspec_match, :deltas_are_icase, :include_untracked_content, :skip_binary_check, :include_typechange, :include_typechange_trees, - :ignore_filemode, :recurse_ignored_dirs, :paths] + :ignore_filemode, :recurse_ignored_dirs, :paths, + :max_files, :max_lines, :all_diffs] if default_options actual_defaults = default_options.dup @@ -188,7 +189,7 @@ module Gitlab end def size - @size ||= @diff.scan(/\n/).count + @size ||= Count.lines(@diff) end private diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb new file mode 100644 index 0000000..a8cc806 --- /dev/null +++ b/lib/gitlab_git/diff_collection.rb @@ -0,0 +1,58 @@ +module Gitlab + module Git + class DiffCollection + class TooManyDiffs < ::StandardError ; end + + def self.empty + new(all_diffs: true) + end + + def initialize(options) + @max_files = options.delete(:max_files) + @max_lines = options.delete(:max_lines) + @all_diffs = options.delete(:all_diffs) + if !@all_diffs && !(@max_files && @max_lines) + raise 'You must pass both :max_files and :max_lines or set :all_files to true.' + end + + @file_count, @line_count = 0, 0 + @array = Array.new + end + + def add(diff) + raise TooManyDiffs, "Can only hold #@max_files files and #@max_lines lines." if full? + @file_count += 1 + @line_count += diff.size + if !too_many_files? && !too_many_lines? + @array << diff + end + end + + def full? + too_many_files? + end + + def too_many_files? + !@all_diffs && (@file_count > @max_files) + end + + def too_many_lines? + !@all_diffs && (@line_count > @max_lines) + end + + def real_size + too_many_files? ? "#{@max_files}+" : @file_count.to_s + end + + def to_a + @array + end + + def map! + @array.each_with_index do |elt, i| + @array[i] = yield(elt) + end + end + end + end +end \ No newline at end of file diff --git a/lib/gitlab_git/repository.rb b/lib/gitlab_git/repository.rb index 31672bc..040310a 100644 --- a/lib/gitlab_git/repository.rb +++ b/lib/gitlab_git/repository.rb @@ -315,11 +315,12 @@ module Gitlab # diff options. The +options+ hash can also include :break_rewrites to # split larger rewrites into delete/add pairs. def diff(from, to, options = {}, *paths) - Enumerator.new do |y| - diff_patches(from, to, options, *paths).each do |p| - y << Gitlab::Git::Diff.new(p) - end + result = DiffCollection.new(options) + diff_patches(from, to, options, *paths).each do |p| + result.add(Gitlab::Git::Diff.new(p)) + break if result.full? end + result end # Return the diff between +from+ and +to+ in a single patch string. The diff --git a/spec/commit_spec.rb b/spec/commit_spec.rb index e2a1435..34e86b1 100644 --- a/spec/commit_spec.rb +++ b/spec/commit_spec.rb @@ -318,11 +318,11 @@ describe Gitlab::Git::Commit do end describe :diffs do - subject { commit.diffs } + subject { commit.diffs(all_diffs: true) } - it { should be_kind_of Enumerator } + it { should be_kind_of Gitlab::Git::DiffCollection } it { subject.to_a.size.should eq(2) } - its(:first) { should be_kind_of Gitlab::Git::Diff } + it { subject.to_a.first.should be_kind_of Gitlab::Git::Diff } end describe :ref_names do diff --git a/spec/compare_spec.rb b/spec/compare_spec.rb index 12ab017..c9076c2 100644 --- a/spec/compare_spec.rb +++ b/spec/compare_spec.rb @@ -16,20 +16,19 @@ describe Gitlab::Git::Compare do describe :diffs do subject do - compare.diffs.map(&:new_path) + compare.diffs(all_diffs: true).map!(&:new_path) end it { should have(10).elements } it { should include('files/ruby/popen.rb') } it { should_not include('LICENSE') } - it { compare.timeout.should be_false } - it { compare.empty_diff?.should be_false } + it { subject; compare.empty_diff?.should be_false } end describe 'non-existing refs' do let(:compare) { Gitlab::Git::Compare.new(repository, 'no-such-branch', '1234567890') } it { compare.commits.should be_empty } - it { compare.diffs.should be_empty } + it { compare.diffs.to_a.should be_empty } end end diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb new file mode 100644 index 0000000..0cd44c3 --- /dev/null +++ b/spec/diff_collection_spec.rb @@ -0,0 +1,138 @@ +require 'spec_helper' + +describe Gitlab::Git::DiffCollection do + subject do + Gitlab::Git::DiffCollection.new( + max_files: max_files, + max_lines: max_lines, + all_diffs: all_diffs, + ) + end + let(:max_files) { 10 } + let(:max_lines) { 100 } + let(:all_diffs) { false } + + its(:to_a) { should be_kind_of ::Array } + + describe :map! do + it 'modifies the array in place' do + 3.times { subject.add fake_diff(1) } + count = 0 + subject.map! { |d| !d.nil? && count += 1 } + subject.to_a.should eq([1, 2, 3]) + end + end + + context 'overflow handling' do + let(:file_count) { 0 } + before do + file_count.times do + subject.add(fake_diff(line_count)) + break if subject.full? + end + end + + it 'raises an exception when adding too much' do + expect { 20.times { subject.add(fake_diff(1)) } }.to raise_error + end + + context 'adding few enough files' do + let(:file_count) { 3 } + + context 'and few enough lines' do + let(:line_count) { 10 } + + its(:too_many_files?) { should be_false } + its(:too_many_lines?) { should be_false } + its(:real_size) { should eq('3') } + it { subject.to_a.size.should eq(3) } + + context 'when limiting is disabled' do + let(:all_diffs) { true } + + its(:too_many_files?) { should be_false } + its(:too_many_lines?) { should be_false } + its(:real_size) { should eq('3') } + it { subject.to_a.size.should eq(3) } + end + end + + context 'and too many lines' do + let(:line_count) { 1000 } + + its(:too_many_files?) { should be_false } + its(:too_many_lines?) { should be_true } + its(:real_size) { should eq('3') } + it { subject.to_a.size.should eq(0) } + + context 'when limiting is disabled' do + let(:all_diffs) { true } + + its(:too_many_files?) { should be_false } + its(:too_many_lines?) { should be_false } + its(:real_size) { should eq('3') } + it { subject.to_a.size.should eq(3) } + end + end + end + + context 'adding too many files' do + let(:file_count) { 11 } + + context 'and few enough lines' do + let(:line_count) { 1 } + + its(:too_many_files?) { should be_true } + its(:too_many_lines?) { should be_false } + its(:real_size) { should eq('10+') } + it { subject.to_a.size.should eq(10) } + + context 'when limiting is disabled' do + let(:all_diffs) { true } + + its(:too_many_files?) { should be_false } + its(:too_many_lines?) { should be_false } + its(:real_size) { should eq('11') } + it { subject.to_a.size.should eq(11) } + end + end + + context 'and too many lines' do + let(:line_count) { 30 } + + its(:too_many_files?) { should be_true } + its(:too_many_lines?) { should be_true } + its(:real_size) { should eq('10+') } + it { subject.to_a.size.should eq(3) } + + context 'when limiting is disabled' do + let(:all_diffs) { true } + + its(:too_many_files?) { should be_false } + its(:too_many_lines?) { should be_false } + its(:real_size) { should eq('11') } + it { subject.to_a.size.should eq(11) } + end + end + end + + context 'adding exactly the maximum number of files' do + let(:file_count) { 10 } + + context 'and few enough lines' do + let(:line_count) { 1 } + + its(:too_many_files?) { should be_false } + its(:too_many_lines?) { should be_false } + its(:real_size) { should eq('10') } + it { subject.to_a.size.should eq(10) } + end + end + end + + FakeDiff = Struct.new(:size) + + def fake_diff(line_count) + FakeDiff.new(line_count) + end +end \ No newline at end of file diff --git a/spec/diff_spec.rb b/spec/diff_spec.rb index c4ad421..923be18 100644 --- a/spec/diff_spec.rb +++ b/spec/diff_spec.rb @@ -49,14 +49,14 @@ EOT end describe :between do - let(:diffs) { Gitlab::Git::Diff.between(repository, 'feature', 'master') } + let(:diffs) { Gitlab::Git::Diff.between(repository, 'feature', 'master', all_diffs: true) } subject { diffs } - it { should be_kind_of Enumerator } + it { should be_kind_of Gitlab::Git::DiffCollection } it { subject.to_a.size.should eq(1) } context :diff do - subject { diffs.first } + subject { diffs.to_a.first } it { should be_kind_of Gitlab::Git::Diff } its(:new_path) { should == 'files/ruby/feature.rb' } diff --git a/spec/repository_spec.rb b/spec/repository_spec.rb index 4830978..e0044ea 100644 --- a/spec/repository_spec.rb +++ b/spec/repository_spec.rb @@ -542,7 +542,7 @@ describe Gitlab::Git::Repository do it "should contain the same diffs as #diff" do diff_text = repo.diff_text("master", "feature") diff_text = encode_utf8(diff_text) - repo.diff("master", "feature").each do |single_diff| + repo.diff("master", "feature", all_diffs: true).to_a.each do |single_diff| expect(diff_text.include?(single_diff.diff)).to be_true end end -- GitLab From 7c9a4c1896f73294e3efe82edce140c34a02030b Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 10 Feb 2016 09:25:31 +0100 Subject: [PATCH 06/33] Fix error message --- lib/gitlab_git/diff_collection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index a8cc806..4334592 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -12,7 +12,7 @@ module Gitlab @max_lines = options.delete(:max_lines) @all_diffs = options.delete(:all_diffs) if !@all_diffs && !(@max_files && @max_lines) - raise 'You must pass both :max_files and :max_lines or set :all_files to true.' + raise 'You must pass both :max_files and :max_lines or set :all_diffs to true.' end @file_count, @line_count = 0, 0 -- GitLab From dc1ab7f7597805bbd7f22a73219ccbfd035c3e39 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 25 Feb 2016 15:43:15 +0100 Subject: [PATCH 07/33] Make DiffCollection a smart enumerator --- lib/gitlab_git/commit.rb | 7 +----- lib/gitlab_git/diff_collection.rb | 42 ++++++++++++++++++------------- lib/gitlab_git/repository.rb | 7 +----- spec/diff_collection_spec.rb | 31 +++++++++-------------- 4 files changed, 38 insertions(+), 49 deletions(-) diff --git a/lib/gitlab_git/commit.rb b/lib/gitlab_git/commit.rb index ca2535d..bedf82f 100644 --- a/lib/gitlab_git/commit.rb +++ b/lib/gitlab_git/commit.rb @@ -205,12 +205,7 @@ module Gitlab end def diffs(options = {}) - result = DiffCollection.new(options) - diff_from_parent(options).each do |diff| - result.add(Gitlab::Git::Diff.new(diff)) - break if result.full? - end - result + DiffCollection.new(diff_from_parent(options), options) end def parents diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 4334592..31b4ec5 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -1,13 +1,14 @@ module Gitlab module Git class DiffCollection - class TooManyDiffs < ::StandardError ; end + include Enumerable def self.empty - new(all_diffs: true) + new([], all_diffs: true) end - def initialize(options) + def initialize(iterator, options) + @iterator = iterator @max_files = options.delete(:max_files) @max_lines = options.delete(:max_lines) @all_diffs = options.delete(:all_diffs) @@ -19,17 +20,22 @@ module Gitlab @array = Array.new end - def add(diff) - raise TooManyDiffs, "Can only hold #@max_files files and #@max_lines lines." if full? - @file_count += 1 - @line_count += diff.size - if !too_many_files? && !too_many_lines? - @array << diff - end - end + def each + @iterator.each_with_index do |raw, i| + if !@array[i].nil? + yield @array[i] + next + end + + @file_count += 1 + break if too_many_files? + + diff = Gitlab::Git::Diff.new(raw) + @line_count += diff.size + break if too_many_lines? - def full? - too_many_files? + yield @array[i] = diff + end end def too_many_files? @@ -41,15 +47,15 @@ module Gitlab end def real_size - too_many_files? ? "#{@max_files}+" : @file_count.to_s - end + return @file_count.to_s if @all_diffs - def to_a - @array + result = [@max_files, @file_count].min.to_s + result << '+' if too_many_files? || too_many_lines? + result end def map! - @array.each_with_index do |elt, i| + each_with_index do |elt, i| @array[i] = yield(elt) end end diff --git a/lib/gitlab_git/repository.rb b/lib/gitlab_git/repository.rb index 80c4944..cf87b4a 100644 --- a/lib/gitlab_git/repository.rb +++ b/lib/gitlab_git/repository.rb @@ -320,12 +320,7 @@ module Gitlab # diff options. The +options+ hash can also include :break_rewrites to # split larger rewrites into delete/add pairs. def diff(from, to, options = {}, *paths) - result = DiffCollection.new(options) - diff_patches(from, to, options, *paths).each do |p| - result.add(Gitlab::Git::Diff.new(p)) - break if result.full? - end - result + DiffCollection.new(diff_patches(from, to, options, *paths), options) end # Return the diff between +from+ and +to+ in a single patch string. The diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb index 0cd44c3..4b8fc67 100644 --- a/spec/diff_collection_spec.rb +++ b/spec/diff_collection_spec.rb @@ -3,11 +3,15 @@ require 'spec_helper' describe Gitlab::Git::DiffCollection do subject do Gitlab::Git::DiffCollection.new( + iterator, max_files: max_files, max_lines: max_lines, all_diffs: all_diffs, ) end + let(:iterator) { Array.new(file_count, fake_diff(line_count)) } + let(:file_count) { 0 } + let(:line_count) { 1 } let(:max_files) { 10 } let(:max_lines) { 100 } let(:all_diffs) { false } @@ -15,8 +19,9 @@ describe Gitlab::Git::DiffCollection do its(:to_a) { should be_kind_of ::Array } describe :map! do + let(:file_count) { 3} + it 'modifies the array in place' do - 3.times { subject.add fake_diff(1) } count = 0 subject.map! { |d| !d.nil? && count += 1 } subject.to_a.should eq([1, 2, 3]) @@ -24,17 +29,7 @@ describe Gitlab::Git::DiffCollection do end context 'overflow handling' do - let(:file_count) { 0 } - before do - file_count.times do - subject.add(fake_diff(line_count)) - break if subject.full? - end - end - - it 'raises an exception when adding too much' do - expect { 20.times { subject.add(fake_diff(1)) } }.to raise_error - end + before { subject.to_a } context 'adding few enough files' do let(:file_count) { 3 } @@ -62,7 +57,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } - its(:real_size) { should eq('3') } + its(:real_size) { should eq('1+') } it { subject.to_a.size.should eq(0) } context 'when limiting is disabled' do @@ -100,9 +95,9 @@ describe Gitlab::Git::DiffCollection do context 'and too many lines' do let(:line_count) { 30 } - its(:too_many_files?) { should be_true } + its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } - its(:real_size) { should eq('10+') } + its(:real_size) { should eq('4+') } it { subject.to_a.size.should eq(3) } context 'when limiting is disabled' do @@ -130,9 +125,7 @@ describe Gitlab::Git::DiffCollection do end end - FakeDiff = Struct.new(:size) - def fake_diff(line_count) - FakeDiff.new(line_count) + {'diff' => "DIFF\n" * line_count} end -end \ No newline at end of file +end -- GitLab From 952c1a8f124df5fc97cfb54b262018bb36008a81 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 25 Feb 2016 16:02:51 +0100 Subject: [PATCH 08/33] Simplify some things --- lib/gitlab_git/compare.rb | 2 +- spec/commit_spec.rb | 4 ++-- spec/compare_spec.rb | 2 +- spec/diff_collection_spec.rb | 18 +++++++++--------- spec/diff_spec.rb | 4 ++-- spec/repository_spec.rb | 2 +- 6 files changed, 16 insertions(+), 16 deletions(-) diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index 2799589..b61f4b4 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -42,7 +42,7 @@ module Gitlab # Check if diff is empty because it is actually empty # and not because its impossible to get it def empty_diff? - diffs.to_a.empty? + !diffs.any? end end end diff --git a/spec/commit_spec.rb b/spec/commit_spec.rb index 34e86b1..fd86b28 100644 --- a/spec/commit_spec.rb +++ b/spec/commit_spec.rb @@ -321,8 +321,8 @@ describe Gitlab::Git::Commit do subject { commit.diffs(all_diffs: true) } it { should be_kind_of Gitlab::Git::DiffCollection } - it { subject.to_a.size.should eq(2) } - it { subject.to_a.first.should be_kind_of Gitlab::Git::Diff } + it { subject.count.should eq(2) } + it { subject.first.should be_kind_of Gitlab::Git::Diff } end describe :ref_names do diff --git a/spec/compare_spec.rb b/spec/compare_spec.rb index c9076c2..f417c90 100644 --- a/spec/compare_spec.rb +++ b/spec/compare_spec.rb @@ -29,6 +29,6 @@ describe Gitlab::Git::Compare do let(:compare) { Gitlab::Git::Compare.new(repository, 'no-such-branch', '1234567890') } it { compare.commits.should be_empty } - it { compare.diffs.to_a.should be_empty } + it { compare.diffs(all_diffs: true).to_a.should be_empty } end end diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb index 4b8fc67..e98fb4c 100644 --- a/spec/diff_collection_spec.rb +++ b/spec/diff_collection_spec.rb @@ -40,7 +40,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } - it { subject.to_a.size.should eq(3) } + it { subject.count.should eq(3) } context 'when limiting is disabled' do let(:all_diffs) { true } @@ -48,7 +48,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } - it { subject.to_a.size.should eq(3) } + it { subject.count.should eq(3) } end end @@ -58,7 +58,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } its(:real_size) { should eq('1+') } - it { subject.to_a.size.should eq(0) } + it { subject.count.should eq(0) } context 'when limiting is disabled' do let(:all_diffs) { true } @@ -66,7 +66,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } - it { subject.to_a.size.should eq(3) } + it { subject.count.should eq(3) } end end end @@ -80,7 +80,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_true } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('10+') } - it { subject.to_a.size.should eq(10) } + it { subject.count.should eq(10) } context 'when limiting is disabled' do let(:all_diffs) { true } @@ -88,7 +88,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('11') } - it { subject.to_a.size.should eq(11) } + it { subject.count.should eq(11) } end end @@ -98,7 +98,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } its(:real_size) { should eq('4+') } - it { subject.to_a.size.should eq(3) } + it { subject.count.should eq(3) } context 'when limiting is disabled' do let(:all_diffs) { true } @@ -106,7 +106,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('11') } - it { subject.to_a.size.should eq(11) } + it { subject.count.should eq(11) } end end end @@ -120,7 +120,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('10') } - it { subject.to_a.size.should eq(10) } + it { subject.count.should eq(10) } end end end diff --git a/spec/diff_spec.rb b/spec/diff_spec.rb index 923be18..6acc7c7 100644 --- a/spec/diff_spec.rb +++ b/spec/diff_spec.rb @@ -53,10 +53,10 @@ EOT subject { diffs } it { should be_kind_of Gitlab::Git::DiffCollection } - it { subject.to_a.size.should eq(1) } + it { subject.count.should eq(1) } context :diff do - subject { diffs.to_a.first } + subject { diffs.first } it { should be_kind_of Gitlab::Git::Diff } its(:new_path) { should == 'files/ruby/feature.rb' } diff --git a/spec/repository_spec.rb b/spec/repository_spec.rb index 8c70856..fceb2d3 100644 --- a/spec/repository_spec.rb +++ b/spec/repository_spec.rb @@ -542,7 +542,7 @@ describe Gitlab::Git::Repository do it "should contain the same diffs as #diff" do diff_text = repo.diff_text("master", "feature") diff_text = encode_utf8(diff_text) - repo.diff("master", "feature", all_diffs: true).to_a.each do |single_diff| + repo.diff("master", "feature", all_diffs: true).each do |single_diff| expect(diff_text.include?(single_diff.diff)).to be_true end end -- GitLab From f2d20dc8f878e5f7865e6115311d8355269cb3f0 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 25 Feb 2016 16:05:44 +0100 Subject: [PATCH 09/33] Use "its" --- spec/diff_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/diff_spec.rb b/spec/diff_spec.rb index 6acc7c7..b4b38cf 100644 --- a/spec/diff_spec.rb +++ b/spec/diff_spec.rb @@ -53,7 +53,7 @@ EOT subject { diffs } it { should be_kind_of Gitlab::Git::DiffCollection } - it { subject.count.should eq(1) } + its(:count) { should eq(1) } context :diff do subject { diffs.first } -- GitLab From 996fc1a043aa1b10f8b109cf38a42ffd56d22a45 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 25 Feb 2016 16:48:09 +0100 Subject: [PATCH 10/33] Rubocop --- lib/gitlab_git/compare.rb | 6 +++--- lib/gitlab_git/diff_collection.rb | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index b61f4b4..fd20898 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -31,9 +31,9 @@ module Gitlab # Try to collect diff only if diffs is empty # Otherwise return cached version if @diffs.nil? - paths = options.delete(:paths) || [] - @diffs = Gitlab::Git::Diff.between(@repository, @head.id, @base.id, - options, *paths) + paths = options.delete(:paths) || [] + @diffs = Gitlab::Git::Diff.between(@repository, @head.id, @base.id, + options, *paths) end @diffs diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 31b4ec5..2855a67 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -61,4 +61,4 @@ module Gitlab end end end -end \ No newline at end of file +end -- GitLab From 4b33eba3e027ce493568dcd556ecde93cab96ee8 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 25 Feb 2016 16:51:47 +0100 Subject: [PATCH 11/33] Rename Diff#size to Diff#line_count --- lib/gitlab_git/diff.rb | 4 ++-- lib/gitlab_git/diff_collection.rb | 2 +- spec/diff_spec.rb | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/gitlab_git/diff.rb b/lib/gitlab_git/diff.rb index da2344b..5fc47ab 100644 --- a/lib/gitlab_git/diff.rb +++ b/lib/gitlab_git/diff.rb @@ -188,8 +188,8 @@ module Gitlab a_mode == '160000' || b_mode == '160000' end - def size - @size ||= Count.lines(@diff) + def line_count + @line_count ||= Count.lines(@diff) end private diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 2855a67..faf4324 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -31,7 +31,7 @@ module Gitlab break if too_many_files? diff = Gitlab::Git::Diff.new(raw) - @line_count += diff.size + @line_count += diff.line_count break if too_many_lines? yield @array[i] = diff diff --git a/spec/diff_spec.rb b/spec/diff_spec.rb index b4b38cf..8fe6419 100644 --- a/spec/diff_spec.rb +++ b/spec/diff_spec.rb @@ -107,9 +107,9 @@ EOT it { Gitlab::Git::Diff.new(@diffs[1]).submodule?.should == true } end - describe :size do + describe :line_count do subject { Gitlab::Git::Diff.new(@rugged_diff) } - its(:size) { should eq(9) } + its(:line_count) { should eq(9) } end end -- GitLab From a339019431eb38fe1721144b3f7b3e6d1c823713 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 25 Feb 2016 17:40:29 +0100 Subject: [PATCH 12/33] Simplify Compare#diffs further --- lib/gitlab_git/compare.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index fd20898..21c4fd9 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -28,15 +28,11 @@ module Gitlab return Gitlab::Git::DiffCollection.empty end - # Try to collect diff only if diffs is empty - # Otherwise return cached version - if @diffs.nil? + @diffs ||= begin paths = options.delete(:paths) || [] - @diffs = Gitlab::Git::Diff.between(@repository, @head.id, @base.id, + Gitlab::Git::Diff.between(@repository, @head.id, @base.id, options, *paths) end - - @diffs end # Check if diff is empty because it is actually empty -- GitLab From 45ffef74b2d3dd31b2ea1ca9f609f3950c9f24d5 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 26 Feb 2016 11:51:38 +0100 Subject: [PATCH 13/33] Improve counting, real_size an int --- lib/gitlab_git/diff_collection.rb | 42 ++++++++++++++++++++++++------- spec/diff_collection_spec.rb | 36 +++++++++++++------------- 2 files changed, 51 insertions(+), 27 deletions(-) diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index faf4324..1c2b070 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -28,30 +28,36 @@ module Gitlab end @file_count += 1 - break if too_many_files? + break if _too_many_files? diff = Gitlab::Git::Diff.new(raw) @line_count += diff.line_count - break if too_many_lines? + break if _too_many_lines? yield @array[i] = diff end end def too_many_files? - !@all_diffs && (@file_count > @max_files) + return @too_many_files unless @too_many_files.nil? + + populate! + @too_many_files = _too_many_files? end def too_many_lines? - !@all_diffs && (@line_count > @max_lines) + return @too_many_lines unless @too_many_lines.nil? + + populate! + @too_many_lines = _too_many_lines? end - def real_size - return @file_count.to_s if @all_diffs + def size + @size ||= count # forces a loop through @iterator + end - result = [@max_files, @file_count].min.to_s - result << '+' if too_many_files? || too_many_lines? - result + def real_size + @real_size ||= @iterator.count end def map! @@ -59,6 +65,24 @@ module Gitlab @array[i] = yield(elt) end end + + private + + def populate! + return if @populated + + each { nil } # force a loop through all diffs + @populated = true + nil + end + + def _too_many_files? + !@all_diffs && (@file_count > @max_files) + end + + def _too_many_lines? + !@all_diffs && (@line_count > @max_lines) + end end end end diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb index e98fb4c..3f9b8b1 100644 --- a/spec/diff_collection_spec.rb +++ b/spec/diff_collection_spec.rb @@ -39,16 +39,16 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq('3') } - it { subject.count.should eq(3) } + its(:real_size) { should eq(3) } + it { subject.size.should eq(3) } context 'when limiting is disabled' do let(:all_diffs) { true } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq('3') } - it { subject.count.should eq(3) } + its(:real_size) { should eq(3) } + it { subject.size.should eq(3) } end end @@ -57,16 +57,16 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } - its(:real_size) { should eq('1+') } - it { subject.count.should eq(0) } + its(:real_size) { should eq(3) } + it { subject.size.should eq(0) } context 'when limiting is disabled' do let(:all_diffs) { true } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq('3') } - it { subject.count.should eq(3) } + its(:real_size) { should eq(3) } + it { subject.size.should eq(3) } end end end @@ -79,16 +79,16 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_true } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq('10+') } - it { subject.count.should eq(10) } + its(:real_size) { should eq(11) } + it { subject.size.should eq(10) } context 'when limiting is disabled' do let(:all_diffs) { true } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq('11') } - it { subject.count.should eq(11) } + its(:real_size) { should eq(11) } + it { subject.size.should eq(11) } end end @@ -97,16 +97,16 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } - its(:real_size) { should eq('4+') } - it { subject.count.should eq(3) } + its(:real_size) { should eq(11) } + it { subject.size.should eq(3) } context 'when limiting is disabled' do let(:all_diffs) { true } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq('11') } - it { subject.count.should eq(11) } + its(:real_size) { should eq(11) } + it { subject.size.should eq(11) } end end end @@ -119,8 +119,8 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq('10') } - it { subject.count.should eq(10) } + its(:real_size) { should eq(10) } + it { subject.size.should eq(10) } end end end -- GitLab From e60f9fdf450a3bc53897001f71089ae1582e323b Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 26 Feb 2016 12:45:05 +0100 Subject: [PATCH 14/33] Counting @iterator is a bad idea after all --- lib/gitlab_git/diff_collection.rb | 45 ++++++++++++++++++------------- spec/diff_collection_spec.rb | 20 +++++++------- 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 1c2b070..32b7be6 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -16,40 +16,50 @@ module Gitlab raise 'You must pass both :max_files and :max_lines or set :all_diffs to true.' end - @file_count, @line_count = 0, 0 + @line_count = 0 + @overflow = false @array = Array.new end def each @iterator.each_with_index do |raw, i| + # First yield cached Diff instances from @array if !@array[i].nil? yield @array[i] next end - @file_count += 1 - break if _too_many_files? + # We have exhausted @array, time to create new Diff instances or stop. + break if @overflow + if !@all_diffs && i >= @max_files + @overflow = true + break + end + + # Going by the number of files alone it is OK to create a new Diff instance. diff = Gitlab::Git::Diff.new(raw) + @line_count += diff.line_count - break if _too_many_lines? + if !@all_diffs && @line_count >= @max_lines + # This last Diff instance pushes us over the lines limit. We stop and + # discard it. + @overflow = true + break + end yield @array[i] = diff end end def too_many_files? - return @too_many_files unless @too_many_files.nil? - populate! - @too_many_files = _too_many_files? + !@all_diffs && @overflow && size >= @max_files end def too_many_lines? - return @too_many_lines unless @too_many_lines.nil? - populate! - @too_many_lines = _too_many_lines? + !@all_diffs && @overflow && @line_count >= @max_lines end def size @@ -57,7 +67,12 @@ module Gitlab end def real_size - @real_size ||= @iterator.count + populate! + return size.to_s if @all_diffs + + result = [@max_files, size].min.to_s + result << '+' if @overflow + result end def map! @@ -75,14 +90,6 @@ module Gitlab @populated = true nil end - - def _too_many_files? - !@all_diffs && (@file_count > @max_files) - end - - def _too_many_lines? - !@all_diffs && (@line_count > @max_lines) - end end end end diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb index 3f9b8b1..ba317d0 100644 --- a/spec/diff_collection_spec.rb +++ b/spec/diff_collection_spec.rb @@ -29,8 +29,6 @@ describe Gitlab::Git::DiffCollection do end context 'overflow handling' do - before { subject.to_a } - context 'adding few enough files' do let(:file_count) { 3 } @@ -39,7 +37,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq(3) } + its(:real_size) { should eq('3') } it { subject.size.should eq(3) } context 'when limiting is disabled' do @@ -47,7 +45,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq(3) } + its(:real_size) { should eq('3') } it { subject.size.should eq(3) } end end @@ -57,7 +55,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } - its(:real_size) { should eq(3) } + its(:real_size) { should eq('0+') } it { subject.size.should eq(0) } context 'when limiting is disabled' do @@ -65,7 +63,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq(3) } + its(:real_size) { should eq('3') } it { subject.size.should eq(3) } end end @@ -79,7 +77,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_true } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq(11) } + its(:real_size) { should eq('10+') } it { subject.size.should eq(10) } context 'when limiting is disabled' do @@ -87,7 +85,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq(11) } + its(:real_size) { should eq('11') } it { subject.size.should eq(11) } end end @@ -97,7 +95,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } - its(:real_size) { should eq(11) } + its(:real_size) { should eq('3+') } it { subject.size.should eq(3) } context 'when limiting is disabled' do @@ -105,7 +103,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq(11) } + its(:real_size) { should eq('11') } it { subject.size.should eq(11) } end end @@ -119,7 +117,7 @@ describe Gitlab::Git::DiffCollection do its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } - its(:real_size) { should eq(10) } + its(:real_size) { should eq('10') } it { subject.size.should eq(10) } end end -- GitLab From 6615d0998a21287b52f49319fab2e53ca889ff7b Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 26 Feb 2016 13:35:36 +0100 Subject: [PATCH 15/33] Simplify #real_size --- lib/gitlab_git/diff_collection.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 32b7be6..c1351cb 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -70,9 +70,11 @@ module Gitlab populate! return size.to_s if @all_diffs - result = [@max_files, size].min.to_s - result << '+' if @overflow - result + if @overflow + "#{size}+" + else + size.to_s + end end def map! -- GitLab From 9f7d490db5b0c60135f5caee040cf99c677e0325 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 26 Feb 2016 14:37:31 +0100 Subject: [PATCH 16/33] Don't cache diffs on Compare instances --- lib/gitlab_git/compare.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index 21c4fd9..a9d3f01 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -28,11 +28,8 @@ module Gitlab return Gitlab::Git::DiffCollection.empty end - @diffs ||= begin - paths = options.delete(:paths) || [] - Gitlab::Git::Diff.between(@repository, @head.id, @base.id, - options, *paths) - end + paths = options.delete(:paths) || [] + Gitlab::Git::Diff.between(@repository, @head.id, @base.id, options, *paths) end # Check if diff is empty because it is actually empty -- GitLab From d2f15c61a4ebabdfabfe16cdb5658aaf80c5dbe8 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 26 Feb 2016 16:44:22 +0100 Subject: [PATCH 17/33] Whitespace --- lib/gitlab_git/diff_collection.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index c1351cb..a241859 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -28,7 +28,7 @@ module Gitlab yield @array[i] next end - + # We have exhausted @array, time to create new Diff instances or stop. break if @overflow @@ -36,7 +36,7 @@ module Gitlab @overflow = true break end - + # Going by the number of files alone it is OK to create a new Diff instance. diff = Gitlab::Git::Diff.new(raw) @@ -87,7 +87,7 @@ module Gitlab def populate! return if @populated - + each { nil } # force a loop through all diffs @populated = true nil -- GitLab From 29bdff29df16703b96d17a814cc9da8b145947a6 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 26 Feb 2016 19:02:26 +0100 Subject: [PATCH 18/33] Fix Compare#empty_diff? --- lib/gitlab_git/compare.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index a9d3f01..28d3821 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -35,7 +35,8 @@ module Gitlab # Check if diff is empty because it is actually empty # and not because its impossible to get it def empty_diff? - !diffs.any? + # It is OK to use 'all_diffs: true' because "any?" stops once it finds one. + !diffs(all_diffs: true).any? end end end -- GitLab From 91e8a25057fa88f8cb68966c9a8a5ec63ed5e0b5 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 29 Feb 2016 11:56:06 +0100 Subject: [PATCH 19/33] Remove all_diffs option and add default diff limits --- lib/gitlab_git/compare.rb | 3 +-- lib/gitlab_git/diff.rb | 2 +- lib/gitlab_git/diff_collection.rb | 19 +++++++--------- spec/commit_spec.rb | 2 +- spec/compare_spec.rb | 4 ++-- spec/diff_collection_spec.rb | 36 ------------------------------- spec/diff_spec.rb | 2 +- spec/repository_spec.rb | 2 +- 8 files changed, 15 insertions(+), 55 deletions(-) diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index 28d3821..a9d3f01 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -35,8 +35,7 @@ module Gitlab # Check if diff is empty because it is actually empty # and not because its impossible to get it def empty_diff? - # It is OK to use 'all_diffs: true' because "any?" stops once it finds one. - !diffs(all_diffs: true).any? + !diffs.any? end end end diff --git a/lib/gitlab_git/diff.rb b/lib/gitlab_git/diff.rb index 5fc47ab..847f5c9 100644 --- a/lib/gitlab_git/diff.rb +++ b/lib/gitlab_git/diff.rb @@ -131,7 +131,7 @@ module Gitlab :include_untracked_content, :skip_binary_check, :include_typechange, :include_typechange_trees, :ignore_filemode, :recurse_ignored_dirs, :paths, - :max_files, :max_lines, :all_diffs] + :max_files, :max_lines] if default_options actual_defaults = default_options.dup diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index a241859..3a1dabb 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -3,18 +3,16 @@ module Gitlab class DiffCollection include Enumerable + DEFAULT_LIMITS = {max_files: 100, max_lines: 5000}.freeze + def self.empty new([], all_diffs: true) end def initialize(iterator, options) @iterator = iterator - @max_files = options.delete(:max_files) - @max_lines = options.delete(:max_lines) - @all_diffs = options.delete(:all_diffs) - if !@all_diffs && !(@max_files && @max_lines) - raise 'You must pass both :max_files and :max_lines or set :all_diffs to true.' - end + @max_files = options.fetch(:max_files, DEFAULT_LIMITS[:max_files]) + @max_lines = options.fetch(:max_lines, DEFAULT_LIMITS[:max_lines]) @line_count = 0 @overflow = false @@ -32,7 +30,7 @@ module Gitlab # We have exhausted @array, time to create new Diff instances or stop. break if @overflow - if !@all_diffs && i >= @max_files + if i >= @max_files @overflow = true break end @@ -41,7 +39,7 @@ module Gitlab diff = Gitlab::Git::Diff.new(raw) @line_count += diff.line_count - if !@all_diffs && @line_count >= @max_lines + if @line_count >= @max_lines # This last Diff instance pushes us over the lines limit. We stop and # discard it. @overflow = true @@ -54,12 +52,12 @@ module Gitlab def too_many_files? populate! - !@all_diffs && @overflow && size >= @max_files + @overflow && size >= @max_files end def too_many_lines? populate! - !@all_diffs && @overflow && @line_count >= @max_lines + @overflow && @line_count >= @max_lines end def size @@ -68,7 +66,6 @@ module Gitlab def real_size populate! - return size.to_s if @all_diffs if @overflow "#{size}+" diff --git a/spec/commit_spec.rb b/spec/commit_spec.rb index fd86b28..6a62fa8 100644 --- a/spec/commit_spec.rb +++ b/spec/commit_spec.rb @@ -318,7 +318,7 @@ describe Gitlab::Git::Commit do end describe :diffs do - subject { commit.diffs(all_diffs: true) } + subject { commit.diffs } it { should be_kind_of Gitlab::Git::DiffCollection } it { subject.count.should eq(2) } diff --git a/spec/compare_spec.rb b/spec/compare_spec.rb index f417c90..ad512a8 100644 --- a/spec/compare_spec.rb +++ b/spec/compare_spec.rb @@ -16,7 +16,7 @@ describe Gitlab::Git::Compare do describe :diffs do subject do - compare.diffs(all_diffs: true).map!(&:new_path) + compare.diffs.map!(&:new_path) end it { should have(10).elements } @@ -29,6 +29,6 @@ describe Gitlab::Git::Compare do let(:compare) { Gitlab::Git::Compare.new(repository, 'no-such-branch', '1234567890') } it { compare.commits.should be_empty } - it { compare.diffs(all_diffs: true).to_a.should be_empty } + it { compare.diffs.to_a.should be_empty } end end diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb index ba317d0..26e1c67 100644 --- a/spec/diff_collection_spec.rb +++ b/spec/diff_collection_spec.rb @@ -39,15 +39,6 @@ describe Gitlab::Git::DiffCollection do its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } it { subject.size.should eq(3) } - - context 'when limiting is disabled' do - let(:all_diffs) { true } - - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_false } - its(:real_size) { should eq('3') } - it { subject.size.should eq(3) } - end end context 'and too many lines' do @@ -57,15 +48,6 @@ describe Gitlab::Git::DiffCollection do its(:too_many_lines?) { should be_true } its(:real_size) { should eq('0+') } it { subject.size.should eq(0) } - - context 'when limiting is disabled' do - let(:all_diffs) { true } - - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_false } - its(:real_size) { should eq('3') } - it { subject.size.should eq(3) } - end end end @@ -79,15 +61,6 @@ describe Gitlab::Git::DiffCollection do its(:too_many_lines?) { should be_false } its(:real_size) { should eq('10+') } it { subject.size.should eq(10) } - - context 'when limiting is disabled' do - let(:all_diffs) { true } - - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_false } - its(:real_size) { should eq('11') } - it { subject.size.should eq(11) } - end end context 'and too many lines' do @@ -97,15 +70,6 @@ describe Gitlab::Git::DiffCollection do its(:too_many_lines?) { should be_true } its(:real_size) { should eq('3+') } it { subject.size.should eq(3) } - - context 'when limiting is disabled' do - let(:all_diffs) { true } - - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_false } - its(:real_size) { should eq('11') } - it { subject.size.should eq(11) } - end end end diff --git a/spec/diff_spec.rb b/spec/diff_spec.rb index 8fe6419..a0690c4 100644 --- a/spec/diff_spec.rb +++ b/spec/diff_spec.rb @@ -49,7 +49,7 @@ EOT end describe :between do - let(:diffs) { Gitlab::Git::Diff.between(repository, 'feature', 'master', all_diffs: true) } + let(:diffs) { Gitlab::Git::Diff.between(repository, 'feature', 'master') } subject { diffs } it { should be_kind_of Gitlab::Git::DiffCollection } diff --git a/spec/repository_spec.rb b/spec/repository_spec.rb index fceb2d3..f33e06a 100644 --- a/spec/repository_spec.rb +++ b/spec/repository_spec.rb @@ -542,7 +542,7 @@ describe Gitlab::Git::Repository do it "should contain the same diffs as #diff" do diff_text = repo.diff_text("master", "feature") diff_text = encode_utf8(diff_text) - repo.diff("master", "feature", all_diffs: true).each do |single_diff| + repo.diff("master", "feature").each do |single_diff| expect(diff_text.include?(single_diff.diff)).to be_true end end -- GitLab From 34da2275421ffec046912f811085db1a210a0226 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 29 Feb 2016 14:59:32 +0100 Subject: [PATCH 20/33] Bring back the all_diffs option It is useful to limit the scope of the changes in gitlab itself. --- lib/gitlab_git/diff.rb | 2 +- lib/gitlab_git/diff_collection.rb | 9 ++++---- spec/diff_collection_spec.rb | 36 +++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/lib/gitlab_git/diff.rb b/lib/gitlab_git/diff.rb index 847f5c9..5fc47ab 100644 --- a/lib/gitlab_git/diff.rb +++ b/lib/gitlab_git/diff.rb @@ -131,7 +131,7 @@ module Gitlab :include_untracked_content, :skip_binary_check, :include_typechange, :include_typechange_trees, :ignore_filemode, :recurse_ignored_dirs, :paths, - :max_files, :max_lines] + :max_files, :max_lines, :all_diffs] if default_options actual_defaults = default_options.dup diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 3a1dabb..49f1a4b 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -13,6 +13,7 @@ module Gitlab @iterator = iterator @max_files = options.fetch(:max_files, DEFAULT_LIMITS[:max_files]) @max_lines = options.fetch(:max_lines, DEFAULT_LIMITS[:max_lines]) + @all_diffs = !!options.fetch(:all_diffs) @line_count = 0 @overflow = false @@ -30,7 +31,7 @@ module Gitlab # We have exhausted @array, time to create new Diff instances or stop. break if @overflow - if i >= @max_files + if !@all_diffs && i >= @max_files @overflow = true break end @@ -39,7 +40,7 @@ module Gitlab diff = Gitlab::Git::Diff.new(raw) @line_count += diff.line_count - if @line_count >= @max_lines + if !@all_diffs && @line_count >= @max_lines # This last Diff instance pushes us over the lines limit. We stop and # discard it. @overflow = true @@ -52,12 +53,12 @@ module Gitlab def too_many_files? populate! - @overflow && size >= @max_files + !@all_diffs && @overflow && size >= @max_files end def too_many_lines? populate! - @overflow && @line_count >= @max_lines + !@all_diffs && @overflow && @line_count >= @max_lines end def size diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb index 26e1c67..ba317d0 100644 --- a/spec/diff_collection_spec.rb +++ b/spec/diff_collection_spec.rb @@ -39,6 +39,15 @@ describe Gitlab::Git::DiffCollection do its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } it { subject.size.should eq(3) } + + context 'when limiting is disabled' do + let(:all_diffs) { true } + + its(:too_many_files?) { should be_false } + its(:too_many_lines?) { should be_false } + its(:real_size) { should eq('3') } + it { subject.size.should eq(3) } + end end context 'and too many lines' do @@ -48,6 +57,15 @@ describe Gitlab::Git::DiffCollection do its(:too_many_lines?) { should be_true } its(:real_size) { should eq('0+') } it { subject.size.should eq(0) } + + context 'when limiting is disabled' do + let(:all_diffs) { true } + + its(:too_many_files?) { should be_false } + its(:too_many_lines?) { should be_false } + its(:real_size) { should eq('3') } + it { subject.size.should eq(3) } + end end end @@ -61,6 +79,15 @@ describe Gitlab::Git::DiffCollection do its(:too_many_lines?) { should be_false } its(:real_size) { should eq('10+') } it { subject.size.should eq(10) } + + context 'when limiting is disabled' do + let(:all_diffs) { true } + + its(:too_many_files?) { should be_false } + its(:too_many_lines?) { should be_false } + its(:real_size) { should eq('11') } + it { subject.size.should eq(11) } + end end context 'and too many lines' do @@ -70,6 +97,15 @@ describe Gitlab::Git::DiffCollection do its(:too_many_lines?) { should be_true } its(:real_size) { should eq('3+') } it { subject.size.should eq(3) } + + context 'when limiting is disabled' do + let(:all_diffs) { true } + + its(:too_many_files?) { should be_false } + its(:too_many_lines?) { should be_false } + its(:real_size) { should eq('11') } + it { subject.size.should eq(11) } + end end end -- GitLab From b4da9ea377c6df91565a955208c28f1ba9828c98 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 29 Feb 2016 15:43:09 +0100 Subject: [PATCH 21/33] Provide default to Hash#fetch --- lib/gitlab_git/diff_collection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 49f1a4b..df97cc9 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -13,7 +13,7 @@ module Gitlab @iterator = iterator @max_files = options.fetch(:max_files, DEFAULT_LIMITS[:max_files]) @max_lines = options.fetch(:max_lines, DEFAULT_LIMITS[:max_lines]) - @all_diffs = !!options.fetch(:all_diffs) + @all_diffs = !!options.fetch(:all_diffs, false) @line_count = 0 @overflow = false -- GitLab From 320c717a45fa9e75cff766d496d842258cb2c9ad Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 1 Mar 2016 13:55:32 +0100 Subject: [PATCH 22/33] Add DiffCollection#overflow? --- lib/gitlab_git/diff_collection.rb | 5 +++++ spec/diff_collection_spec.rb | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index df97cc9..8e40a78 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -51,6 +51,11 @@ module Gitlab end end + def overflow? + populate! + !!@overflow + end + def too_many_files? populate! !@all_diffs && @overflow && size >= @max_files diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb index ba317d0..4167d70 100644 --- a/spec/diff_collection_spec.rb +++ b/spec/diff_collection_spec.rb @@ -35,6 +35,7 @@ describe Gitlab::Git::DiffCollection do context 'and few enough lines' do let(:line_count) { 10 } + its(:overflow?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } @@ -53,6 +54,7 @@ describe Gitlab::Git::DiffCollection do context 'and too many lines' do let(:line_count) { 1000 } + its(:overflow?) { should be_true } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } its(:real_size) { should eq('0+') } @@ -75,6 +77,7 @@ describe Gitlab::Git::DiffCollection do context 'and few enough lines' do let(:line_count) { 1 } + its(:overflow?) { should be_true } its(:too_many_files?) { should be_true } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('10+') } @@ -83,6 +86,7 @@ describe Gitlab::Git::DiffCollection do context 'when limiting is disabled' do let(:all_diffs) { true } + its(:overflow?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('11') } @@ -93,6 +97,7 @@ describe Gitlab::Git::DiffCollection do context 'and too many lines' do let(:line_count) { 30 } + its(:overflow?) { should be_true } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } its(:real_size) { should eq('3+') } @@ -101,6 +106,7 @@ describe Gitlab::Git::DiffCollection do context 'when limiting is disabled' do let(:all_diffs) { true } + its(:overflow?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('11') } @@ -115,6 +121,7 @@ describe Gitlab::Git::DiffCollection do context 'and few enough lines' do let(:line_count) { 1 } + its(:overflow?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('10') } -- GitLab From 4d7d7eea27eac4ed335acc5b9af843f2781c536b Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 1 Mar 2016 14:14:00 +0100 Subject: [PATCH 23/33] Add DiffCollection#empty? --- lib/gitlab_git/compare.rb | 2 +- lib/gitlab_git/diff_collection.rb | 4 ++++ spec/diff_collection_spec.rb | 20 ++++++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index a9d3f01..5b1c713 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -35,7 +35,7 @@ module Gitlab # Check if diff is empty because it is actually empty # and not because its impossible to get it def empty_diff? - !diffs.any? + diffs.empty? end end end diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 8e40a78..8ad189a 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -51,6 +51,10 @@ module Gitlab end end + def empty? + !@iterator.any? + end + def overflow? populate! !!@overflow diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb index 4167d70..eaa474f 100644 --- a/spec/diff_collection_spec.rb +++ b/spec/diff_collection_spec.rb @@ -36,6 +36,7 @@ describe Gitlab::Git::DiffCollection do let(:line_count) { 10 } its(:overflow?) { should be_false } + its(:empty?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } @@ -44,6 +45,8 @@ describe Gitlab::Git::DiffCollection do context 'when limiting is disabled' do let(:all_diffs) { true } + its(:overflow?) { should be_false } + its(:empty?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } @@ -55,6 +58,7 @@ describe Gitlab::Git::DiffCollection do let(:line_count) { 1000 } its(:overflow?) { should be_true } + its(:empty?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } its(:real_size) { should eq('0+') } @@ -63,6 +67,8 @@ describe Gitlab::Git::DiffCollection do context 'when limiting is disabled' do let(:all_diffs) { true } + its(:overflow?) { should be_false } + its(:empty?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } @@ -78,6 +84,7 @@ describe Gitlab::Git::DiffCollection do let(:line_count) { 1 } its(:overflow?) { should be_true } + its(:empty?) { should be_false } its(:too_many_files?) { should be_true } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('10+') } @@ -87,6 +94,7 @@ describe Gitlab::Git::DiffCollection do let(:all_diffs) { true } its(:overflow?) { should be_false } + its(:empty?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('11') } @@ -98,6 +106,7 @@ describe Gitlab::Git::DiffCollection do let(:line_count) { 30 } its(:overflow?) { should be_true } + its(:empty?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_true } its(:real_size) { should eq('3+') } @@ -107,6 +116,7 @@ describe Gitlab::Git::DiffCollection do let(:all_diffs) { true } its(:overflow?) { should be_false } + its(:empty?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('11') } @@ -122,6 +132,7 @@ describe Gitlab::Git::DiffCollection do let(:line_count) { 1 } its(:overflow?) { should be_false } + its(:empty?) { should be_false } its(:too_many_files?) { should be_false } its(:too_many_lines?) { should be_false } its(:real_size) { should eq('10') } @@ -130,6 +141,15 @@ describe Gitlab::Git::DiffCollection do end end + describe '::empty' do + subject { Gitlab::Git::DiffCollection.empty } + + its(:overflow?) { should be_false } + its(:empty?) { should be_true } + its(:size) { should eq(0) } + its(:real_size) { should eq('0')} + end + def fake_diff(line_count) {'diff' => "DIFF\n" * line_count} end -- GitLab From 7f1d5d25b59640febf9107b8c28f88848e101b9e Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 1 Mar 2016 14:43:45 +0100 Subject: [PATCH 24/33] Remove unused method It is used nowhere in gitlab --- lib/gitlab_git/compare.rb | 6 ------ spec/compare_spec.rb | 1 - 2 files changed, 7 deletions(-) diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index 5b1c713..fa63416 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -31,12 +31,6 @@ module Gitlab paths = options.delete(:paths) || [] Gitlab::Git::Diff.between(@repository, @head.id, @base.id, options, *paths) end - - # Check if diff is empty because it is actually empty - # and not because its impossible to get it - def empty_diff? - diffs.empty? - end end end end diff --git a/spec/compare_spec.rb b/spec/compare_spec.rb index ad512a8..60f5416 100644 --- a/spec/compare_spec.rb +++ b/spec/compare_spec.rb @@ -22,7 +22,6 @@ describe Gitlab::Git::Compare do it { should have(10).elements } it { should include('files/ruby/popen.rb') } it { should_not include('LICENSE') } - it { subject; compare.empty_diff?.should be_false } end describe 'non-existing refs' do -- GitLab From 31ee086ecb687282e5a024b21f631aaaab3ca6c9 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 1 Mar 2016 16:18:27 +0100 Subject: [PATCH 25/33] Rubocop --- lib/gitlab_git/diff_collection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 8ad189a..3a6cce5 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -3,7 +3,7 @@ module Gitlab class DiffCollection include Enumerable - DEFAULT_LIMITS = {max_files: 100, max_lines: 5000}.freeze + DEFAULT_LIMITS = { max_files: 100, max_lines: 5000 }.freeze def self.empty new([], all_diffs: true) -- GitLab From c1a1500e35c78727780e34b5ef18f9e79cb11e97 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 1 Mar 2016 16:22:35 +0100 Subject: [PATCH 26/33] Add missing space --- lib/gitlab_git/compare.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index fa63416..0f4d8fc 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -4,7 +4,7 @@ module Gitlab attr_reader :commits, :same, :head, :base def initialize(repository, base, head) - @commits= [] + @commits = [] @same = false @repository = repository -- GitLab From 6ef6ce6754ef52e7ca59b61401d4e53f78937ae1 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 2 Mar 2016 12:38:14 +0100 Subject: [PATCH 27/33] Remove DiffCollection.empty --- lib/gitlab_git/compare.rb | 2 +- lib/gitlab_git/diff_collection.rb | 6 +----- spec/diff_collection_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/gitlab_git/compare.rb b/lib/gitlab_git/compare.rb index 0f4d8fc..0e55fa1 100644 --- a/lib/gitlab_git/compare.rb +++ b/lib/gitlab_git/compare.rb @@ -25,7 +25,7 @@ module Gitlab def diffs(options = {}) unless @head && @base - return Gitlab::Git::DiffCollection.empty + return Gitlab::Git::DiffCollection.new([]) end paths = options.delete(:paths) || [] diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 3a6cce5..67ccbb4 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -5,11 +5,7 @@ module Gitlab DEFAULT_LIMITS = { max_files: 100, max_lines: 5000 }.freeze - def self.empty - new([], all_diffs: true) - end - - def initialize(iterator, options) + def initialize(iterator, options={}) @iterator = iterator @max_files = options.fetch(:max_files, DEFAULT_LIMITS[:max_files]) @max_lines = options.fetch(:max_lines, DEFAULT_LIMITS[:max_lines]) diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb index eaa474f..e8d7dbf 100644 --- a/spec/diff_collection_spec.rb +++ b/spec/diff_collection_spec.rb @@ -141,8 +141,8 @@ describe Gitlab::Git::DiffCollection do end end - describe '::empty' do - subject { Gitlab::Git::DiffCollection.empty } + describe 'empty collection' do + subject { Gitlab::Git::DiffCollection.new([]) } its(:overflow?) { should be_false } its(:empty?) { should be_true } -- GitLab From b30daf0706d4351a3b2d1aef675892c82a33e27e Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 2 Mar 2016 12:41:57 +0100 Subject: [PATCH 28/33] Rename Count.lines to Util.count_lines --- lib/gitlab_git.rb | 2 +- lib/gitlab_git/diff.rb | 2 +- lib/gitlab_git/{count.rb => util.rb} | 4 ++-- spec/{count_spec.rb => util_spec.rb} | 6 +++--- 4 files changed, 7 insertions(+), 7 deletions(-) rename lib/gitlab_git/{count.rb => util.rb} (82%) rename spec/{count_spec.rb => util_spec.rb} (64%) diff --git a/lib/gitlab_git.rb b/lib/gitlab_git.rb index 79759d1..39a9d4b 100644 --- a/lib/gitlab_git.rb +++ b/lib/gitlab_git.rb @@ -16,7 +16,6 @@ require_relative "gitlab_git/blob" require_relative "gitlab_git/commit" require_relative "gitlab_git/commit_stats" require_relative "gitlab_git/compare" -require_relative "gitlab_git/count" require_relative "gitlab_git/diff" require_relative "gitlab_git/diff_collection" require_relative "gitlab_git/repository" @@ -25,3 +24,4 @@ require_relative "gitlab_git/blob_snippet" require_relative "gitlab_git/ref" require_relative "gitlab_git/branch" require_relative "gitlab_git/tag" +require_relative "gitlab_git/util" diff --git a/lib/gitlab_git/diff.rb b/lib/gitlab_git/diff.rb index 5fc47ab..781c75f 100644 --- a/lib/gitlab_git/diff.rb +++ b/lib/gitlab_git/diff.rb @@ -189,7 +189,7 @@ module Gitlab end def line_count - @line_count ||= Count.lines(@diff) + @line_count ||= Util.count_lines(@diff) end private diff --git a/lib/gitlab_git/count.rb b/lib/gitlab_git/util.rb similarity index 82% rename from lib/gitlab_git/count.rb rename to lib/gitlab_git/util.rb index c10debe..03996b8 100644 --- a/lib/gitlab_git/count.rb +++ b/lib/gitlab_git/util.rb @@ -1,9 +1,9 @@ module Gitlab module Git - module Count + module Util LINE_SEP = "\n" - def self.lines(string) + def self.count_lines(string) case string[-1] when nil 0 diff --git a/spec/count_spec.rb b/spec/util_spec.rb similarity index 64% rename from spec/count_spec.rb rename to spec/util_spec.rb index b272c5c..ea29c38 100644 --- a/spec/count_spec.rb +++ b/spec/util_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' -describe Gitlab::Git::Count do - describe :lines do +describe Gitlab::Git::Util do + describe :count_lines do [ ["", 0], ["foo", 1], @@ -9,7 +9,7 @@ describe Gitlab::Git::Count do ["foo\n\n", 2], ].each do |string, line_count| it "counts #{line_count} lines in #{string.inspect}" do - Gitlab::Git::Count.lines(string).should eq(line_count) + Gitlab::Git::Util.count_lines(string).should eq(line_count) end end end -- GitLab From 5e4d82c36b92bcc98847d9af2a10e260a27a2ffb Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 2 Mar 2016 12:42:43 +0100 Subject: [PATCH 29/33] Changes suggested by Douwe --- lib/gitlab_git/diff_collection.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 67ccbb4..4a0a31f 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -19,7 +19,7 @@ module Gitlab def each @iterator.each_with_index do |raw, i| # First yield cached Diff instances from @array - if !@array[i].nil? + if @array[i] yield @array[i] next end @@ -81,8 +81,8 @@ module Gitlab end def map! - each_with_index do |elt, i| - @array[i] = yield(elt) + each_with_index do |element, i| + @array[i] = yield(element) end end -- GitLab From 38fec556d89e1bab860a89e9313ef1821c80fd3c Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 2 Mar 2016 12:47:29 +0100 Subject: [PATCH 30/33] Remove superfluous to_a --- spec/compare_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/compare_spec.rb b/spec/compare_spec.rb index 60f5416..62aa187 100644 --- a/spec/compare_spec.rb +++ b/spec/compare_spec.rb @@ -28,6 +28,6 @@ describe Gitlab::Git::Compare do let(:compare) { Gitlab::Git::Compare.new(repository, 'no-such-branch', '1234567890') } it { compare.commits.should be_empty } - it { compare.diffs.to_a.should be_empty } + it { compare.diffs.should be_empty } end end -- GitLab From f0d9419d729c20e1ec367df807a0951b80a773b1 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 2 Mar 2016 12:49:03 +0100 Subject: [PATCH 31/33] No need to prefer #count --- spec/diff_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/diff_spec.rb b/spec/diff_spec.rb index a0690c4..c15b539 100644 --- a/spec/diff_spec.rb +++ b/spec/diff_spec.rb @@ -53,7 +53,7 @@ EOT subject { diffs } it { should be_kind_of Gitlab::Git::DiffCollection } - its(:count) { should eq(1) } + its(:size) { should eq(1) } context :diff do subject { diffs.first } -- GitLab From 475beb8200ffa64707282eaa151c9b7b4c3d3be9 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 2 Mar 2016 17:10:51 +0100 Subject: [PATCH 32/33] Rename DiffCollection#map! to #decorate! --- lib/gitlab_git/diff_collection.rb | 2 +- spec/compare_spec.rb | 2 +- spec/diff_collection_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 4a0a31f..0b35e79 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -80,7 +80,7 @@ module Gitlab end end - def map! + def decorate! each_with_index do |element, i| @array[i] = yield(element) end diff --git a/spec/compare_spec.rb b/spec/compare_spec.rb index 62aa187..88c50d4 100644 --- a/spec/compare_spec.rb +++ b/spec/compare_spec.rb @@ -16,7 +16,7 @@ describe Gitlab::Git::Compare do describe :diffs do subject do - compare.diffs.map!(&:new_path) + compare.diffs.map(&:new_path) end it { should have(10).elements } diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb index e8d7dbf..f06748d 100644 --- a/spec/diff_collection_spec.rb +++ b/spec/diff_collection_spec.rb @@ -18,12 +18,12 @@ describe Gitlab::Git::DiffCollection do its(:to_a) { should be_kind_of ::Array } - describe :map! do + describe :decorate! do let(:file_count) { 3} it 'modifies the array in place' do count = 0 - subject.map! { |d| !d.nil? && count += 1 } + subject.decorate! { |d| !d.nil? && count += 1 } subject.to_a.should eq([1, 2, 3]) end end -- GitLab From 7c7e0cc783fd33481ee16b3a4bf8436087008ae6 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 2 Mar 2016 17:40:48 +0100 Subject: [PATCH 33/33] Remove too_many_foo methods --- lib/gitlab_git/diff_collection.rb | 10 ---------- spec/diff_collection_spec.rb | 18 ------------------ 2 files changed, 28 deletions(-) diff --git a/lib/gitlab_git/diff_collection.rb b/lib/gitlab_git/diff_collection.rb index 0b35e79..1ebef8d 100644 --- a/lib/gitlab_git/diff_collection.rb +++ b/lib/gitlab_git/diff_collection.rb @@ -56,16 +56,6 @@ module Gitlab !!@overflow end - def too_many_files? - populate! - !@all_diffs && @overflow && size >= @max_files - end - - def too_many_lines? - populate! - !@all_diffs && @overflow && @line_count >= @max_lines - end - def size @size ||= count # forces a loop through @iterator end diff --git a/spec/diff_collection_spec.rb b/spec/diff_collection_spec.rb index f06748d..20ffa4d 100644 --- a/spec/diff_collection_spec.rb +++ b/spec/diff_collection_spec.rb @@ -37,8 +37,6 @@ describe Gitlab::Git::DiffCollection do its(:overflow?) { should be_false } its(:empty?) { should be_false } - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } it { subject.size.should eq(3) } @@ -47,8 +45,6 @@ describe Gitlab::Git::DiffCollection do its(:overflow?) { should be_false } its(:empty?) { should be_false } - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } it { subject.size.should eq(3) } end @@ -59,8 +55,6 @@ describe Gitlab::Git::DiffCollection do its(:overflow?) { should be_true } its(:empty?) { should be_false } - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_true } its(:real_size) { should eq('0+') } it { subject.size.should eq(0) } @@ -69,8 +63,6 @@ describe Gitlab::Git::DiffCollection do its(:overflow?) { should be_false } its(:empty?) { should be_false } - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_false } its(:real_size) { should eq('3') } it { subject.size.should eq(3) } end @@ -85,8 +77,6 @@ describe Gitlab::Git::DiffCollection do its(:overflow?) { should be_true } its(:empty?) { should be_false } - its(:too_many_files?) { should be_true } - its(:too_many_lines?) { should be_false } its(:real_size) { should eq('10+') } it { subject.size.should eq(10) } @@ -95,8 +85,6 @@ describe Gitlab::Git::DiffCollection do its(:overflow?) { should be_false } its(:empty?) { should be_false } - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_false } its(:real_size) { should eq('11') } it { subject.size.should eq(11) } end @@ -107,8 +95,6 @@ describe Gitlab::Git::DiffCollection do its(:overflow?) { should be_true } its(:empty?) { should be_false } - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_true } its(:real_size) { should eq('3+') } it { subject.size.should eq(3) } @@ -117,8 +103,6 @@ describe Gitlab::Git::DiffCollection do its(:overflow?) { should be_false } its(:empty?) { should be_false } - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_false } its(:real_size) { should eq('11') } it { subject.size.should eq(11) } end @@ -133,8 +117,6 @@ describe Gitlab::Git::DiffCollection do its(:overflow?) { should be_false } its(:empty?) { should be_false } - its(:too_many_files?) { should be_false } - its(:too_many_lines?) { should be_false } its(:real_size) { should eq('10') } it { subject.size.should eq(10) } end -- GitLab