Add support for :all option to {count,find}_commits

GITALY_SERVER_VERSION

-0.87.0
+0.88.0

Gemfile

@@ -411,7 +411,7 @@ group :ed25519 do
 end
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.87.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.88.0', require: 'gitaly'
 # Locked until https://github.com/google/protobuf/issues/4210 is closed
 gem 'google-protobuf', '= 3.5.1'

Gemfile.lock

@@ -285,7 +285,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly-proto (0.87.0)
+    gitaly-proto (0.88.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (5.3.3)
@@ -1057,7 +1057,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly-proto (~> 0.87.0)
+  gitaly-proto (~> 0.88.0)
   github-linguist (~> 5.3.3)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.6.2)

lib/gitlab/git/repository.rb

@@ -479,9 +479,8 @@ module Gitlab
           raise ArgumentError.new("invalid Repository#log limit: #{limit.inspect}")
         end
-        # TODO support options[:all] in Gitaly https://gitlab.com/gitlab-org/gitaly/issues/1049
         gitaly_migrate(:find_commits) do |is_enabled|
-          if is_enabled && !options[:all]
+          if is_enabled
             gitaly_commit_client.find_commits(options)
           else
             raw_log(options).map { |c| Commit.decorate(self, c) }
@@ -508,9 +507,8 @@ module Gitlab
       def count_commits(options)
         count_commits_options = process_count_commits_options(options)
-        # TODO add support for options[:all] in Gitaly https://gitlab.com/gitlab-org/gitaly/issues/1050
         gitaly_migrate(:count_commits) do |is_enabled|
-          if is_enabled && !options[:all]
+          if is_enabled
             count_commits_by_gitaly(count_commits_options)
           else
             count_commits_by_shelling_out(count_commits_options)

lib/gitlab/gitaly_client/commit_service.rb

@@ -134,7 +134,8 @@ module Gitlab
       def commit_count(ref, options = {})
         request = Gitaly::CountCommitsRequest.new(
           repository: @gitaly_repo,
-          revision: encode_binary(ref)
+          revision: encode_binary(ref),
+          all: !!options[:all]
         )
         request.after = Google::Protobuf::Timestamp.new(seconds: options[:after].to_i) if options[:after].present?
         request.before = Google::Protobuf::Timestamp.new(seconds: options[:before].to_i) if options[:before].present?
@@ -269,6 +270,7 @@ module Gitlab
           offset:       options[:offset],
           follow:       options[:follow],
           skip_merges:  options[:skip_merges],
+          all:          !!options[:all],
           disable_walk: true # This option is deprecated. The 'walk' implementation is being removed.
         )
         request.after    = GitalyClient.timestamp(options[:after]) if options[:after]

spec/lib/gitlab/git/repository_spec.rb

@@ -751,255 +751,263 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
   describe "#log" do
-    let(:commit_with_old_name) do
-      Gitlab::Git::Commit.decorate(repository, @commit_with_old_name_id)
-    end
-    let(:commit_with_new_name) do
-      Gitlab::Git::Commit.decorate(repository, @commit_with_new_name_id)
-    end
-    let(:rename_commit) do
-      Gitlab::Git::Commit.decorate(repository, @rename_commit_id)
-    end
-
-    before(:context) do
-      # Add new commits so that there's a renamed file in the commit history
-      repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
-      @commit_with_old_name_id = new_commit_edit_old_file(repo)
-      @rename_commit_id = new_commit_move_file(repo)
-      @commit_with_new_name_id = new_commit_edit_new_file(repo)
-    end
-
-    after(:context) do
-      # Erase our commits so other tests get the original repo
-      repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
-      repo.references.update("refs/heads/master", SeedRepo::LastCommit::ID)
-    end
-
-    context "where 'follow' == true" do
-      let(:options) { { ref: "master", follow: true } }
-      context "and 'path' is a directory" do
-        it "does not follow renames" do
-          log_commits = repository.log(options.merge(path: "encoding"))
-          aggregate_failures do
-            expect(log_commits).to include(commit_with_new_name)
-            expect(log_commits).to include(rename_commit)
-            expect(log_commits).not_to include(commit_with_old_name)
-          end
-        end
+    shared_examples 'repository log' do
+      let(:commit_with_old_name) do
+        Gitlab::Git::Commit.decorate(repository, @commit_with_old_name_id)
+      end
+      let(:commit_with_new_name) do
+        Gitlab::Git::Commit.decorate(repository, @commit_with_new_name_id)
+      end
+      let(:rename_commit) do
+        Gitlab::Git::Commit.decorate(repository, @rename_commit_id)
+      end
+      before(:context) do
+        # Add new commits so that there's a renamed file in the commit history
+        repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
+        @commit_with_old_name_id = new_commit_edit_old_file(repo)
+        @rename_commit_id = new_commit_move_file(repo)
+        @commit_with_new_name_id = new_commit_edit_new_file(repo)
+      end
+      after(:context) do
+        # Erase our commits so other tests get the original repo
+        repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
+        repo.references.update("refs/heads/master", SeedRepo::LastCommit::ID)
       end
