Returns correct header data for commits endpoint

10 files changed, 119 insertions, 47 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index c0c5816d151..6ab04440ca8 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -499,10 +499,12 @@ class Repository
   cache_method :commit_count, fallback: 0
 
   def commit_count_for_ref(ref)
-    return 0 if empty?
+    return 0 unless exists?
 
-    cache.fetch(:"commit_count_#{ref}") do
-      raw_repository.commit_count(ref)
+    begin
+      cache.fetch(:"commit_count_#{ref}") { raw_repository.commit_count(ref) }
+    rescue Rugged::ReferenceError
+      0
     end
   end
 
diff --git a/changelogs/unreleased/13605-add-pagination-headers-for-repository-commits-api-endpoint.yml b/changelogs/unreleased/13605-add-pagination-headers-for-repository-commits-api-endpoint.yml
deleted file mode 100644
index 723510c219c..00000000000
--- a/changelogs/unreleased/13605-add-pagination-headers-for-repository-commits-api-endpoint.yml
+++ /dev/null
@@ -1,4 +0,0 @@
----
-title: Fix pagination headers for repository commits api endpoint
-merge_request:
-author: George Andrinopoulos
diff --git a/changelogs/unreleased/1381-present-commits-pagination-headers-correctly.yml b/changelogs/unreleased/1381-present-commits-pagination-headers-correctly.yml
new file mode 100644
index 00000000000..1b7e294bd67
--- /dev/null
+++ b/changelogs/unreleased/1381-present-commits-pagination-headers-correctly.yml
@@ -0,0 +1,4 @@
+---
+title: "GET 'projects/:id/repository/commits' endpoint improvements"
+merge_request: 9679
+author: George Andrinopoulos, Jordan Ryan Reuter
diff --git a/changelogs/unreleased/1381-use-commit-count-for-pagination.yml b/changelogs/unreleased/1381-use-commit-count-for-pagination.yml
deleted file mode 100644
index ae84f64ed03..00000000000
--- a/changelogs/unreleased/1381-use-commit-count-for-pagination.yml
+++ /dev/null
@@ -1,4 +0,0 @@
----
-title: Add pagination to project commits API endpoint
-merge_request: 5298
-author: Jordan Ryan Reuter
diff --git a/doc/api/v3_to_v4.md b/doc/api/v3_to_v4.md
index f42a5e9158b..1e320bef84a 100644
--- a/doc/api/v3_to_v4.md
+++ b/doc/api/v3_to_v4.md
@@ -71,3 +71,6 @@ changes are in V4:
 - Simplify project payload exposed on Environment endpoints [!9675](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9675)
 - API uses merge request `IID`s (internal ID, as in the web UI) rather than `ID`s. This affects the merge requests, award emoji, todos, and time tracking APIs. [!9530](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9530)
 - API uses issue `IID`s (internal ID, as in the web UI) rather than `ID`s. This affects the issues, award emoji, todos, and time tracking APIs. [!9530](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9530)
+- Change initial page from `0` to `1` on `GET projects/:id/repository/commits` (like on the rest of the API) [!9679] (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9679)
+- Return correct `Link` header data for `GET projects/:id/repository/commits` [!9679] (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9679)
+
diff --git a/lib/api/commits.rb b/lib/api/commits.rb
index 330ad4e3d3b..42401abfe0f 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -18,22 +18,32 @@ module API
         optional :ref_name, type: String, desc: 'The name of a repository branch or tag, if not given the default branch is used'
         optional :since,    type: DateTime, desc: 'Only commits after or on this date will be returned'
         optional :until,    type: DateTime, desc: 'Only commits before or on this date will be returned'
-        optional :page,     type: Integer, default: 0, desc: 'The page for pagination'
-        optional :per_page, type: Integer, default: 20, desc: 'The number of results per page'
         optional :path,     type: String, desc: 'The file path'
+        use :pagination
       end
       get ":id/repository/commits" do
-        ref = params[:ref_name] || user_project.try(:default_branch) || 'master'
+        path   = params[:path]
+        before = params[:until]
+        after  = params[:since]
+        ref    = params[:ref_name] || user_project.try(:default_branch) || 'master'
         offset = (params[:page] - 1) * params[:per_page]
 
         commits = user_project.repository.commits(ref,
-                                                  path: params[:path],
+                                                  path: path,
                                                   limit: params[:per_page],
                                                   offset: offset,
-                                                  after: params[:since],
-                                                  before: params[:until])
+                                                  before: before,
+                                                  after: after)
+
+        commit_count =
+          if path || before || after
+            user_project.repository.count_commits(ref: ref, path: path, before: before, after: after)
+          else
+            # Cacheable commit count.
+            user_project.repository.commit_count_for_ref(ref)
+          end
 
