summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/repository.rb12
-rw-r--r--changelogs/unreleased/1381-present-commits-pagination-headers-correctly.yml4
-rw-r--r--doc/api/v3_to_v4.md3
-rw-r--r--lib/api/commits.rb28
-rw-r--r--lib/gitlab/git/repository.rb12
-rw-r--r--spec/lib/gitlab/git/repository_spec.rb26
-rw-r--r--spec/models/repository_spec.rb29
-rw-r--r--spec/requests/api/commits_spec.rb94
8 files changed, 195 insertions, 13 deletions
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 2a12b36a84d..6ab04440ca8 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -312,11 +312,13 @@ class Repository
if !branch_name || branch_name == root_ref
branches.each do |branch|
cache.expire(:"diverging_commit_counts_#{branch.name}")
+ cache.expire(:"commit_count_#{branch.name}")
end
# In case a commit is pushed to a non-root branch we only have to flush the
# cache for said branch.
else
cache.expire(:"diverging_commit_counts_#{branch_name}")
+ cache.expire(:"commit_count_#{branch_name}")
end
end
@@ -496,6 +498,16 @@ class Repository
end
cache_method :commit_count, fallback: 0
+ def commit_count_for_ref(ref)
+ return 0 unless exists?
+
+ begin
+ cache.fetch(:"commit_count_#{ref}") { raw_repository.commit_count(ref) }
+ rescue Rugged::ReferenceError
+ 0
+ end
+ end
+
def branch_names
branches.map(&:name)
end
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/doc/api/v3_to_v4.md b/doc/api/v3_to_v4.md
index bf180836b57..95d4c9a13b3 100644
--- a/doc/api/v3_to_v4.md
+++ b/doc/api/v3_to_v4.md
@@ -73,3 +73,6 @@ Below are the changes made between V3 and 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 b0aa10f8bf2..42401abfe0f 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -18,22 +18,34 @@ 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'
- offset = params[:page] * params[:per_page]
+ 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: commit_count)
- present commits, with: Entities::RepoCommit
+ present paginate(paginated_commits), with: Entities::RepoCommit
end
desc 'Commit multiple file changes as one commit' do
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 6540730ca7a..228ef7bb7a9 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -354,6 +354,18 @@ module Gitlab
lines.map! { |c| Rugged::Commit.new(rugged, c.strip) }
end
+ def count_commits(options)
+ 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
+
def sha_from_ref(ref)
rev_parse_target(ref).oid
end
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 3f11f0a4516..bc139d5ef28 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -824,6 +824,32 @@ describe Gitlab::Git::Repository, seed_helper: true do
it { is_expected.to eq(17) }
end
+ describe '#count_commits' do
+ context 'with after timestamp' do
+ it 'returns the number of commits after timestamp' do
+ options = { ref: 'master', limit: nil, after: Time.iso8601('2013-03-03T20:15:01+00:00') }
+
+ expect(repository.count_commits(options)).to eq(25)
+ end
+ end
+
+ context 'with before timestamp' do
+ it 'returns the number of commits after timestamp' do
+ options = { ref: 'feature', limit: nil, before: Time.iso8601('2015-03-03T20:15:01+00:00') }
+
+ expect(repository.count_commits(options)).to eq(9)
+ end
+ end
+
+ context 'with path' do
+ it 'returns the number of commits with path ' do
+ options = { ref: 'master', limit: nil, path: "encoding" }
+
+ expect(repository.count_commits(options)).to eq(2)
+ end
+ end
+ end
+
describe "branch_names_contains" do
subject { repository.branch_names_contains(SeedRepo::LastCommit::ID) }
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index eb992e1354e..274e4f00a0a 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1042,7 +1042,7 @@ describe Repository, models: true do
it 'expires the cache for all branches' do
expect(cache).to receive(:expire).
- at_least(repository.branches.length).
+ at_least(repository.branches.length * 2).
times
repository.expire_branch_cache
@@ -1050,14 +1050,14 @@ describe Repository, models: true do
it 'expires the cache for all branches when the root branch is given' do
expect(cache).to receive(:expire).
- at_least(repository.branches.length).
+ at_least(repository.branches.length * 2).
times
repository.expire_branch_cache(repository.root_ref)
end
it 'expires the cache for a specific branch' do
- expect(cache).to receive(:expire).once
+ expect(cache).to receive(:expire).twice
repository.expire_branch_cache('foo')
end
@@ -1742,6 +1742,29 @@ describe Repository, models: true do
end
end
+ describe '#commit_count_for_ref' do
+ let(:project) { create :empty_project }
+
+ context 'with a non-existing repository' do
+ it 'returns 0' do
+ 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
+
+ context 'when searching for the root ref' do
+ it 'returns the same count as #commit_count' do
+ expect(repository.commit_count_for_ref(repository.root_ref)).to eq(repository.commit_count)
+ end
+ end
+ end
+
describe '#cache_method_output', caching: true do
context 'with a non-existing repository' do
let(:value) do
diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb
index 5190fcca2d1..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,11 +114,66 @@ 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
+
+ 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)
+
+ 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 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
+ it 'returns the first 5 commits' do
+ commit = project.repository.commit
+
+ 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 third page' do
+ let(:page) { 3 }
+
+ 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
end
View Lorry Controller Status