handle start and target repositoires being the same in UserCommitFiles

Gitaly receives requests in UserCommitFiles where the start and the
target repositories are the same. In such cases, it doesn't have to
resolve the branches from start repository as it is the same. This
commit checks if the repositories are the same and only operates on
the local repository if that is the case.

2 files changed, 57 insertions, 23 deletions

diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go
index 11662e904..1fec21113 100644
--- a/internal/gitaly/service/operations/commit_files.go
+++ b/internal/gitaly/service/operations/commit_files.go
@@ -161,6 +161,15 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi
 		return fmt.Errorf("get repo path: %w", err)
 	}
 
+	remoteRepo := header.GetStartRepository()
+	if sameRepository(header.GetRepository(), remoteRepo) {
+		// Some requests set a StartRepository that refers to the same repository as the target repository.
+		// This check never works behind Praefect. See: https://gitlab.com/gitlab-org/gitaly/-/issues/3294
+		// Plain Gitalies still benefit from identifying the case and avoiding unnecessary RPC to resolve the
+		// branch.
+		remoteRepo = nil
+	}
+
 	localRepo := git.NewRepository(header.Repository)
 
 	targetBranchName := "refs/heads/" + string(header.BranchName)
@@ -178,7 +187,7 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi
 		parentCommitOID, err = s.resolveParentCommit(
 			ctx,
 			localRepo,
-			header.StartRepository,
+			remoteRepo,
 			targetBranchName,
 			targetBranchCommit,
 			string(header.StartBranchName),
@@ -189,7 +198,7 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi
 	}
 
 	if parentCommitOID != targetBranchCommit {
-		if err := s.fetchMissingCommit(ctx, header.Repository, header.StartRepository, parentCommitOID); err != nil {
+		if err := s.fetchMissingCommit(ctx, header.Repository, remoteRepo, parentCommitOID); err != nil {
 			return fmt.Errorf("fetch missing commit: %w", err)
 		}
 	}
@@ -348,6 +357,11 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi
 	}})
 }
 
+func sameRepository(repoA, repoB *gitalypb.Repository) bool {
+	return repoA.GetStorageName() == repoB.GetStorageName() &&
+		repoA.GetRelativePath() == repoB.GetRelativePath()
+}
+
 func (s *Server) resolveParentCommit(ctx context.Context, local git.Repository, remote *gitalypb.Repository, targetBranch, targetBranchCommit, startBranch string) (string, error) {
 	if remote == nil && startBranch == "" {
 		return targetBranchCommit, nil
diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go
index 32433e0bb..a24ac9d02 100644
--- a/internal/gitaly/service/operations/commit_files_test.go
+++ b/internal/gitaly/service/operations/commit_files_test.go
@@ -61,10 +61,13 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) {
 	defer os.RemoveAll(storageRoot)
 
 	const storageName = "default"
-	relativePath, err := filepath.Rel(testhelper.GitlabTestStoragePath(), filepath.Join(storageRoot, "test-repository"))
+	targetRelativePath, err := filepath.Rel(testhelper.GitlabTestStoragePath(), filepath.Join(storageRoot, "target-repository"))
 	require.NoError(t, err)
 
-	repoPath := filepath.Join(testhelper.GitlabTestStoragePath(), relativePath)
+	startRepo, _, cleanStartRepo := testhelper.InitBareRepo(t)
+	defer cleanStartRepo()
+
+	repoPath := filepath.Join(testhelper.GitlabTestStoragePath(), targetRelativePath)
 
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
@@ -72,20 +75,21 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) {
 	client, conn := newOperationClient(t, serverSocketPath)
 	defer conn.Close()
 
+	ctxWithServerMetadata := ctx
 	for key, values := range testhelper.GitalyServersMetadata(t, serverSocketPath) {
 		for _, value := range values {
-			ctx = metadata.AppendToOutgoingContext(ctx, key, value)
+			ctxWithServerMetadata = metadata.AppendToOutgoingContext(ctxWithServerMetadata, key, value)
 		}
 	}
 
 	type step struct {
-		actions       []*gitalypb.UserCommitFilesRequest
-		changeHeader  func(*gitalypb.UserCommitFilesRequest)
-		error         error
-		indexError    string
-		repoCreated   bool
-		branchCreated bool
-		treeEntries   []testhelper.TreeEntry
+		actions         []*gitalypb.UserCommitFilesRequest
+		startRepository *gitalypb.Repository
+		error           error
+		indexError      string
+		repoCreated     bool
+		branchCreated   bool
+		treeEntries     []testhelper.TreeEntry
 	}
 
 	for _, tc := range []struct {
@@ -771,11 +775,9 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) {
 						createFileHeaderRequest("file-1"),
 						actionContentRequest("content-1"),
 					},
-					changeHeader: func(header *gitalypb.UserCommitFilesRequest) {
-						setStartRepository(header, &gitalypb.Repository{
-							StorageName:  storageName,
-							RelativePath: relativePath,
-						})
+					startRepository: &gitalypb.Repository{
+						StorageName:  storageName,
+						RelativePath: targetRelativePath,
 					},
 					branchCreated: true,
 					repoCreated:   true,
@@ -785,6 +787,23 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) {
 				},
 			},
 		},
+		{
+			desc: "start repository refers to an empty repository",
+			steps: []step{
+				{
+					actions: []*gitalypb.UserCommitFilesRequest{
+						createFileHeaderRequest("file-1"),
+						actionContentRequest("content-1"),
+					},
+					startRepository: startRepo,
+					branchCreated:   true,
+					repoCreated:     true,
+					treeEntries: []testhelper.TreeEntry{
+						{Mode: DefaultMode, Path: "file-1", Content: "content-1"},
+					},
+				},
+			},
+		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			defer os.RemoveAll(repoPath)
@@ -793,21 +812,22 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) {
 			const branch = "master"
 
 			for i, step := range tc.steps {
-				stream, err := client.UserCommitFiles(ctx)
-				require.NoError(t, err)
-
 				headerRequest := headerRequest(
-					testhelper.CreateRepo(t, storageRoot, relativePath),
+					testhelper.CreateRepo(t, storageRoot, targetRelativePath),
 					testhelper.TestUser,
 					branch,
 					[]byte("commit message"),
 				)
 				setAuthorAndEmail(headerRequest, []byte("Author Name"), []byte("author.email@example.com"))
 
-				if step.changeHeader != nil {
-					step.changeHeader(headerRequest)
+				ctx := ctx
+				if step.startRepository != nil {
+					ctx = ctxWithServerMetadata
+					setStartRepository(headerRequest, step.startRepository)
 				}
 
+				stream, err := client.UserCommitFiles(ctx)
+				require.NoError(t, err)
 				require.NoError(t, stream.Send(headerRequest))
 
 				for j, action := range step.actions {