-      context "and 'path' is a file that matches the new filename" do
-        context 'without offset' do
-          it "follows renames" do
-            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG"))
+      context "where 'follow' == true" do
+        let(:options) { { ref: "master", follow: true } }
+
+        context "and 'path' is a directory" do
+          it "does not follow renames" do
+            log_commits = repository.log(options.merge(path: "encoding"))
             aggregate_failures do
               expect(log_commits).to include(commit_with_new_name)
               expect(log_commits).to include(rename_commit)
-              expect(log_commits).to include(commit_with_old_name)
+              expect(log_commits).not_to include(commit_with_old_name)
             end
           end
         end
-        context 'with offset=1' do
-          it "follows renames and skip the latest commit" do
-            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1))
-            aggregate_failures do
-              expect(log_commits).not_to include(commit_with_new_name)
-              expect(log_commits).to include(rename_commit)
-              expect(log_commits).to include(commit_with_old_name)
+        context "and 'path' is a file that matches the new filename" do
+          context 'without offset' do
+            it "follows renames" do
+              log_commits = repository.log(options.merge(path: "encoding/CHANGELOG"))
+              aggregate_failures do
+                expect(log_commits).to include(commit_with_new_name)
+                expect(log_commits).to include(rename_commit)
+                expect(log_commits).to include(commit_with_old_name)
+              end
             end
           end
-        end
-        context 'with offset=1', 'and limit=1' do
-          it "follows renames, skip the latest commit and return only one commit" do
-            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1, limit: 1))
-            expect(log_commits).to contain_exactly(rename_commit)
+          context 'with offset=1' do
+            it "follows renames and skip the latest commit" do
+              log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1))
+              aggregate_failures do
+                expect(log_commits).not_to include(commit_with_new_name)
+                expect(log_commits).to include(rename_commit)
+                expect(log_commits).to include(commit_with_old_name)
+              end
+            end
           end
-        end
-        context 'with offset=1', 'and limit=2' do
-          it "follows renames, skip the latest commit and return only two commits" do
-            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1, limit: 2))
-            aggregate_failures do
-              expect(log_commits).to contain_exactly(rename_commit, commit_with_old_name)
+          context 'with offset=1', 'and limit=1' do
+            it "follows renames, skip the latest commit and return only one commit" do
+              log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1, limit: 1))
+              expect(log_commits).to contain_exactly(rename_commit)
             end
           end
-        end
-        context 'with offset=2' do
-          it "follows renames and skip the latest commit" do
-            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2))
-            aggregate_failures do
-              expect(log_commits).not_to include(commit_with_new_name)
-              expect(log_commits).not_to include(rename_commit)
-              expect(log_commits).to include(commit_with_old_name)
+          context 'with offset=1', 'and limit=2' do
+            it "follows renames, skip the latest commit and return only two commits" do
+              log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1, limit: 2))
+              aggregate_failures do
+                expect(log_commits).to contain_exactly(rename_commit, commit_with_old_name)
+              end
             end
           end
-        end
-        context 'with offset=2', 'and limit=1' do
-          it "follows renames, skip the two latest commit and return only one commit" do
-            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2, limit: 1))
-            expect(log_commits).to contain_exactly(commit_with_old_name)
+          context 'with offset=2' do
+            it "follows renames and skip the latest commit" do
+              log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2))
+              aggregate_failures do
+                expect(log_commits).not_to include(commit_with_new_name)
+                expect(log_commits).not_to include(rename_commit)
+                expect(log_commits).to include(commit_with_old_name)
+              end
+            end
+          end
+
+          context 'with offset=2', 'and limit=1' do
+            it "follows renames, skip the two latest commit and return only one commit" do
+              log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2, limit: 1))
+
+              expect(log_commits).to contain_exactly(commit_with_old_name)
+            end
+          end
+
+          context 'with offset=2', 'and limit=2' do
+            it "follows renames, skip the two latest commit and return only one commit" do
+              log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2, limit: 2))
+
+              aggregate_failures do
+                expect(log_commits).not_to include(commit_with_new_name)
+                expect(log_commits).not_to include(rename_commit)
+                expect(log_commits).to include(commit_with_old_name)
+              end
+            end
           end
         end
