diff options
| -rw-r--r-- | go.mod | 2 | ||||
| -rw-r--r-- | internal/gitaly/service/commit/find_commit_test.go | 512 |
2 files changed, 264 insertions, 250 deletions
@@ -43,6 +43,7 @@ require ( golang.org/x/exp v0.0.0-20230811145659-89c5cff77bcb golang.org/x/sync v0.3.0 golang.org/x/sys v0.11.0 + golang.org/x/text v0.12.0 golang.org/x/time v0.3.0 google.golang.org/grpc v1.57.0 google.golang.org/protobuf v1.31.0 @@ -186,7 +187,6 @@ require ( golang.org/x/mod v0.12.0 // indirect golang.org/x/net v0.13.0 // indirect golang.org/x/oauth2 v0.10.0 // indirect - golang.org/x/text v0.12.0 // indirect golang.org/x/tools v0.11.0 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect gonum.org/v1/gonum v0.11.0 // indirect diff --git a/internal/gitaly/service/commit/find_commit_test.go b/internal/gitaly/service/commit/find_commit_test.go index 69b0d67ad..e8fcbb203 100644 --- a/internal/gitaly/service/commit/find_commit_test.go +++ b/internal/gitaly/service/commit/find_commit_test.go @@ -1,5 +1,3 @@ -//go:build !gitaly_test_sha256 - package commit import ( @@ -10,289 +8,311 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/helper" "gitlab.com/gitlab-org/gitaly/v16/internal/structerr" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" + "golang.org/x/text/encoding/charmap" "google.golang.org/grpc/metadata" - "google.golang.org/protobuf/types/known/timestamppb" ) -func TestSuccessfulFindCommitRequest(t *testing.T) { +func TestFindCommit(t *testing.T) { t.Parallel() - windows1251Message := testhelper.MustReadFile(t, "testdata/commit-c809470461118b7bcab850f6e9a7ca97ac42f8ea-message.txt") ctx := testhelper.Context(t) - cfg, repoProto, repoPath, client := setupCommitServiceWithRepo(t, ctx) - - bigMessage := "An empty commit with REALLY BIG message\n\n" + strings.Repeat("MOAR!\n", 20*1024) - bigCommitID := gittest.WriteCommit(t, cfg, repoPath, - gittest.WithBranch("local-big-commits"), gittest.WithMessage(bigMessage), - gittest.WithParents("60ecb67744cb56576c30214ff52294f8ce2def98"), - ) - - testCases := []struct { - description string - revision string - trailers bool - commit *gitalypb.GitCommit + cfg, client := setupCommitService(t, ctx) + + writeCommit := func(t *testing.T, repoProto *gitalypb.Repository, opts ...gittest.WriteCommitOption) (git.ObjectID, *gitalypb.GitCommit) { + t.Helper() + + repo := localrepo.NewTestRepo(t, cfg, repoProto) + repoPath, err := repo.Path() + require.NoError(t, err) + + commitID := gittest.WriteCommit(t, cfg, repoPath, opts...) + commitProto, err := repo.ReadCommit(ctx, commitID.Revision()) + require.NoError(t, err) + + return commitID, commitProto + } + + type setupData struct { + request *gitalypb.FindCommitRequest + expectedErr error + expectedCommit *gitalypb.GitCommit + } + + for _, tc := range []struct { + desc string + setup func(t *testing.T) setupData }{ { - description: "With a branch name", - revision: "branch-merged", - commit: testhelper.GitLabTestCommit("498214de67004b1da3d820901307bed2a68a8ef6"), + desc: "Repository is nil", + setup: func(t *testing.T) setupData { + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: nil, + }, + expectedErr: structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet), + } + }, }, { - description: "With a tag name no trailers", - revision: "v1.0.0", - trailers: false, - commit: &gitalypb.GitCommit{ - Id: "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9", - Subject: []byte("More submodules"), - Body: []byte("More submodules\n\nSigned-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>\n"), - Author: &gitalypb.CommitAuthor{ - Name: []byte("Dmitriy Zaporozhets"), - Email: []byte("dmitriy.zaporozhets@gmail.com"), - Date: ×tamppb.Timestamp{Seconds: 1393491261}, - Timezone: []byte("+0200"), - }, - Committer: &gitalypb.CommitAuthor{ - Name: []byte("Dmitriy Zaporozhets"), - Email: []byte("dmitriy.zaporozhets@gmail.com"), - Date: ×tamppb.Timestamp{Seconds: 1393491261}, - Timezone: []byte("+0200"), - }, - ParentIds: []string{"d14d6c0abdd253381df51a723d58691b2ee1ab08"}, - BodySize: 84, - SignatureType: gitalypb.SignatureType_PGP, - TreeId: "70d69cce111b0e1f54f7e5438bbbba9511a8e23c", + desc: "unknown storage", + setup: func(t *testing.T) setupData { + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: &gitalypb.Repository{ + StorageName: "fake", + RelativePath: "path", + }, + }, + expectedErr: testhelper.ToInterceptedMetadata(structerr.NewInvalidArgument( + "%w", storage.NewStorageNotFoundError("fake"), + )), + } }, }, { - description: "With a tag name and trailers", - revision: "v1.0.0", - trailers: true, - commit: &gitalypb.GitCommit{ - Id: "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9", - Subject: []byte("More submodules"), - Body: []byte("More submodules\n\nSigned-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>\n"), - Author: &gitalypb.CommitAuthor{ - Name: []byte("Dmitriy Zaporozhets"), - Email: []byte("dmitriy.zaporozhets@gmail.com"), - Date: ×tamppb.Timestamp{Seconds: 1393491261}, - Timezone: []byte("+0200"), - }, - Committer: &gitalypb.CommitAuthor{ - Name: []byte("Dmitriy Zaporozhets"), - Email: []byte("dmitriy.zaporozhets@gmail.com"), - Date: ×tamppb.Timestamp{Seconds: 1393491261}, - Timezone: []byte("+0200"), - }, - ParentIds: []string{"d14d6c0abdd253381df51a723d58691b2ee1ab08"}, - BodySize: 84, - SignatureType: gitalypb.SignatureType_PGP, - TreeId: "70d69cce111b0e1f54f7e5438bbbba9511a8e23c", - Trailers: []*gitalypb.CommitTrailer{ - { - Key: []byte("Signed-off-by"), - Value: []byte("Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>"), + desc: "unset revision", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, }, - }, + expectedErr: structerr.NewInvalidArgument("empty revision"), + } }, }, { - description: "With a hash", - revision: "b83d6e391c22777fca1ed3012fce84f633d7fed0", - commit: testhelper.GitLabTestCommit("b83d6e391c22777fca1ed3012fce84f633d7fed0"), + desc: "invalid revision", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte("-master"), + }, + expectedErr: structerr.NewInvalidArgument("revision can't start with '-'"), + } + }, }, { - description: "With an initial commit", - revision: "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863", - commit: testhelper.GitLabTestCommit("1a0b36b3cdad1d2ee32457c102a8c0b7056fa863"), + desc: "invalid revision", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte("mas:ter"), + }, + expectedErr: structerr.NewInvalidArgument("revision can't contain ':'"), + } + }, }, { - description: "More submodules", - revision: "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9", - trailers: true, - commit: &gitalypb.GitCommit{ - Id: "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9", - Subject: []byte("More submodules"), - Body: []byte("More submodules\n\nSigned-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>\n"), - Author: &gitalypb.CommitAuthor{ - Name: []byte("Dmitriy Zaporozhets"), - Email: []byte("dmitriy.zaporozhets@gmail.com"), - Date: ×tamppb.Timestamp{Seconds: 1393491261}, - Timezone: []byte("+0200"), - }, - Committer: &gitalypb.CommitAuthor{ - Name: []byte("Dmitriy Zaporozhets"), - Email: []byte("dmitriy.zaporozhets@gmail.com"), - Date: ×tamppb.Timestamp{Seconds: 1393491261}, - Timezone: []byte("+0200"), - }, - ParentIds: []string{"d14d6c0abdd253381df51a723d58691b2ee1ab08"}, - BodySize: 84, - SignatureType: gitalypb.SignatureType_PGP, - TreeId: "70d69cce111b0e1f54f7e5438bbbba9511a8e23c", - Trailers: []*gitalypb.CommitTrailer{ - { - Key: []byte("Signed-off-by"), - Value: []byte("Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>"), + desc: "non-existent reference", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte("does-not-exist"), }, - }, + } }, }, { - description: "with non-utf8 message encoding, recognized by Git", - revision: "c809470461118b7bcab850f6e9a7ca97ac42f8ea", - commit: &gitalypb.GitCommit{ - Id: "c809470461118b7bcab850f6e9a7ca97ac42f8ea", - Subject: windows1251Message[:len(windows1251Message)-1], - Body: windows1251Message, - Author: &gitalypb.CommitAuthor{ - Name: []byte("Jacob Vosmaer"), - Email: []byte("jacob@gitlab.com"), - Date: ×tamppb.Timestamp{Seconds: 1512132977}, - Timezone: []byte("+0100"), - }, - Committer: &gitalypb.CommitAuthor{ - Name: []byte("Jacob Vosmaer"), - Email: []byte("jacob@gitlab.com"), - Date: ×tamppb.Timestamp{Seconds: 1512132977}, - Timezone: []byte("+0100"), - }, - ParentIds: []string{"e63f41fe459e62e1228fcef60d7189127aeba95a"}, - BodySize: 49, - TreeId: "86ec18bfe87ad42a782fdabd8310f9b7ac750f51", - Encoding: "windows-1251", + desc: "non-existent object ID", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte(gittest.DefaultObjectHash.HashData([]byte("nonexistent-commit"))), + }, + } }, }, { - description: "with non-utf8 garbage message encoding, not recognized by Git", - revision: "0999bb770f8dc92ab5581cc0b474b3e31a96bf5c", - commit: &gitalypb.GitCommit{ - Id: "0999bb770f8dc92ab5581cc0b474b3e31a96bf5c", - Subject: []byte("Hello\xf0world"), - Body: []byte("Hello\xf0world\n"), - Author: &gitalypb.CommitAuthor{ - Name: []byte("Jacob Vosmaer"), - Email: []byte("jacob@gitlab.com"), - Date: ×tamppb.Timestamp{Seconds: 1517328273}, - Timezone: []byte("+0100"), - }, - Committer: &gitalypb.CommitAuthor{ - Name: []byte("Jacob Vosmaer"), - Email: []byte("jacob@gitlab.com"), - Date: ×tamppb.Timestamp{Seconds: 1517328273}, - Timezone: []byte("+0100"), - }, - ParentIds: []string{"60ecb67744cb56576c30214ff52294f8ce2def98"}, - BodySize: 12, - TreeId: "7e2f26d033ee47cd0745649d1a28277c56197921", + desc: "by branch name", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + _, commit := writeCommit(t, repo, gittest.WithBranch("branch")) + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte("branch"), + }, + expectedCommit: commit, + } }, }, { - description: "with a very large message", - revision: bigCommitID.String(), - commit: &gitalypb.GitCommit{ - Id: bigCommitID.String(), - Subject: []byte("An empty commit with REALLY BIG message"), - Author: gittest.DefaultCommitAuthor, - Committer: gittest.DefaultCommitAuthor, - ParentIds: []string{"60ecb67744cb56576c30214ff52294f8ce2def98"}, - Body: []byte(bigMessage[:helper.MaxCommitOrTagMessageSize]), - BodySize: int64(len(bigMessage)), - TreeId: "7e2f26d033ee47cd0745649d1a28277c56197921", + desc: "by tag", + setup: func(t *testing.T) setupData { + repo, repoPath := gittest.CreateRepository(t, ctx, cfg) + + commitID, commit := writeCommit(t, repo) + gittest.WriteTag(t, cfg, repoPath, "v1.0.0", commitID.Revision(), gittest.WriteTagConfig{ + Message: "tag message", + }) + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte("v1.0.0"), + }, + expectedCommit: commit, + } }, }, { - description: "with different author and committer", - revision: "77e835ef0856f33c4f0982f84d10bdb0567fe440", - commit: testhelper.GitLabTestCommit("77e835ef0856f33c4f0982f84d10bdb0567fe440"), + desc: "by commit ID", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + commitID, commit := writeCommit(t, repo) + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte(commitID), + }, + expectedCommit: commit, + } + }, }, { - description: "With a non-existing ref name", - revision: "this-doesnt-exists", - commit: nil, + desc: "without commit trailer", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + commitID, commit := writeCommit(t, repo, gittest.WithMessage("subject\n\nbody\n\nSigned-off-by: foo\n")) + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte(commitID), + Trailers: false, + }, + expectedCommit: commit, + } + }, }, { - description: "With a non-existing hash", - revision: "f48214de67004b1da3d820901307bed2a68a8ef6", - commit: nil, + desc: "with commit trailer", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + + commitID, commit := writeCommit(t, repo, gittest.WithMessage("subject\n\nbody\n\nSigned-off-by: foo\n")) + commit.Trailers = []*gitalypb.CommitTrailer{ + {Key: []byte("Signed-off-by"), Value: []byte("foo")}, + } + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte(commitID), + Trailers: true, + }, + expectedCommit: commit, + } + }, }, - } - - for _, testCase := range testCases { - t.Run(testCase.description, func(t *testing.T) { - request := &gitalypb.FindCommitRequest{ - Repository: repoProto, - Revision: []byte(testCase.revision), - Trailers: testCase.trailers, - } - - response, err := client.FindCommit(ctx, request) - require.NoError(t, err) - - testhelper.ProtoEqual(t, testCase.commit, response.Commit) - }) - } -} + { + desc: "with non-UTF8 encoding recognized by Git", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) -func TestFailedFindCommitRequest(t *testing.T) { - t.Parallel() + windows1250, err := charmap.Windows1250.NewEncoder().String("üöä") + require.NoError(t, err) - ctx := testhelper.Context(t) - _, repo, _, client := setupCommitServiceWithRepo(t, ctx) + commitID, commit := writeCommit(t, repo, gittest.WithEncoding("windows-1250"), gittest.WithMessage(windows1250)) + commit.Body = []byte(windows1250) + commit.Encoding = "windows-1250" - invalidRepo := &gitalypb.Repository{StorageName: "fake", RelativePath: "path"} - - for _, tc := range []struct { - desc string - revision []byte - repo *gitalypb.Repository - expectedErr error - }{ - { - desc: "Repository is nil", - repo: nil, - expectedErr: structerr.NewInvalidArgument("%w", storage.ErrRepositoryNotSet), - }, - { - desc: "Invalid repo", - repo: invalidRepo, - revision: []byte("master"), - expectedErr: testhelper.ToInterceptedMetadata(structerr.NewInvalidArgument( - "%w", storage.NewStorageNotFoundError("fake"), - )), + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte(commitID), + }, + expectedCommit: commit, + } + }, }, { - desc: "Empty revision", - repo: repo, - revision: []byte(""), - expectedErr: structerr.NewInvalidArgument("empty revision"), + desc: "with non-UTF8 encoding not recognized by Git", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + + commitID, commit := writeCommit(t, repo, gittest.WithMessage("Hello�orld")) + commit.Body = []byte("Hello�orld") + commit.Encoding = "" + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte(commitID), + }, + expectedCommit: commit, + } + }, }, { - desc: "Invalid revision", - repo: repo, - revision: []byte("-master"), - expectedErr: structerr.NewInvalidArgument("revision can't start with '-'"), + desc: "very large commit message", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + + bigMessage := "An empty commit with REALLY BIG message\n\n" + strings.Repeat("MOAR!\n", 20*1024) + commitID, commit := writeCommit(t, repo, gittest.WithMessage(bigMessage)) + commit.Body = []byte(bigMessage[:helper.MaxCommitOrTagMessageSize]) + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte(commitID), + }, + expectedCommit: commit, + } + }, }, { - desc: "Invalid revision", - repo: repo, - revision: []byte("mas:ter"), - expectedErr: structerr.NewInvalidArgument("revision can't contain ':'"), + desc: "with different author and committer", + setup: func(t *testing.T) setupData { + repo, _ := gittest.CreateRepository(t, ctx, cfg) + + commitID, commit := writeCommit(t, repo, gittest.WithAuthorName("author"), gittest.WithCommitterName("committer")) + commit.Author.Name = []byte("author") + commit.Committer.Name = []byte("committer") + + return setupData{ + request: &gitalypb.FindCommitRequest{ + Repository: repo, + Revision: []byte(commitID), + }, + expectedCommit: commit, + } + }, }, } { + tc := tc + t.Run(tc.desc, func(t *testing.T) { - request := &gitalypb.FindCommitRequest{ - Repository: tc.repo, - Revision: tc.revision, - } + t.Parallel() - _, err := client.FindCommit(ctx, request) - testhelper.RequireGrpcError(t, tc.expectedErr, err) + setup := tc.setup(t) + + response, err := client.FindCommit(ctx, setup.request) + testhelper.RequireGrpcError(t, setup.expectedErr, err) + testhelper.ProtoEqual(t, setup.expectedCommit, response.GetCommit()) }) } } @@ -342,38 +362,32 @@ func benchmarkFindCommit(b *testing.B, withCache bool) { } } -func TestFindCommitWithCache(t *testing.T) { +func TestFindCommit_cached(t *testing.T) { t.Parallel() ctx := testhelper.Context(t) - cfg, repo, _, client := setupCommitServiceWithRepo(t, ctx) + cfg, client := setupCommitService(t, ctx) - // get a list of revisions + repo, repoPath := gittest.CreateRepository(t, ctx, cfg) - gitCmdFactory := gittest.NewCommandFactory(t, cfg) - logCmd, err := gitCmdFactory.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), - git.Command{Name: "log", Flags: []git.Option{git.Flag{Name: "--format=format:%H"}}}) - require.NoError(t, err) - - logScanner := bufio.NewScanner(logCmd) + var commitIDs []git.ObjectID + for i := 0; i < 10; i++ { + var parents []git.ObjectID + if i > 0 { + parents = []git.ObjectID{commitIDs[i-1]} + } - var revisions []string - for logScanner.Scan() { - revisions = append(revisions, logScanner.Text()) + commitIDs = append(commitIDs, gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(parents...))) } - require.NoError(t, logCmd.Wait()) + ctx = metadata.NewOutgoingContext(ctx, metadata.New(map[string]string{ + "gitaly-session-id": "abc123", + })) - for i := 0; i < 10; i++ { - revision := revisions[i%len(revisions)] - md := metadata.New(map[string]string{ - "gitaly-session-id": "abc123", - }) - - ctx = metadata.NewOutgoingContext(ctx, md) + for _, commitID := range commitIDs { _, err := client.FindCommit(ctx, &gitalypb.FindCommitRequest{ Repository: repo, - Revision: []byte(revision), + Revision: []byte(commitID), }) require.NoError(t, err) } |