From 6a57e01bb308ee17ed4012988a8a3770b660b0b4 Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Wed, 16 Dec 2020 18:55:34 +0100 Subject: [PATCH] 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. --- .../gitaly/service/operations/commit_files.go | 18 +++++- .../service/operations/commit_files_test.go | 62 ++++++++++++------- 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 11662e9048e..1fec21113d3 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 32433e0bb4c..a24ac9d0269 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 { -- GitLab