-        context 'with offset=2', 'and limit=2' do
-          it "follows renames, skip the two latest commit and return only one commit" do
-            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2, limit: 2))
+        context "and 'path' is a file that matches the old filename" do
+          it "does not follow renames" do
+            log_commits = repository.log(options.merge(path: "CHANGELOG"))
             aggregate_failures do
               expect(log_commits).not_to include(commit_with_new_name)
-              expect(log_commits).not_to include(rename_commit)
+              expect(log_commits).to include(rename_commit)
               expect(log_commits).to include(commit_with_old_name)
             end
           end
         end
-      end
-      context "and 'path' is a file that matches the old filename" do
-        it "does not follow renames" do
-          log_commits = repository.log(options.merge(path: "CHANGELOG"))
-          aggregate_failures do
-            expect(log_commits).not_to include(commit_with_new_name)
-            expect(log_commits).to include(rename_commit)
-            expect(log_commits).to include(commit_with_old_name)
+        context "unknown ref" do
+          it "returns an empty array" do
+            log_commits = repository.log(options.merge(ref: 'unknown'))
+            expect(log_commits).to eq([])
           end
         end
       end
-      context "unknown ref" do
-        it "returns an empty array" do
-          log_commits = repository.log(options.merge(ref: 'unknown'))
-
-          expect(log_commits).to eq([])
-        end
-      end
-    end
-    context "where 'follow' == false" do
-      options = { follow: false }
-      context "and 'path' is a directory" do
-        let(:log_commits) do
-          repository.log(options.merge(path: "encoding"))
+      context "where 'follow' == false" do
+        options = { follow: false }
+        context "and 'path' is a directory" do
+          let(:log_commits) do
+            repository.log(options.merge(path: "encoding"))
+          end
+          it "does not follow renames" do
+            expect(log_commits).to include(commit_with_new_name)
+            expect(log_commits).to include(rename_commit)
+            expect(log_commits).not_to include(commit_with_old_name)
+          end
         end
-        it "does not follow renames" do
-          expect(log_commits).to include(commit_with_new_name)
-          expect(log_commits).to include(rename_commit)
-          expect(log_commits).not_to include(commit_with_old_name)
-        end
-      end
-      context "and 'path' is a file that matches the new filename" do
-        let(:log_commits) do
-          repository.log(options.merge(path: "encoding/CHANGELOG"))
+        context "and 'path' is a file that matches the new filename" do
+          let(:log_commits) do
+            repository.log(options.merge(path: "encoding/CHANGELOG"))
+          end
+          it "does not follow renames" do
+            expect(log_commits).to include(commit_with_new_name)
+            expect(log_commits).to include(rename_commit)
+            expect(log_commits).not_to include(commit_with_old_name)
+          end
         end
-        it "does not follow renames" do
-          expect(log_commits).to include(commit_with_new_name)
-          expect(log_commits).to include(rename_commit)
-          expect(log_commits).not_to include(commit_with_old_name)
-        end
-      end
-      context "and 'path' is a file that matches the old filename" do
-        let(:log_commits) do
-          repository.log(options.merge(path: "CHANGELOG"))
+        context "and 'path' is a file that matches the old filename" do
+          let(:log_commits) do
+            repository.log(options.merge(path: "CHANGELOG"))
+          end
+          it "does not follow renames" do
+            expect(log_commits).to include(commit_with_old_name)
+            expect(log_commits).to include(rename_commit)
+            expect(log_commits).not_to include(commit_with_new_name)
+          end
         end
-        it "does not follow renames" do
-          expect(log_commits).to include(commit_with_old_name)
-          expect(log_commits).to include(rename_commit)
-          expect(log_commits).not_to include(commit_with_new_name)
+        context "and 'path' includes a directory that used to be a file" do
+          let(:log_commits) do
+            repository.log(options.merge(ref: "refs/heads/fix-blob-path", path: "files/testdir/file.txt"))
+          end
+
+          it "returns a list of commits" do
+            expect(log_commits.size).to eq(1)
+          end
         end
       end
-      context "and 'path' includes a directory that used to be a file" do
-        let(:log_commits) do
-          repository.log(options.merge(ref: "refs/heads/fix-blob-path", path: "files/testdir/file.txt"))
-        end
-        it "returns a list of commits" do
-          expect(log_commits.size).to eq(1)
+      context "where provides 'after' timestamp" do
+        options = { after: Time.iso8601('2014-03-03T20:15:01+00:00') }
+        it "should returns commits on or after that timestamp" do
+          commits = repository.log(options)
+
+          expect(commits.size).to be > 0
+          expect(commits).to satisfy do |commits|
+            commits.all? { |commit| commit.committed_date >= options[:after] }
+          end
         end
       end
