From b51e2fc3134755cbe8fe59208334a64e0464034c Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Sat, 26 Dec 2020 13:03:32 +0200 Subject: [PATCH 1/6] handle non-existent start branch in UserCommitFiles UserCommitFiles receives requests where the StartBranch is set even for an empty repository. Such requests fail when attempting to resolve the start branch' commit as the branch does not exist. The Ruby implementation of the RPC went around this case by checking whether the repository has no branches. If so, it ignored the start branch. This scenario caused failures in the QA pipeline. It doesn't appear such requests are sent in production though. This commit fixes the failing pipeline by mirroring the Ruby implementations behavior and ignoring the start branch when the repository has no branches. The Go implementation already handled this for the remote repository. This changes the handling to also take place when the start repository is not set. --- .../gitaly/service/operations/commit_files.go | 46 +++++++++---------- .../service/operations/commit_files_test.go | 25 +++++++++- 2 files changed, 47 insertions(+), 24 deletions(-) diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index 449176f33cb..dbb01a8f156 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -374,30 +374,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 3853a7437f7..fa0a2ab5c89 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 @@ -790,13 +791,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 +847,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)) -- GitLab From da7399b81aa4750468a7d1f33aa3ac5424758ca2 Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Sat, 26 Dec 2020 13:17:25 +0200 Subject: [PATCH 2/6] Revert "Revert "Revert "Merge branch 'smh-revert-user-commit-files' into 'master'""" This reverts commit f68256c48dafb65944de4fef30fe634fe76152ed. --- changelogs/unreleased/smh-enable-user-commit-files.yml | 5 +++++ internal/metadata/featureflag/feature_flags.go | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/smh-enable-user-commit-files.yml 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 00000000000..2334da4d3e4 --- /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/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 20dd2a695b7..bf5b7263473 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 -- GitLab From 27b74bb91094a663e6aa174e1fa2b4999a8cd47e Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Mon, 18 Jan 2021 13:24:17 +0100 Subject: [PATCH 3/6] extend WriteBlob tests to cover config settings also Currently WriteBlob tests the file endings are normalized when appropriate attributes are set. This commit extends the tests to also cover cases when autocrlf config option has been set. --- internal/git/repository_test.go | 46 ++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/internal/git/repository_test.go b/internal/git/repository_test.go index 348b19d0fab..36afc81bed0 100644 --- a/internal/git/repository_test.go +++ b/internal/git/repository_test.go @@ -187,22 +187,16 @@ 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 + sha string + error error + content string }{ { desc: "error reading", @@ -222,22 +216,38 @@ lf text 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"), + sha: "8b137891791fe96927ad78e64b0aad7bded08bdc", + content: "\n", + }, + { + desc: "line endings normalized due to config", + config: ` +[core] +autocrlf = input + `, input: strings.NewReader("\r\n"), sha: "8b137891791fe96927ad78e64b0aad7bded08bdc", 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, "file-path", tc.input) require.Equal(t, tc.error, err) if tc.error != nil { return -- GitLab From 6eb1003c306e8008134f7f7c178820ea7a76cd48 Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Mon, 18 Jan 2021 13:26:01 +0100 Subject: [PATCH 4/6] don't assert against object sha in WriteBlob tests The tests are asserting against the content of the blob fetched by the returned sha. The sha has to be correct for the correct object to be fetched. This makes asserting the sha redundant due to which this commit removes the explicit assertation. --- internal/git/repository_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/internal/git/repository_test.go b/internal/git/repository_test.go index 36afc81bed0..ba8b708d4fe 100644 --- a/internal/git/repository_test.go +++ b/internal/git/repository_test.go @@ -206,26 +206,22 @@ func TestLocalRepository_WriteBlob(t *testing.T) { { 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 without attributes or config", input: strings.NewReader("\r\n"), - sha: "d3f5a12faa99758192ecc4ed3fc22c9249232e86", content: "\r\n", }, { desc: "line endings normalized due to attributes", attributes: "file-path text", input: strings.NewReader("\r\n"), - sha: "8b137891791fe96927ad78e64b0aad7bded08bdc", content: "\n", }, { @@ -235,7 +231,6 @@ func TestLocalRepository_WriteBlob(t *testing.T) { autocrlf = input `, input: strings.NewReader("\r\n"), - sha: "8b137891791fe96927ad78e64b0aad7bded08bdc", content: "\n", }, } { @@ -253,7 +248,6 @@ autocrlf = input return } - assert.Equal(t, tc.sha, sha) content, err := repo.ReadObject(ctx, sha) require.NoError(t, err) assert.Equal(t, tc.content, string(content)) -- GitLab From 45611853ebf92819dcbff1b62e2f8c341c04ae8c Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Mon, 18 Jan 2021 15:03:35 +0100 Subject: [PATCH 5/6] normalize line endings in Go port of UserCommitFiles WriteBlob currently relies on existing configuration in the repository to determine whether the line endings should be normalized or not. The Ruby implementation of UserCommitFiles however always ran with 'core.autocrlf=input'. The Go port has to follow the suit, so this commit adds and option to normalize the line endings by setting the appropriate config option on the 'hash-object' invocation. This behavior difference was caught by a failing test in the QA pipeline of Rails. --- internal/git/repository.go | 33 +++++++--- internal/git/repository_test.go | 13 +++- internal/git2go/apply_test.go | 6 +- internal/git2go/commit_test.go | 4 +- .../gitaly/service/operations/commit_files.go | 15 ++++- .../service/operations/commit_files_test.go | 60 +++++++++++++++++++ 6 files changed, 114 insertions(+), 17 deletions(-) diff --git a/internal/git/repository.go b/internal/git/repository.go index fe852abacee..b62ad16e817 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 ba8b708d4fe..c5d7d4f2778 100644 --- a/internal/git/repository_test.go +++ b/internal/git/repository_test.go @@ -194,6 +194,7 @@ func TestLocalRepository_WriteBlob(t *testing.T) { attributes string config string input io.Reader + options WriteBlobOptions sha string error error content string @@ -222,10 +223,12 @@ func TestLocalRepository_WriteBlob(t *testing.T) { 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", + desc: "line endings normalized due to config", + options: WriteBlobOptions{Path: "file-path"}, config: ` [core] autocrlf = input @@ -233,6 +236,12 @@ autocrlf = input input: strings.NewReader("\r\n"), 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) { for path, content := range map[string]string{ @@ -242,7 +251,7 @@ autocrlf = input require.NoError(t, ioutil.WriteFile(path, []byte(content), os.ModePerm)) } - sha, err := repo.WriteBlob(ctx, "file-path", tc.input) + sha, err := repo.WriteBlob(ctx, tc.input, tc.options) require.Equal(t, tc.error, err) if tc.error != nil { return diff --git a/internal/git2go/apply_test.go b/internal/git2go/apply_test.go index fe12d603d3e..71a0333d255 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 215cc59dae0..b00481ff9dd 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 dbb01a8f156..51ff91481de 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -254,7 +254,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 +281,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 +296,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) } diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go index fa0a2ab5c89..ed955b115ba 100644 --- a/internal/gitaly/service/operations/commit_files_test.go +++ b/internal/gitaly/service/operations/commit_files_test.go @@ -282,6 +282,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{ @@ -330,6 +347,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{ @@ -583,6 +618,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{ -- GitLab From 6a7ee4ed5fa77b4d7017b76a7d87b40875cc1590 Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Wed, 20 Jan 2021 16:36:17 +0100 Subject: [PATCH 6/6] fix path normalization deviation in UserCommitFiles Go port The Ruby implementation's path normalization accepts double slashes in the path as is. This later causes Rugged to error out when attempting to add a file with such an invalid path to the index. The Go port is cleaning the filepaths, which replaces double slashes with single slashes. This makes the filepath valid and thus adding the file to the index didn't error out. This commit a special cases double slashes to workaround this quirk. --- internal/gitaly/service/operations/commit_files.go | 12 ++++++++++++ .../gitaly/service/operations/commit_files_test.go | 12 ++++++++++++ 2 files changed, 24 insertions(+) diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index 51ff91481de..65cfeaceafd 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) diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go index ed955b115ba..44b29a200c1 100644 --- a/internal/gitaly/service/operations/commit_files_test.go +++ b/internal/gitaly/service/operations/commit_files_test.go @@ -217,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{ -- GitLab