Merge branch 'smh-identify-same-repo-user-commit-files' into 'master'

Handle start and target repositoires being the same in UserCommitFiles

See merge request gitlab-org/gitaly!2941

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 de5d815ff..3e1962d99 100644
--- a/internal/gitaly/service/operations/commit_files_test.go
+++ b/internal/gitaly/service/operations/commit_files_test.go
@@ -62,10 +62,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()
@@ -73,20 +76,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 {
@@ -772,11 +776,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,
@@ -786,6 +788,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)
@@ -794,21 +813,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 {