diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index 11662e9048ee0d1e157ce1924d665b2146435648..1fec21113d382874b37c72a745025d294a96daa1 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 32433e0bb4c0768331099b97bc87a35d5d58514d..a24ac9d0269df3e9c19d7c5b09a9eb16721ec928 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 {