Merge branch '37433-solve-n-1-in-refs-controller-logs-tree' into 'master'

Adds list_last_commits_for_tree as the method call for the Gitaly ListLastCommitsForTree RPC

Closes #37433

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

GITALY_SERVER_VERSION

-0.122.0
+0.123.0

Gemfile

@@ -420,7 +420,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.117.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.118.1', require: 'gitaly'
 gem 'grpc', '~> 1.11.0'
 
 # Locked until https://github.com/google/protobuf/issues/4210 is closed

Gemfile.lock

@@ -274,7 +274,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (0.117.0)
+    gitaly-proto (0.118.1)
       google-protobuf (~> 3.1)
       grpc (~> 1.10)
     github-linguist (5.3.3)
@@ -1028,7 +1028,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.117.0)
+  gitaly-proto (~> 0.118.1)
   github-linguist (~> 5.3.3)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-gollum-lib (~> 4.2)

Gemfile.rails5.lock

@@ -277,7 +277,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (0.117.0)
+    gitaly-proto (0.118.1)
       google-protobuf (~> 3.1)
       grpc (~> 1.10)
     github-linguist (5.3.3)
@@ -1037,7 +1037,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.117.0)
+  gitaly-proto (~> 0.118.1)
   github-linguist (~> 5.3.3)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-gollum-lib (~> 4.2)

app/models/repository.rb

@@ -668,6 +668,14 @@ class Repository
     end
   end
 
+  def list_last_commits_for_tree(sha, path, offset: 0, limit: 25)
+    commits = raw_repository.list_last_commits_for_tree(sha, path, offset: offset, limit: limit)
+
+    commits.each do |path, commit|
+      commits[path] = ::Commit.new(commit, @project)
+    end
+  end
+
   def last_commit_for_path(sha, path)
     commit = raw_repository.last_commit_for_path(sha, path)
     ::Commit.new(commit, @project) if commit

changelogs/unreleased/37433-solve-n-1-in-refs-controller-logs-tree.yml

+---
+title: Adds support for Gitaly ListLastCommitsForTree RPC in order to make bulk-fetch
+  of commits more performant
+merge_request: 21921
+author:
+type: performance

lib/gitlab/git/repository.rb

@@ -953,6 +953,12 @@ module Gitlab
         end
       end
 
+      def list_last_commits_for_tree(sha, path, offset: 0, limit: 25)
+        wrapped_gitaly_errors do
+          gitaly_commit_client.list_last_commits_for_tree(sha, path, offset: offset, limit: limit)
+        end
+      end
+
       def last_commit_for_path(sha, path)
         wrapped_gitaly_errors do
           gitaly_commit_client.last_commit_for_path(sha, path)

lib/gitlab/gitaly_client/commit_service.rb

@@ -148,6 +148,24 @@ module Gitlab
         GitalyClient.call(@repository.storage, :commit_service, :count_commits, request, timeout: GitalyClient.medium_timeout).count
       end
 
+      def list_last_commits_for_tree(revision, path, offset: 0, limit: 25)
+        request = Gitaly::ListLastCommitsForTreeRequest.new(
+          repository: @gitaly_repo,
+          revision: encode_binary(revision),
+          path: encode_binary(path.to_s),
+          offset: offset,
+          limit: limit
+        )
+
+        response = GitalyClient.call(@repository.storage, :commit_service, :list_last_commits_for_tree, request, timeout: GitalyClient.medium_timeout)
+
+        response.each_with_object({}) do |gitaly_response, hsh|
+          gitaly_response.commits.each do |commit_for_tree|
+            hsh[commit_for_tree.path] = Gitlab::Git::Commit.new(@repository, commit_for_tree.commit)
+          end
+        end
+      end
+
       def last_commit_for_path(revision, path)
         request = Gitaly::LastCommitForPathRequest.new(
           repository: @gitaly_repo,

lib/gitlab/tree_summary.rb

@@ -73,25 +73,29 @@ module Gitlab
     end
 
     def fill_last_commits!(entries)
-      # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37433
-      Gitlab::GitalyClient.allow_n_plus_1_calls do
-        entries.each do |entry|
-          raw_commit = repository.last_commit_for_path(commit.id, entry_path(entry))
+      # Ensure the path is in "path/" format
+      ensured_path =
+        if path
+          File.join(*[path, ""])
+        end
 
-          if raw_commit
-            commit = resolve_commit(raw_commit)
+      commits_hsh = repository.list_last_commits_for_tree(commit.id, ensured_path, offset: offset, limit: limit)
 
+      entries.each do |entry|
+        path_key = entry_path(entry)
+        commit = cache_commit(commits_hsh[path_key])
+
+        if commit
           entry[:commit] = commit
           entry[:commit_path] = commit_path(commit)
         end
       end
     end
-    end
 
-    def resolve_commit(raw_commit)
-      return nil unless raw_commit.present?
+    def cache_commit(commit)
+      return nil unless commit.present?
 
-      resolved_commits[raw_commit.id] ||= ::Commit.new(raw_commit, project)
+      resolved_commits[commit.id] ||= commit
     end
 
     def commit_path(commit)

spec/models/repository_spec.rb

@@ -188,6 +188,57 @@ describe Repository do
     end
   end
 
+  describe '#list_last_commits_for_tree' do
+    let(:path_to_commit) do
+      {
+        "encoding" => "913c66a37b4a45b9769037c55c2d238bd0942d2e",
+        "files" => "570e7b2abdd848b95f2f578043fc23bd6f6fd24d",
+        ".gitignore" => "c1acaa58bbcbc3eafe538cb8274ba387047b69f8",
+        ".gitmodules" => "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9",
+        "CHANGELOG" => "913c66a37b4a45b9769037c55c2d238bd0942d2e",
+        "CONTRIBUTING.md" => "6d394385cf567f80a8fd85055db1ab4c5295806f",
+        "Gemfile.zip" => "ae73cb07c9eeaf35924a10f713b364d32b2dd34f",
+        "LICENSE" => "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863",
+        "MAINTENANCE.md" => "913c66a37b4a45b9769037c55c2d238bd0942d2e",
+        "PROCESS.md" => "913c66a37b4a45b9769037c55c2d238bd0942d2e",
+        "README.md" => "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863",
+        "VERSION" => "913c66a37b4a45b9769037c55c2d238bd0942d2e",
+        "gitlab-shell" => "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9",
+        "six" => "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
+      }
+    end
+
+    subject { repository.list_last_commits_for_tree(sample_commit.id, '.').id }
+
+    it 'returns the last commits for every entry in the current path' do
+      result = repository.list_last_commits_for_tree(sample_commit.id, '.')
+
+      result.each do |key, value|
+        result[key] = value.id
+      end
+
+      expect(result).to include(path_to_commit)
+    end
+
+    it 'returns the last commits for every entry in the current path starting from the offset' do
+      result = repository.list_last_commits_for_tree(sample_commit.id, '.', offset: path_to_commit.size - 1)
+
+      expect(result.size).to eq(1)
+    end
+
+    it 'returns a limited number of last commits for every entry in the current path starting from the offset' do
+      result = repository.list_last_commits_for_tree(sample_commit.id, '.', limit: 1)
+
+      expect(result.size).to eq(1)
+    end
+
+    it 'returns an empty hash when offset is out of bounds' do
+      result = repository.list_last_commits_for_tree(sample_commit.id, '.', offset: path_to_commit.size)
+
+      expect(result.size).to eq(0)
+    end
+  end
+
   describe '#last_commit_for_path' do
     shared_examples 'getting last commit for path' do
       subject { repository.last_commit_for_path(sample_commit.id, '.gitignore').id }