Merge branch 'limit-never-zero' into 'master'

Don't allow Repository#log with limit zero

See merge request gitlab-org/gitlab-ce!16674

lib/gitlab/git/repository.rb

@@ -468,9 +468,13 @@ module Gitlab
         }
 
         options = default_options.merge(options)
-        options[:limit] ||= 0
         options[:offset] ||= 0
 
+        limit = options[:limit]
+        if limit == 0 || !limit.is_a?(Integer)
+          raise ArgumentError.new("invalid Repository#log limit: #{limit.inspect}")
+        end
+
         gitaly_migrate(:find_commits) do |is_enabled|
           if is_enabled
             gitaly_commit_client.find_commits(options)

spec/features/commits_spec.rb

@@ -194,7 +194,7 @@ describe 'Commits' do
     end
 
     it 'includes the committed_date for each commit' do
-      commits = project.repository.commits(branch_name)
+      commits = project.repository.commits(branch_name, limit: 40)
 
       commits.each do |commit|
         expect(page).to have_content("authored #{commit.authored_date.strftime("%b %d, %Y")}")

spec/lib/gitlab/git/repository_spec.rb

@@ -981,6 +981,16 @@ describe Gitlab::Git::Repository, seed_helper: true do
         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) }
+      end
+    end
   end
 
   describe "#rugged_commits_between" do

spec/models/repository_spec.rb

@@ -222,20 +222,20 @@ describe Repository do
     it 'sets follow when path is a single path' do
       expect(Gitlab::Git::Commit).to receive(:where).with(a_hash_including(follow: true)).and_call_original.twice
 
-      repository.commits('master', path: 'README.md')
-      repository.commits('master', path: ['README.md'])
+      repository.commits('master', limit: 1, path: 'README.md')
+      repository.commits('master', limit: 1, path: ['README.md'])
     end
 
     it 'does not set follow when path is multiple paths' do
       expect(Gitlab::Git::Commit).to receive(:where).with(a_hash_including(follow: false)).and_call_original
 
-      repository.commits('master', path: ['README.md', 'CHANGELOG'])
+      repository.commits('master', limit: 1, path: ['README.md', 'CHANGELOG'])
     end
 
     it 'does not set follow when there are no paths' do
       expect(Gitlab::Git::Commit).to receive(:where).with(a_hash_including(follow: false)).and_call_original
 
-      repository.commits('master')
+      repository.commits('master', limit: 1)
     end
   end
 
@@ -455,7 +455,7 @@ describe Repository do
       expect do
         repository.create_dir(user, 'newdir',
           message: 'Create newdir', branch_name: 'master')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       newdir = repository.tree('master', 'newdir')
       expect(newdir.path).to eq('newdir')
@@ -469,7 +469,7 @@ describe Repository do
           repository.create_dir(user, 'newdir',
             message: 'Create newdir', branch_name: 'patch',
             start_branch_name: 'master', start_project: forked_project)
-        end.to change { repository.commits('master').count }.by(0)
+        end.to change { repository.count_commits(ref: 'master') }.by(0)
 
         expect(repository.branch_exists?('patch')).to be_truthy
         expect(forked_project.repository.branch_exists?('patch')).to be_falsy
@@ -486,7 +486,7 @@ describe Repository do
             message: 'Add newdir',
             branch_name: 'master',
             author_email: author_email, author_name: author_name)
-        end.to change { repository.commits('master').count }.by(1)
+        end.to change { repository.count_commits(ref: 'master') }.by(1)
 
         last_commit = repository.commit
 
@@ -502,7 +502,7 @@ describe Repository do
         repository.create_file(user, 'NEWCHANGELOG', 'Changelog!',
                                message: 'Create changelog',
                                branch_name: 'master')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       blob = repository.blob_at('master', 'NEWCHANGELOG')
 
@@ -514,7 +514,7 @@ describe Repository do
         repository.create_file(user, 'new_dir/new_file.txt', 'File!',
                                message: 'Create new_file with new_dir',
                                branch_name: 'master')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       expect(repository.tree('master', 'new_dir').path).to eq('new_dir')
       expect(repository.blob_at('master', 'new_dir/new_file.txt').data).to eq('File!')
@@ -538,7 +538,7 @@ describe Repository do
                                  branch_name: 'master',
                                  author_email: author_email,
                                  author_name: author_name)
-        end.to change { repository.commits('master').count }.by(1)
+        end.to change { repository.count_commits(ref: 'master') }.by(1)
 
         last_commit = repository.commit
 