-    end
-    context "where provides 'after' timestamp" do
-      options = { after: Time.iso8601('2014-03-03T20:15:01+00:00') }
-      it "should returns commits on or after that timestamp" do
-        commits = repository.log(options)
-        expect(commits.size).to be > 0
-        expect(commits).to satisfy do |commits|
-          commits.all? { |commit| commit.committed_date >= options[:after] }
+      context "where provides 'before' timestamp" do
+        options = { before: Time.iso8601('2014-03-03T20:15:01+00:00') }
+        it "should returns commits on or before that timestamp" do
+          commits = repository.log(options)
+          expect(commits.size).to be > 0
+          expect(commits).to satisfy do |commits|
+            commits.all? { |commit| commit.committed_date <= options[:before] }
+          end
         end
       end
-    end
-    context "where provides 'before' timestamp" do
-      options = { before: Time.iso8601('2014-03-03T20:15:01+00:00') }
-      it "should returns commits on or before that timestamp" do
-        commits = repository.log(options)
-
-        expect(commits.size).to be > 0
-        expect(commits).to satisfy do |commits|
-          commits.all? { |commit| commit.committed_date <= options[:before] }
+      context 'when multiple paths are provided' do
+        let(:options) { { ref: 'master', path: ['PROCESS.md', 'README.md'] } }
+        def commit_files(commit)
+          commit.rugged_diff_from_parent.deltas.flat_map do |delta|
+            [delta.old_file[:path], delta.new_file[:path]].uniq.compact
+          end
         end
-      end
-    end
-    context 'when multiple paths are provided' do
-      let(:options) { { ref: 'master', path: ['PROCESS.md', 'README.md'] } }
-      def commit_files(commit)
-        commit.rugged_diff_from_parent.deltas.flat_map do |delta|
-          [delta.old_file[:path], delta.new_file[:path]].uniq.compact
+        it 'only returns commits matching at least one path' do
+          commits = repository.log(options)
+          expect(commits.size).to be > 0
+          expect(commits).to satisfy do |commits|
+            commits.none? { |commit| (commit_files(commit) & options[:path]).empty? }
+          end
         end
       end
-      it 'only returns commits matching at least one path' do
-        commits = repository.log(options)
-        expect(commits.size).to be > 0
-        expect(commits).to satisfy do |commits|
-          commits.none? { |commit| (commit_files(commit) & options[:path]).empty? }
+      context 'limit validation' do
+        where(:limit) do
+          [0, nil, '', 'foo']
+        end
+        with_them do
+          it { expect { repository.log(limit: limit) }.to raise_error(ArgumentError) }
         end
       end
-    end
-    context 'limit validation' do
-      where(:limit) do
-        [0, nil, '', 'foo']
-      end
-      with_them do
-        it { expect { repository.log(limit: limit) }.to raise_error(ArgumentError) }
+      context 'with all' do
+        it 'returns a list of commits' do
+          commits = repository.log({ all: true, limit: 50 })
+          expect(commits.size).to eq(37)
+        end
       end
     end
-    context 'with all' do
-      let(:options) { { all: true, limit: 50 } }
-
-      it 'returns a list of commits' do
-        commits = repository.log(options)
-        expect(commits.size).to eq(37)
-      end
+    context 'when Gitaly find_commits feature is enabled' do
+      it_behaves_like 'repository log'
+    end
+    context 'when Gitaly find_commits feature is disabled', :disable_gitaly do
+      it_behaves_like 'repository log'
     end
   end
@@ -1136,14 +1144,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
           expect(repository.count_commits(options)).to eq(10)
         end
       end
-    end
-
-    context 'when Gitaly count_commits feature is enabled' do
-      it_behaves_like 'extended commit counting'
-    end
-
-    context 'when Gitaly count_commits feature is disabled', :skip_gitaly_mock do
-      it_behaves_like 'extended commit counting'
       context "with all" do
         it "returns the number of commits in the whole repository" do
@@ -1155,10 +1155,18 @@ describe Gitlab::Git::Repository, seed_helper: true do
       context 'without all or ref being specified' do
         it "raises an ArgumentError" do
-          expect { repository.count_commits({}) }.to raise_error(ArgumentError, "Please specify a valid ref or set the 'all' attribute to true")
+          expect { repository.count_commits({}) }.to raise_error(ArgumentError)
         end
       end
     end
+
+    context 'when Gitaly count_commits feature is enabled' do
+      it_behaves_like 'extended commit counting'
+    end
+
+    context 'when Gitaly count_commits feature is disabled', :disable_gitaly do
+      it_behaves_like 'extended commit counting'
+    end
   end
   describe '#autocrlf' do