Fix catfile N+1 in ListLastCommitsForTree

4 files changed, 47 insertions, 15 deletions

diff --git a/changelogs/unreleased/last-commit-catfile.yml b/changelogs/unreleased/last-commit-catfile.yml
new file mode 100644
index 000000000..1235a78d3
--- /dev/null
+++ b/changelogs/unreleased/last-commit-catfile.yml
@@ -0,0 +1,5 @@
+---
+title: Fix catfile N+1 in ListLastCommitsForTree
+merge_request: 1253
+author:
+type: performance
diff --git a/internal/git/log/last_commit.go b/internal/git/log/last_commit.go
index 0f356d570..7bd6af470 100644
--- a/internal/git/log/last_commit.go
+++ b/internal/git/log/last_commit.go
@@ -9,11 +9,12 @@ import (
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 )
 
 // LastCommitForPath returns the last commit which modified path.
-func LastCommitForPath(ctx context.Context, repo *gitalypb.Repository, revision string, path string) (*gitalypb.GitCommit, error) {
+func LastCommitForPath(ctx context.Context, batch *catfile.Batch, repo *gitalypb.Repository, revision string, path string) (*gitalypb.GitCommit, error) {
 	cmd, err := git.Command(ctx, repo, "log", "--format=%H", "--max-count=1", revision, "--", path)
 	if err != nil {
 		return nil, err
@@ -24,7 +25,7 @@ func LastCommitForPath(ctx context.Context, repo *gitalypb.Repository, revision
 		return nil, err
 	}
 
-	return GetCommit(ctx, repo, text.ChompBytes(commitID))
+	return GetCommitCatfile(batch, text.ChompBytes(commitID))
 }
 
 // GitLogCommand returns a Command that executes git log with the given the arguments
diff --git a/internal/service/commit/last_commit_for_path.go b/internal/service/commit/last_commit_for_path.go
index fc4b896f3..d5d1c4d3a 100644
--- a/internal/service/commit/last_commit_for_path.go
+++ b/internal/service/commit/last_commit_for_path.go
@@ -5,22 +5,37 @@ import (
 	"fmt"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
 func (s *server) LastCommitForPath(ctx context.Context, in *gitalypb.LastCommitForPathRequest) (*gitalypb.LastCommitForPathResponse, error) {
 	if err := validateLastCommitForPathRequest(in); err != nil {
-		return nil, status.Errorf(codes.InvalidArgument, "LastCommitForPath: %v", err)
+		return nil, helper.ErrInvalidArgument(err)
 	}
 
+	resp, err := lastCommitForPath(ctx, in)
+	if err != nil {
+		return nil, helper.ErrInternal(err)
+	}
+
+	return resp, nil
+}
+
+func lastCommitForPath(ctx context.Context, in *gitalypb.LastCommitForPathRequest) (*gitalypb.LastCommitForPathResponse, error) {
 	path := string(in.GetPath())
 	if len(path) == 0 || path == "/" {
 		path = "."
 	}
 
-	commit, err := log.LastCommitForPath(ctx, in.GetRepository(), string(in.GetRevision()), path)
+	repo := in.GetRepository()
+	c, err := catfile.New(ctx, repo)
+	if err != nil {
+		return nil, err
+	}
+
+	commit, err := log.LastCommitForPath(ctx, c, repo, string(in.GetRevision()), path)
 	if log.IsNotFound(err) {
 		return &gitalypb.LastCommitForPathResponse{}, nil
 	}
diff --git a/internal/service/commit/list_last_commits_for_tree.go b/internal/service/commit/list_last_commits_for_tree.go
index b82947b48..7b2741c4d 100644
--- a/internal/service/commit/list_last_commits_for_tree.go
+++ b/internal/service/commit/list_last_commits_for_tree.go
@@ -8,10 +8,10 @@ import (
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/git/lstree"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
 const (
@@ -27,16 +27,27 @@ var (
 
 func (s *server) ListLastCommitsForTree(in *gitalypb.ListLastCommitsForTreeRequest, stream gitalypb.CommitService_ListLastCommitsForTreeServer) error {
 	if err := validateListLastCommitsForTreeRequest(in); err != nil {
-		return status.Errorf(codes.InvalidArgument, "ListLastCommitsForTree: %v", err)
+		return helper.ErrInvalidArgument(err)
 	}
 
+	if err := listLastCommitsForTree(in, stream); err != nil {
+		return helper.ErrInternal(err)
+	}
+
+	return nil
+}
+
+func listLastCommitsForTree(in *gitalypb.ListLastCommitsForTreeRequest, stream gitalypb.CommitService_ListLastCommitsForTreeServer) error {
 	cmd, parser, err := newLSTreeParser(in, stream)
 	if err != nil {
-		if _, ok := status.FromError(err); ok {
-			return err
-		}
+		return err
+	}
 
-		return status.Errorf(codes.Internal, "ListLastCommitsForTree: gitCommand: %v", err)
+	ctx := stream.Context()
+	repo := in.GetRepository()
+	c, err := catfile.New(ctx, repo)
+	if err != nil {
+		return err
 	}
 
 	batch := make([]*gitalypb.ListLastCommitsForTreeResponse_CommitForTree, 0, maxNumStatBatchSize)
@@ -57,7 +68,7 @@ func (s *server) ListLastCommitsForTree(in *gitalypb.ListLastCommitsForTreeReque
 	}
 
 	for _, entry := range entries[offset:limit] {
-		commit, err := log.LastCommitForPath(stream.Context(), in.GetRepository(), string(in.GetRevision()), entry.Path)
+		commit, err := log.LastCommitForPath(ctx, c, repo, string(in.GetRevision()), entry.Path)
 		if err != nil {
 			return err
 		}
@@ -78,7 +89,7 @@ func (s *server) ListLastCommitsForTree(in *gitalypb.ListLastCommitsForTreeReque
 	}
 
 	if err := cmd.Wait(); err != nil {
-		return status.Errorf(codes.Internal, "ListLastCommitsForTree: %v", err)
+		return err
 	}
 
 	return sendCommitsForTree(batch, stream)