Improve diff collection for Compare

* Get rid of BROKEN_DIFF constant
* Diff.between raises exception in case of timeout now
* Add tests to Compare
* Cache diff result for Compare

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

CHANGELOG

@@ -1,3 +1,9 @@
+v 5.4.0
+  - Get rid of BROKEN_DIFF constant
+  - Diff.between raises exception in case of timeout now
+  - Add tests to Compare
+  - Cache diff result for Compare 
+
 v 5.3.0
   - Add Repository#submodules method
 

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    gitlab_git (5.2.0)
+    gitlab_git (5.3.0)
       activesupport (~> 4.0.0)
       charlock_holmes (~> 0.6.9)
       gitlab-grit (~> 2.6.1)

lib/gitlab_git/compare.rb

@@ -1,14 +1,15 @@
 module Gitlab
   module Git
     class Compare
-      attr_accessor :commits, :commit, :diffs, :same
+      attr_accessor :commits, :commit, :diffs, :same, :limit, :timeout
 
       def initialize(repository, from, to, limit = 100)
         @commits, @diffs = [], []
         @commit = nil
         @same = false
         @limit = limit
         @repository = repository
+        @timeout = false
 
         return unless from && to
 
@@ -27,10 +28,33 @@ def initialize(repository, from, to, limit = 100)
       end
 
       def diffs(paths = nil)
-        return [] if @commits.size > @limit
-        Gitlab::Git::Diff.between(@repository, @head.id, @base.id, *paths) rescue []
+        # Return empty array if amount of commits
+        # more than specified limit
+        return [] if commits_over_limit?
+
+        # Try to collect diff only if diffs is empty
+        # Otherwise return cached version
+        if @diffs.empty? && @timeout == false
+          begin
+            @diffs = Gitlab::Git::Diff.between(@repository, @head.id, @base.id, *paths)
+          rescue Gitlab::Git::Diff::TimeoutError => ex
+            @diffs = []
+            @timeout = true
+          end
+        end
+
+        @diffs
+      end
+
+      # 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 && commits.size < limit
+      end
+
+      def commits_over_limit?
+        commits.size > limit
       end
     end
   end
 end
-

lib/gitlab_git/diff.rb

@@ -4,7 +4,7 @@
 module Gitlab
   module Git
     class Diff
-      BROKEN_DIFF = "--broken-diff"
+      class TimeoutError < StandardError; end
 
       attr_accessor :raw_diff
 
@@ -25,7 +25,7 @@ def between(repo, head, base, *paths)
             Gitlab::Git::Diff.new(diff)
           end
         rescue Grit::Git::GitTimeout
-          [Gitlab::Git::Diff::BROKEN_DIFF]
+          raise TimeoutError.new("Diff.between exited with timeout")
         end
       end
 

spec/compare_spec.rb

@@ -0,0 +1,30 @@
+require "spec_helper"
+
+describe Gitlab::Git::Compare do
+  let(:repository) { Gitlab::Git::Repository.new(TEST_REPO_PATH) }
+  let(:compare) { Gitlab::Git::Compare.new(repository,
+                                           "3a4b4fb4cde7809f033822a171b9feae19d41fff",
+                                           "8470d70da67355c9c009e4401746b1d5410af2e3") }
+
+  describe :commits do
+    subject do
+      compare.commits.map(&:id)
+    end
+
+    it { should have(3).elements }
+    it { should include("f0f14c8eaba69ebddd766498a9d0b0e79becd633") }
+    it { should_not include("bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a") }
+  end
+
+  describe :diffs do
+    subject do
+      compare.diffs.map(&:new_path)
+    end
+
+    it { should have(19).elements }
+    it { should include('app/assets/javascripts/application.js') }
+    it { should_not include('Gemfile') }
+    it { compare.timeout.should be_false }
+    it { compare.empty_diff?.should be_false }
+  end
+end