@@ -554,7 +554,7 @@ describe Repository do
         repository.update_file(user, 'CHANGELOG', 'Changelog!',
                                message: 'Update changelog',
                                branch_name: 'master')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       blob = repository.blob_at('master', 'CHANGELOG')
 
@@ -567,7 +567,7 @@ describe Repository do
                                      branch_name: 'master',
                                      previous_path: 'LICENSE',
                                      message: 'Changes filename')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       files = repository.ls_files('master')
 
@@ -584,7 +584,7 @@ describe Repository do
                                  message: 'Update README',
                                  author_email: author_email,
                                  author_name: author_name)
-        end.to change { repository.commits('master').count }.by(1)
+        end.to change { repository.count_commits(ref: 'master') }.by(1)
 
         last_commit = repository.commit
 
@@ -599,7 +599,7 @@ describe Repository do
       expect do
         repository.delete_file(user, 'README',
           message: 'Remove README', branch_name: 'master')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       expect(repository.blob_at('master', 'README')).to be_nil
     end
@@ -610,7 +610,7 @@ describe Repository do
           repository.delete_file(user, 'README',
             message: 'Remove README', branch_name: 'master',
             author_email: author_email, author_name: author_name)
-        end.to change { repository.commits('master').count }.by(1)
+        end.to change { repository.count_commits(ref: 'master') }.by(1)
 
         last_commit = repository.commit
 

spec/requests/api/commits_spec.rb

@@ -62,7 +62,7 @@ describe API::Commits do
 
       context "since optional parameter" do
         it "returns project commits since provided parameter" do
-          commits = project.repository.commits("master")
+          commits = project.repository.commits("master", limit: 2)
           after = commits.second.created_at
 
           get api("/projects/#{project_id}/repository/commits?since=#{after.utc.iso8601}", user)
@@ -73,7 +73,7 @@ describe API::Commits do
         end
 
         it 'include correct pagination headers' do
-          commits = project.repository.commits("master")
+          commits = project.repository.commits("master", limit: 2)
           after = commits.second.created_at
           commit_count = project.repository.count_commits(ref: 'master', after: after).to_s
 
@@ -87,12 +87,12 @@ describe API::Commits do
 
       context "until optional parameter" do
         it "returns project commits until provided parameter" do
-          commits = project.repository.commits("master")
+          commits = project.repository.commits("master", limit: 20)
           before = commits.second.created_at
 
           get api("/projects/#{project_id}/repository/commits?until=#{before.utc.iso8601}", user)
 
-          if commits.size >= 20
+          if commits.size == 20
             expect(json_response.size).to eq(20)
           else
             expect(json_response.size).to eq(commits.size - 1)
@@ -103,7 +103,7 @@ describe API::Commits do
         end
 
         it 'include correct pagination headers' do
-          commits = project.repository.commits("master")
+          commits = project.repository.commits("master", limit: 2)
           before = commits.second.created_at
           commit_count = project.repository.count_commits(ref: 'master', before: before).to_s
 
@@ -181,7 +181,7 @@ describe API::Commits do
           let(:page) { 3 }
 
           it 'returns the third 5 commits' do
-            commit = project.repository.commits('HEAD', offset: (page - 1) * per_page).first
+            commit = project.repository.commits('HEAD', limit: per_page, offset: (page - 1) * per_page).first
 
             expect(json_response.size).to eq(per_page)
             expect(json_response.first['id']).to eq(commit.id)

spec/requests/api/v3/commits_spec.rb

@@ -36,7 +36,7 @@ describe API::V3::Commits do
 
     context "since optional parameter" do
       it "returns project commits since provided parameter" do
-        commits = project.repository.commits("master")
+        commits = project.repository.commits("master", limit: 2)
         since = commits.second.created_at
 
         get v3_api("/projects/#{project.id}/repository/commits?since=#{since.utc.iso8601}", user)
@@ -49,12 +49,12 @@ describe API::V3::Commits do
 
     context "until optional parameter" do
       it "returns project commits until provided parameter" do
-        commits = project.repository.commits("master")
+        commits = project.repository.commits("master", limit: 20)
         before = commits.second.created_at
 
         get v3_api("/projects/#{project.id}/repository/commits?until=#{before.utc.iso8601}", user)
 
-        if commits.size >= 20
+        if commits.size == 20
           expect(json_response.size).to eq(20)
         else
           expect(json_response.size).to eq(commits.size - 1)