diff --git a/changelogs/unreleased/smh-enable-user-commit-files.yml b/changelogs/unreleased/smh-enable-user-commit-files.yml new file mode 100644 index 0000000000000000000000000000000000000000..2334da4d3e481cda2b2d122c67ccc50478d50739 --- /dev/null +++ b/changelogs/unreleased/smh-enable-user-commit-files.yml @@ -0,0 +1,5 @@ +--- +title: Enable Go port of UserCommitFiles by default +merge_request: 2922 +author: +type: performance diff --git a/internal/git/repository.go b/internal/git/repository.go index fe852abacee578dd7d14a66b5b4f12bc65cee46d..b62ad16e8179783fe3e972a850b7a1076301d956 100644 --- a/internal/git/repository.go +++ b/internal/git/repository.go @@ -131,20 +131,39 @@ func (repo *LocalRepository) command(ctx context.Context, globals []GlobalOption return NewCommand(ctx, repo.repo, globals, cmd, opts...) } +// WriteBlobOptions contains options that can be set to influence how the blob +// is written to the object database. +type WriteBlobOptions struct { + // Path is the path of the written file. It is used to determine which + // filter to run on the written blob. + Path string + // NormalizeLineEndings when set configures `core.autocrlf=input` + // to normalize the line endings of the blob. If not set, the decision + // is deferred to git by using existing configuration and attributes. + NormalizeLineEndings bool +} + // WriteBlob writes a blob to the repository's object database and // returns its object ID. Path is used by git to decide which filters to // run on the content. -func (repo *LocalRepository) WriteBlob(ctx context.Context, path string, content io.Reader) (string, error) { +func (repo *LocalRepository) WriteBlob(ctx context.Context, content io.Reader, opts WriteBlobOptions) (string, error) { stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} - cmd, err := repo.command(ctx, nil, + var globalOpts []GlobalOption + if opts.NormalizeLineEndings { + globalOpts = append(globalOpts, ValueFlag{Name: "-c", Value: "core.autocrlf=input"}) + } + + cmdOpts := []Option{Flag{Name: "--stdin"}, Flag{Name: "-w"}} + if opts.Path != "" { + cmdOpts = append(cmdOpts, ValueFlag{Name: "--path", Value: opts.Path}) + } + + cmd, err := repo.command(ctx, globalOpts, SubCmd{ - Name: "hash-object", - Flags: []Option{ - ValueFlag{Name: "--path", Value: path}, - Flag{Name: "--stdin"}, Flag{Name: "-w"}, - }, + Name: "hash-object", + Flags: cmdOpts, }, WithStdin(content), WithStdout(stdout), diff --git a/internal/git/repository_test.go b/internal/git/repository_test.go index 348b19d0fab9f5a7036f25675edbca4a665397e9..c5d7d4f2778e557d6bf5301a78ecb1995e181656 100644 --- a/internal/git/repository_test.go +++ b/internal/git/repository_test.go @@ -187,22 +187,17 @@ func TestLocalRepository_WriteBlob(t *testing.T) { pbRepo, repoPath, clean := testhelper.InitBareRepo(t) defer clean() - // write attributes file so we can verify WriteBlob runs the files through filters as - // appropriate - require.NoError(t, ioutil.WriteFile(filepath.Join(repoPath, "info", "attributes"), []byte(` -crlf binary -lf text - `), os.ModePerm)) - repo := NewRepository(pbRepo) for _, tc := range []struct { - desc string - path string - input io.Reader - sha string - error error - content string + desc string + attributes string + config string + input io.Reader + options WriteBlobOptions + sha string + error error + content string }{ { desc: "error reading", @@ -212,38 +207,56 @@ lf text { desc: "successful empty blob", input: strings.NewReader(""), - sha: "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391", content: "", }, { desc: "successful blob", input: strings.NewReader("some content"), - sha: "f0eec86f614944a81f87d879ebdc9a79aea0d7ea", content: "some content", }, { - desc: "line endings not normalized", - path: "crlf", + desc: "line endings not normalized without attributes or config", input: strings.NewReader("\r\n"), - sha: "d3f5a12faa99758192ecc4ed3fc22c9249232e86", content: "\r\n", }, { - desc: "line endings normalized", - path: "lf", + desc: "line endings normalized due to attributes", + attributes: "file-path text", + input: strings.NewReader("\r\n"), + options: WriteBlobOptions{Path: "file-path"}, + content: "\n", + }, + { + desc: "line endings normalized due to config", + options: WriteBlobOptions{Path: "file-path"}, + config: ` +[core] +autocrlf = input + `, input: strings.NewReader("\r\n"), - sha: "8b137891791fe96927ad78e64b0aad7bded08bdc", + content: "\n", + }, + { + desc: "line endings normalized due to options", + input: strings.NewReader("\r\n"), + options: WriteBlobOptions{Path: "file-path", NormalizeLineEndings: true}, content: "\n", }, } { t.Run(tc.desc, func(t *testing.T) { - sha, err := repo.WriteBlob(ctx, tc.path, tc.input) + for path, content := range map[string]string{ + filepath.Join(repoPath, "info", "attributes"): tc.attributes, + filepath.Join(repoPath, "config"): tc.config, + } { + require.NoError(t, ioutil.WriteFile(path, []byte(content), os.ModePerm)) + } + + sha, err := repo.WriteBlob(ctx, tc.input, tc.options) require.Equal(t, tc.error, err) if tc.error != nil { return } - assert.Equal(t, tc.sha, sha) content, err := repo.ReadObject(ctx, sha) require.NoError(t, err) assert.Equal(t, tc.content, string(content)) diff --git a/internal/git2go/apply_test.go b/internal/git2go/apply_test.go index fe12d603d3e0cb34fe1bc4e023aafdcedb731856..71a0333d255cd9764e493af371204a19cfbbaae7 100644 --- a/internal/git2go/apply_test.go +++ b/internal/git2go/apply_test.go @@ -23,13 +23,13 @@ func TestExecutor_Apply(t *testing.T) { ctx, cancel := testhelper.Context() defer cancel() - oidBase, err := repo.WriteBlob(ctx, "file", strings.NewReader("base")) + oidBase, err := repo.WriteBlob(ctx, strings.NewReader("base"), git.WriteBlobOptions{}) require.NoError(t, err) - oidA, err := repo.WriteBlob(ctx, "file", strings.NewReader("a")) + oidA, err := repo.WriteBlob(ctx, strings.NewReader("a"), git.WriteBlobOptions{}) require.NoError(t, err) - oidB, err := repo.WriteBlob(ctx, "file", strings.NewReader("b")) + oidB, err := repo.WriteBlob(ctx, strings.NewReader("b"), git.WriteBlobOptions{}) require.NoError(t, err) author := NewSignature("Test Author", "test.author@example.com", time.Now()) diff --git a/internal/git2go/commit_test.go b/internal/git2go/commit_test.go index 215cc59dae080bdc570b3fb6f7179a2550eb78c9..b00481ff9dd669d247b4cc513c169e12535b1bee 100644 --- a/internal/git2go/commit_test.go +++ b/internal/git2go/commit_test.go @@ -57,10 +57,10 @@ func TestExecutor_Commit(t *testing.T) { repo := git.NewRepository(pbRepo) - originalFile, err := repo.WriteBlob(ctx, "file", bytes.NewBufferString("original")) + originalFile, err := repo.WriteBlob(ctx, bytes.NewBufferString("original"), git.WriteBlobOptions{}) require.NoError(t, err) - updatedFile, err := repo.WriteBlob(ctx, "file", bytes.NewBufferString("updated")) + updatedFile, err := repo.WriteBlob(ctx, bytes.NewBufferString("updated"), git.WriteBlobOptions{}) require.NoError(t, err) executor := New(filepath.Join(config.Config.BinDir, "gitaly-git2go"), config.Config.Git.BinPath) diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index 449176f33cb4bd7e16309ddc5ad4b8cb2dbc8764..65cfeaceafdc052c43d5ab10fdc3ca276f563969 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "strings" "time" "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" @@ -141,6 +142,17 @@ func (s *Server) UserCommitFiles(stream gitalypb.OperationService_UserCommitFile func validatePath(rootPath, relPath string) (string, error) { if relPath == "" { return "", indexError("You must provide a file path") + } else if strings.Contains(relPath, "//") { + // This is a workaround to address a quirk in porting the RPC from Ruby to Go. + // GitLab's QA pipeline runs tests with filepath 'invalid://file/name/here'. + // Go's filepath.Clean returns 'invalid:/file/name/here'. The Ruby implementation's + // filepath normalization accepted the path as is. Adding a file with this path to the + // index via Rugged failed with an invalid path error. As Go's cleaning resulted a valid + // filepath, adding the file succeeded, which made the QA pipeline's specs fail. + // + // The Rails code expects to receive an error prefixed with 'invalid path', which is done + // here to retain compatibility. + return "", indexError(fmt.Sprintf("invalid path: '%s'", relPath)) } path, err := storage.ValidateRelativePath(rootPath, relPath) @@ -254,7 +266,10 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi switch pbAction.header.Action { case gitalypb.UserCommitFilesActionHeader_CREATE: - blobID, err := localRepo.WriteBlob(ctx, path, content) + blobID, err := localRepo.WriteBlob(ctx, content, git.WriteBlobOptions{ + Path: path, + NormalizeLineEndings: true, + }) if err != nil { return fmt.Errorf("write created blob: %w", err) } @@ -278,7 +293,10 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi var oid string if !pbAction.header.InferContent { var err error - oid, err = localRepo.WriteBlob(ctx, path, content) + oid, err = localRepo.WriteBlob(ctx, content, git.WriteBlobOptions{ + Path: path, + NormalizeLineEndings: true, + }) if err != nil { return err } @@ -290,7 +308,10 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi OID: oid, }) case gitalypb.UserCommitFilesActionHeader_UPDATE: - oid, err := localRepo.WriteBlob(ctx, path, content) + oid, err := localRepo.WriteBlob(ctx, content, git.WriteBlobOptions{ + Path: path, + NormalizeLineEndings: true, + }) if err != nil { return fmt.Errorf("write updated blob: %w", err) } @@ -374,30 +395,30 @@ func (s *Server) resolveParentCommit(ctx context.Context, local git.Repository, if err != nil { return "", fmt.Errorf("remote repository: %w", err) } + } - if hasBranches, err := repo.HasBranches(ctx); err != nil { - return "", fmt.Errorf("has branches: %w", err) - } else if !hasBranches { - // GitLab sends requests to UserCommitFiles where target repository - // and start repository are the same. If the request hits Gitaly directly, - // Gitaly could check if the repos are the same by comparing their storages - // and relative paths and simply resolve the branch locally. When request is proxied - // through Praefect, the start repository's storage is not rewritten, thus Gitaly can't - // identify the repos as being the same. - // - // If the start repository is set, we have to resolve the branch there as it - // might be on a different commit than the local repository. As Gitaly can't identify - // the repositories are the same behind Praefect, it has to perform an RPC to resolve - // the branch. The resolving would fail as the branch does not yet exist in the start - // repository, which is actually the local repository. - // - // Due to this, we check if the remote has any branches. If not, we likely hit this case - // and we're creating the first branch. If so, we'll just return the commit that was - // already resolved locally. - // - // See: https://gitlab.com/gitlab-org/gitaly/-/issues/3294 - return targetBranchCommit, nil - } + if hasBranches, err := repo.HasBranches(ctx); err != nil { + return "", fmt.Errorf("has branches: %w", err) + } else if !hasBranches { + // GitLab sends requests to UserCommitFiles where target repository + // and start repository are the same. If the request hits Gitaly directly, + // Gitaly could check if the repos are the same by comparing their storages + // and relative paths and simply resolve the branch locally. When request is proxied + // through Praefect, the start repository's storage is not rewritten, thus Gitaly can't + // identify the repos as being the same. + // + // If the start repository is set, we have to resolve the branch there as it + // might be on a different commit than the local repository. As Gitaly can't identify + // the repositories are the same behind Praefect, it has to perform an RPC to resolve + // the branch. The resolving would fail as the branch does not yet exist in the start + // repository, which is actually the local repository. + // + // Due to this, we check if the remote has any branches. If not, we likely hit this case + // and we're creating the first branch. If so, we'll just return the commit that was + // already resolved locally. + // + // See: https://gitlab.com/gitlab-org/gitaly/-/issues/3294 + return targetBranchCommit, nil } branch := targetBranch diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go index 3853a7437f783d3c700548279d37024e747d512f..44b29a200c150d231ba753483819dabd9be6e241 100644 --- a/internal/gitaly/service/operations/commit_files_test.go +++ b/internal/gitaly/service/operations/commit_files_test.go @@ -87,6 +87,7 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) { type step struct { actions []*gitalypb.UserCommitFilesRequest startRepository *gitalypb.Repository + startBranch string error error indexError string repoCreated bool @@ -216,6 +217,18 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) { }, }, }, + { + desc: "create file with double slash", + steps: []step{ + { + actions: []*gitalypb.UserCommitFilesRequest{ + createFileHeaderRequest("invalid://file/name/here"), + actionContentRequest("content-1"), + }, + indexError: "invalid path: 'invalid://file/name/here'", + }, + }, + }, { desc: "create file without content", steps: []step{ @@ -281,6 +294,23 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) { }, }, }, + { + desc: "create file normalizes line endings", + steps: []step{ + { + actions: []*gitalypb.UserCommitFilesRequest{ + createFileHeaderRequest("file-1"), + actionContentRequest("content-1\r\n"), + actionContentRequest(" content-2\r\n"), + }, + repoCreated: true, + branchCreated: true, + treeEntries: []testhelper.TreeEntry{ + {Mode: DefaultMode, Path: "file-1", Content: "content-1\n content-2\n"}, + }, + }, + }, + }, { desc: "create duplicate file", steps: []step{ @@ -329,6 +359,24 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) { }, }, }, + { + desc: "update file normalizes line endings", + steps: []step{ + { + actions: []*gitalypb.UserCommitFilesRequest{ + createFileHeaderRequest("file-1"), + actionContentRequest("content-1"), + updateFileHeaderRequest("file-1"), + actionContentRequest("content-2\r\n"), + }, + repoCreated: true, + branchCreated: true, + treeEntries: []testhelper.TreeEntry{ + {Mode: DefaultMode, Path: "file-1", Content: "content-2\n"}, + }, + }, + }, + }, { desc: "update base64 content", steps: []step{ @@ -582,6 +630,31 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) { }, }, }, + { + desc: "move file normalizes line endings", + steps: []step{ + { + actions: []*gitalypb.UserCommitFilesRequest{ + createFileHeaderRequest("original-file"), + actionContentRequest("original-content"), + }, + repoCreated: true, + branchCreated: true, + treeEntries: []testhelper.TreeEntry{ + {Mode: DefaultMode, Path: "original-file", Content: "original-content"}, + }, + }, + { + actions: []*gitalypb.UserCommitFilesRequest{ + moveFileHeaderRequest("original-file", "moved-file", false), + actionContentRequest("new-content\r\n"), + }, + treeEntries: []testhelper.TreeEntry{ + {Mode: DefaultMode, Path: "moved-file", Content: "new-content\n"}, + }, + }, + }, + }, { desc: "mark non-existing file executable", steps: []step{ @@ -790,13 +863,31 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) { }, }, { - desc: "start repository refers to an empty repository", + desc: "empty target repository with start branch set", steps: []step{ { actions: []*gitalypb.UserCommitFilesRequest{ createFileHeaderRequest("file-1"), actionContentRequest("content-1"), }, + startBranch: "master", + branchCreated: true, + repoCreated: true, + treeEntries: []testhelper.TreeEntry{ + {Mode: DefaultMode, Path: "file-1", Content: "content-1"}, + }, + }, + }, + }, + { + desc: "start repository refers to an empty remote repository", + steps: []step{ + { + actions: []*gitalypb.UserCommitFilesRequest{ + createFileHeaderRequest("file-1"), + actionContentRequest("content-1"), + }, + startBranch: "master", startRepository: startRepo, branchCreated: true, repoCreated: true, @@ -828,6 +919,10 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) { setStartRepository(headerRequest, step.startRepository) } + if step.startBranch != "" { + setStartBranchName(headerRequest, []byte(step.startBranch)) + } + stream, err := client.UserCommitFiles(ctx) require.NoError(t, err) require.NoError(t, stream.Send(headerRequest)) diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 20dd2a695b73b9dcbf50371df692cb7f73dbc4a9..bf5b72634732664be0598de52fc1abf62e6b92c7 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -26,7 +26,7 @@ var ( // GoUserDeleteBranch enables the Go implementation of UserDeleteBranch GoUserDeleteBranch = FeatureFlag{Name: "go_user_delete_branch", OnByDefault: true} // GoUserCommitFiles enables the Go implementation of UserCommitFiles - GoUserCommitFiles = FeatureFlag{Name: "go_user_commit_files", OnByDefault: false} + GoUserCommitFiles = FeatureFlag{Name: "go_user_commit_files", OnByDefault: true} // GoResolveConflicts enables the Go implementation of ResolveConflicts GoResolveConflicts = FeatureFlag{Name: "go_resolve_conflicts", OnByDefault: false} // GoUserUpdateSubmodule enables the Go implementation of