-        paginated_commits = Kaminari.paginate_array(commits, total_count: commits.size)
+        paginated_commits = Kaminari.paginate_array(commits, total_count: commit_count)
 
         present paginate(paginated_commits), with: Entities::RepoCommit
       end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 0f092d4c4ab..228ef7bb7a9 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -292,6 +292,7 @@ module Gitlab
         }
 
         options = default_options.merge(options)
+        options[:limit] ||= 0
         options[:offset] ||= 0
         actual_ref = options[:ref] || root_ref
         begin
@@ -354,12 +355,14 @@ module Gitlab
       end
 
       def count_commits(options)
-        cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{path} rev-list)
-        cmd += %W(--after=#{options[:after].iso8601}) if options[:after]
-        cmd += %W(--before=#{options[:before].iso8601}) if options[:before]
-        cmd += %W(--count #{options[:ref]})
-        cmd += %W(-- #{options[:path]}) if options[:path].present?
-        raw_output = IO.popen(cmd) {|io| io.read }
+        cmd = %W[#{Gitlab.config.git.bin_path} --git-dir=#{path} rev-list]
+        cmd << "--after=#{options[:after].iso8601}" if options[:after]
+        cmd << "--before=#{options[:before].iso8601}" if options[:before]
+        cmd += %W[--count #{options[:ref]}]
+        cmd += %W[-- #{options[:path]}] if options[:path].present?
+
+        raw_output = IO.popen(cmd) { |io| io.read }
+
         raw_output.to_i
       end
 
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 2c0e005e942..bc139d5ef28 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -826,29 +826,26 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
   describe '#count_commits' do
     context 'with after timestamp' do
-      options = { ref: 'master', limit: nil, after: Time.iso8601('2013-03-03T20:15:01+00:00') }
       it 'returns the number of commits after timestamp' do
-        commits = repository.log(options)
+        options = { ref: 'master', limit: nil, after: Time.iso8601('2013-03-03T20:15:01+00:00') }
 
-        expect(repository.count_commits(options)).to eq(commits.size)
+        expect(repository.count_commits(options)).to eq(25)
       end
     end
 
     context 'with before timestamp' do
-      options = { ref: 'feature', limit: nil, before: Time.iso8601('2015-03-03T20:15:01+00:00') }
       it 'returns the number of commits after timestamp' do
-        commits = repository.log(options)
+        options = { ref: 'feature', limit: nil, before: Time.iso8601('2015-03-03T20:15:01+00:00') }
 
-        expect(repository.count_commits(options)).to eq(commits.size)
+        expect(repository.count_commits(options)).to eq(9)
       end
     end
 
     context 'with path' do
-      options = { ref: 'master', limit: nil, path: "encoding" }
       it 'returns the number of commits with path ' do
-        commits = repository.log(options)
+        options = { ref: 'master', limit: nil, path: "encoding" }
 
-        expect(repository.count_commits(options)).to eq(commits.size)
+        expect(repository.count_commits(options)).to eq(2)
       end
     end
   end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 45acdc52d08..274e4f00a0a 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1743,10 +1743,18 @@ describe Repository, models: true do
   end
 
   describe '#commit_count_for_ref' do
+    let(:project) { create :empty_project }
+
     context 'with a non-existing repository' do
       it 'returns 0' do
-        expect(repository).to receive(:raw_repository).and_return(nil)
-        expect(repository.commit_count).to eq(0)
+        expect(project.repository.commit_count_for_ref('master')).to eq(0)
+      end
+    end
+
+    context 'with empty repository' do
+      it 'returns 0' do
+        project.create_repository
+        expect(project.repository.commit_count_for_ref('master')).to eq(0)
       end
     end
 
diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb
index 1d298988f17..585449e62b6 100644
--- a/spec/requests/api/commits_spec.rb
+++ b/spec/requests/api/commits_spec.rb
@@ -19,6 +19,7 @@ describe API::Commits, api: true  do
 
       it "returns project commits" do
         commit = project.repository.commit
+
         get api("/projects/#{project.id}/repository/commits", user)
 
         expect(response).to have_http_status(200)
@@ -27,6 +28,16 @@ describe API::Commits, api: true  do
         expect(json_response.first['committer_name']).to eq(commit.committer_name)
         expect(json_response.first['committer_email']).to eq(commit.committer_email)
       end
+
+      it 'include correct pagination headers' do
+        commit_count = project.repository.count_commits(ref: 'master').to_s
+
+        get api("/projects/#{project.id}/repository/commits", user)
+
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq(commit_count)
+        expect(response.headers['X-Page']).to eql('1')
+      end
     end
 
     context "unauthorized user" do
@@ -39,14 +50,26 @@ describe API::Commits, api: true  do
     context "since optional parameter" do
       it "returns project commits since provided parameter" do
         commits = project.repository.commits("master")
-        since = commits.second.created_at
+        after = commits.second.created_at
 
-        get api("/projects/#{project.id}/repository/commits?since=#{since.utc.iso8601}", user)
+        get api("/projects/#{project.id}/repository/commits?since=#{after.utc.iso8601}", user)
 
         expect(json_response.size).to eq 2
         expect(json_response.first["id"]).to eq(commits.first.id)
         expect(json_response.second["id"]).to eq(commits.second.id)
       end
+
+      it 'include correct pagination headers' do
+        commits = project.repository.commits("master")
+        after = commits.second.created_at
+        commit_count = project.repository.count_commits(ref: 'master', after: after).to_s
+
+        get api("/projects/#{project.id}/repository/commits?since=#{after.utc.iso8601}", user)
+
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq(commit_count)
+        expect(response.headers['X-Page']).to eql('1')
+      end
     end
 
     context "until optional parameter" do
@@ -65,6 +88,18 @@ describe API::Commits, api: true  do
         expect(json_response.first["id"]).to eq(commits.second.id)
         expect(json_response.second["id"]).to eq(commits.third.id)
       end
+
+      it 'include correct pagination headers' do
+        commits = project.repository.commits("master")
+        before = commits.second.created_at
+        commit_count = project.repository.count_commits(ref: 'master', before: before).to_s
+
+        get api("/projects/#{project.id}/repository/commits?until=#{before.utc.iso8601}", user)
+
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq(commit_count)
+        expect(response.headers['X-Page']).to eql('1')
+      end
     end
 
     context "invalid xmlschema date parameters" do
@@ -79,28 +114,44 @@ describe API::Commits, api: true  do
     context "path optional parameter" do
       it "returns project commits matching provided path parameter" do
         path = 'files/ruby/popen.rb'
+        commit_count = project.repository.count_commits(ref: 'master', path: path).to_s
 
         get api("/projects/#{project.id}/repository/commits?path=#{path}", user)
 
         expect(json_response.size).to eq(3)
         expect(json_response.first["id"]).to eq("570e7b2abdd848b95f2f578043fc23bd6f6fd24d")
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq(commit_count)
       end
-    end
 
-    context 'pagination' do
-      it_behaves_like 'a paginated resources'
+      it 'include correct pagination headers' do
+        path = 'files/ruby/popen.rb'
+        commit_count = project.repository.count_commits(ref: 'master', path: path).to_s
+
+        get api("/projects/#{project.id}/repository/commits?path=#{path}", user)
 
-      let(:page) { 0 }
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq(commit_count)
+        expect(response.headers['X-Page']).to eql('1')
+      end
+    end
+
+    context 'with pagination params' do
+      let(:page) { 1 }
       let(:per_page) { 5 }
       let(:ref_name) { 'master' }
       let!(:request) do
         get api("/projects/#{project.id}/repository/commits?page=#{page}&per_page=#{per_page}&ref_name=#{ref_name}", user)
       end
 
-      it 'returns the commit count in the correct header' do
-        commit_count = project.repository.commit_count_for_ref(ref_name).to_s
+      it 'returns correct headers' do
+        commit_count = project.repository.count_commits(ref: ref_name).to_s
 
+        expect(response).to include_pagination_headers
         expect(response.headers['X-Total']).to eq(commit_count)
+        expect(response.headers['X-Page']).to eq('1')
+        expect(response.headers['Link']).to match(/page=1&per_page=5/)
+        expect(response.headers['Link']).to match(/page=2&per_page=5/)
       end
 
       context 'viewing the first page' do
@@ -109,17 +160,19 @@ describe API::Commits, api: true  do
 
           expect(json_response.size).to eq(per_page)
           expect(json_response.first['id']).to eq(commit.id)
+          expect(response.headers['X-Page']).to eq('1')
         end
       end
 
-      context 'viewing the second page' do
-        let(:page) { 1 }
+      context 'viewing the third page' do
+        let(:page) { 3 }
 
-        it 'returns the second 5 commits' do
-          commit = project.repository.commits('HEAD', offset: per_page * page).first
+        it 'returns the third 5 commits' do
+          commit = project.repository.commits('HEAD', offset: (page - 1) * per_page).first
 
           expect(json_response.size).to eq(per_page)
           expect(json_response.first['id']).to eq(commit.id)
+          expect(response.headers['X-Page']).to eq('3')
         end
       end
     end