diff options
author | Toon Claes <toon@gitlab.com> | 2022-08-04 22:33:22 +0300 |
---|---|---|
committer | Toon Claes <toon@gitlab.com> | 2022-08-04 22:33:22 +0300 |
commit | b731241d770f8cd1e8b81f48d8a011f0383b4b48 (patch) | |
tree | 0cc11c475f4b47cdee5f8bf32473d50dd7148d27 | |
parent | 993d944ebeea3e0ec8157481f125f9c70161ae4a (diff) | |
parent | 8bdd79826c9817796b5d77c45cc15d7ce350a20c (diff) |
Merge branch 'pks-git-catfile-sha-256' into 'master'
catfile: Support repositories with SHA256 object hashes
See merge request gitlab-org/gitaly!4779
22 files changed, 602 insertions, 375 deletions
diff --git a/internal/git/catfile/cache_test.go b/internal/git/catfile/cache_test.go index b3c5e08de..0ac225444 100644 --- a/internal/git/catfile/cache_test.go +++ b/internal/git/catfile/cache_test.go @@ -1,5 +1,3 @@ -//go:build !gitaly_test_sha256 - package catfile import ( @@ -11,6 +9,7 @@ import ( "time" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v15/internal/git/repository" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v15/internal/helper" @@ -24,7 +23,8 @@ func TestProcesses_add(t *testing.T) { const maxLen = 3 p := &processes{maxLen: maxLen} - cfg, repo, _ := testcfg.BuildWithRepo(t) + cfg := testcfg.Build(t) + repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0]) key0 := mustCreateKey(t, "0", repo) value0, cancel := mustCreateCacheable(t, cfg, repo) @@ -56,7 +56,8 @@ func TestProcesses_add(t *testing.T) { func TestProcesses_addTwice(t *testing.T) { p := &processes{maxLen: 10} - cfg, repo, _ := testcfg.BuildWithRepo(t) + cfg := testcfg.Build(t) + repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0]) key0 := mustCreateKey(t, "0", repo) value0, cancel := mustCreateCacheable(t, cfg, repo) @@ -83,7 +84,8 @@ func TestProcesses_addTwice(t *testing.T) { func TestProcesses_Checkout(t *testing.T) { p := &processes{maxLen: 10} - cfg, repo, _ := testcfg.BuildWithRepo(t) + cfg := testcfg.Build(t) + repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0]) key0 := mustCreateKey(t, "0", repo) value0, cancel := mustCreateCacheable(t, cfg, repo) @@ -110,7 +112,8 @@ func TestProcesses_Checkout(t *testing.T) { func TestProcesses_EnforceTTL(t *testing.T) { p := &processes{maxLen: 10} - cfg, repo, _ := testcfg.BuildWithRepo(t) + cfg := testcfg.Build(t) + repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0]) cutoff := time.Now() @@ -155,7 +158,8 @@ func TestCache_autoExpiry(t *testing.T) { c := newCache(time.Hour, 10, monitorTicker) defer c.Stop() - cfg, repo, _ := testcfg.BuildWithRepo(t) + cfg := testcfg.Build(t) + repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0]) // Add a process that has expired already. key0 := mustCreateKey(t, "0", repo) @@ -179,7 +183,11 @@ func TestCache_autoExpiry(t *testing.T) { } func TestCache_ObjectReader(t *testing.T) { - cfg, repo, _ := testcfg.BuildWithRepo(t) + cfg := testcfg.Build(t) + + repo, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) + gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main")) + repoExecutor := newRepoExecutor(t, cfg, repo) cache := newCache(time.Hour, 10, helper.NewManualTicker()) @@ -223,7 +231,7 @@ func TestCache_ObjectReader(t *testing.T) { }}, keys) // Assert that we can still read from the cached process. - _, err = reader.Object(ctx, "refs/heads/master") + _, err = reader.Object(ctx, "refs/heads/main") require.NoError(t, err) }) @@ -239,7 +247,7 @@ func TestCache_ObjectReader(t *testing.T) { // While we request object data, we do not consume it at all. The reader is thus // dirty and cannot be reused and shouldn't be returned to the cache. - object, err := reader.Object(ctx, "refs/heads/master") + object, err := reader.Object(ctx, "refs/heads/main") require.NoError(t, err) // Cancel the process such that it will be considered for return to the cache. @@ -274,7 +282,11 @@ func TestCache_ObjectReader(t *testing.T) { } func TestCache_ObjectInfoReader(t *testing.T) { - cfg, repo, _ := testcfg.BuildWithRepo(t) + cfg := testcfg.Build(t) + + repo, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) + gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main")) + repoExecutor := newRepoExecutor(t, cfg, repo) cache := newCache(time.Hour, 10, helper.NewManualTicker()) @@ -317,7 +329,7 @@ func TestCache_ObjectInfoReader(t *testing.T) { }}, keys) // Assert that we can still read from the cached process. - _, err = reader.Info(ctx, "refs/heads/master") + _, err = reader.Info(ctx, "refs/heads/main") require.NoError(t, err) }) diff --git a/internal/git/catfile/commit_test.go b/internal/git/catfile/commit_test.go index 6fc58b483..81fceccec 100644 --- a/internal/git/catfile/commit_test.go +++ b/internal/git/catfile/commit_test.go @@ -1,8 +1,7 @@ -//go:build !gitaly_test_sha256 - package catfile import ( + "errors" "testing" "github.com/stretchr/testify/require" @@ -15,66 +14,81 @@ import ( func TestGetCommit(t *testing.T) { ctx := testhelper.Context(t) - - _, objectReader, _ := setupObjectReader(t, ctx) - ctx = metadata.NewIncomingContext(ctx, metadata.MD{}) - const commitSha = "2d1db523e11e777e49377cfb22d368deec3f0793" - const commitMsg = "Correct test_env.rb path for adding branch\n" - const blobSha = "c60514b6d3d6bf4bec1030f70026e34dfbd69ad5" + cfg, objectReader, _, repoPath := setupObjectReader(t, ctx) - testCases := []struct { - desc string - revision string - errStr string + blobID := gittest.WriteBlob(t, cfg, repoPath, []byte("data")) + treeID := gittest.WriteTree(t, cfg, repoPath, []gittest.TreeEntry{ + {Path: "file", Mode: "100644", OID: blobID}, + }) + commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("commit message\n\ncommit body"), gittest.WithTree(treeID)) + + for _, tc := range []struct { + desc string + revision string + expectedErr error + expectedCommit *gitalypb.GitCommit }{ { desc: "commit", - revision: commitSha, + revision: commitID.String(), + expectedCommit: &gitalypb.GitCommit{ + Id: commitID.String(), + TreeId: treeID.String(), + Author: gittest.DefaultCommitAuthor, + Committer: gittest.DefaultCommitAuthor, + Body: []byte("commit message\n\ncommit body"), + BodySize: 27, + Subject: []byte("commit message"), + }, }, { - desc: "not existing commit", - revision: "not existing revision", - errStr: "object not found", + desc: "not existing commit", + revision: "not existing revision", + expectedErr: NotFoundError{errors.New("object not found")}, }, { - desc: "blob sha", - revision: blobSha, - errStr: "object not found", + desc: "blob sha", + revision: blobID.String(), + expectedErr: NotFoundError{errors.New("object not found")}, }, - } - - for _, tc := range testCases { + } { t.Run(tc.desc, func(t *testing.T) { - c, err := GetCommit(ctx, objectReader, git.Revision(tc.revision)) - - if tc.errStr == "" { - require.NoError(t, err) - require.Equal(t, commitMsg, string(c.Body)) - } else { - require.EqualError(t, err, tc.errStr) - } + commit, err := GetCommit(ctx, objectReader, git.Revision(tc.revision)) + require.Equal(t, tc.expectedErr, err) + testhelper.ProtoEqual(t, tc.expectedCommit, commit) }) } } func TestGetCommitWithTrailers(t *testing.T) { ctx := testhelper.Context(t) + ctx = metadata.NewIncomingContext(ctx, metadata.MD{}) - cfg, objectReader, testRepo := setupObjectReader(t, ctx) + cfg, objectReader, repo, repoPath := setupObjectReader(t, ctx) - ctx = metadata.NewIncomingContext(ctx, metadata.MD{}) + commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage( + "some header\n"+ + "\n"+ + "Commit message.\n"+ + "\n"+ + "Signed-off-by: John Doe <john.doe@example.com>\n"+ + "Signed-off-by: Jane Doe <jane.doe@example.com>\n", + )) - commit, err := GetCommitWithTrailers(ctx, gittest.NewCommandFactory(t, cfg), testRepo, - objectReader, "5937ac0a7beb003549fc5fd26fc247adbce4a52e") + commit, err := GetCommitWithTrailers(ctx, gittest.NewCommandFactory(t, cfg), repo, objectReader, commitID.Revision()) require.NoError(t, err) require.Equal(t, commit.Trailers, []*gitalypb.CommitTrailer{ { Key: []byte("Signed-off-by"), - Value: []byte("Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>"), + Value: []byte("John Doe <john.doe@example.com>"), + }, + { + Key: []byte("Signed-off-by"), + Value: []byte("Jane Doe <jane.doe@example.com>"), }, }) } diff --git a/internal/git/catfile/object_info_reader.go b/internal/git/catfile/object_info_reader.go index fda2c4ee4..595f6d52e 100644 --- a/internal/git/catfile/object_info_reader.go +++ b/internal/git/catfile/object_info_reader.go @@ -49,8 +49,9 @@ func IsNotFound(err error) bool { return ok } -// ParseObjectInfo reads from a reader and parses the data into an ObjectInfo struct -func ParseObjectInfo(stdout *bufio.Reader) (*ObjectInfo, error) { +// ParseObjectInfo reads from a reader and parses the data into an ObjectInfo struct with the given +// object hash. +func ParseObjectInfo(objectHash git.ObjectHash, stdout *bufio.Reader) (*ObjectInfo, error) { restart: infoLine, err := stdout.ReadString('\n') if err != nil { @@ -75,7 +76,7 @@ restart: return nil, fmt.Errorf("invalid info line: %q", infoLine) } - oid, err := git.ObjectHashSHA1.FromHex(info[0]) + oid, err := objectHash.FromHex(info[0]) if err != nil { return nil, fmt.Errorf("parse object ID: %w", err) } @@ -125,7 +126,8 @@ type ObjectInfoQueue interface { // long-lived `git cat-file --batch-check` process such that we do not have to spawn a separate // process per object info we're about to read. type objectInfoReader struct { - cmd *command.Command + cmd *command.Command + objectHash git.ObjectHash counter *prometheus.CounterVec @@ -152,12 +154,19 @@ func newObjectInfoReader( return nil, err } + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return nil, fmt.Errorf("detecting object hash: %w", err) + } + objectInfoReader := &objectInfoReader{ - cmd: batchCmd, - counter: counter, + cmd: batchCmd, + objectHash: objectHash, + counter: counter, queue: requestQueue{ - stdout: bufio.NewReader(batchCmd), - stdin: bufio.NewWriter(batchCmd), + objectHash: objectHash, + stdout: bufio.NewReader(batchCmd), + stdin: bufio.NewWriter(batchCmd), }, } diff --git a/internal/git/catfile/object_info_reader_fuzz.go b/internal/git/catfile/object_info_reader_fuzz.go index f5227668f..a21b7bee5 100644 --- a/internal/git/catfile/object_info_reader_fuzz.go +++ b/internal/git/catfile/object_info_reader_fuzz.go @@ -5,10 +5,12 @@ package catfile import ( "bufio" "bytes" + + "gitlab.com/gitlab-org/gitaly/v15/internal/git" ) func Fuzz(data []byte) int { reader := bufio.NewReader(bytes.NewReader(data)) - ParseObjectInfo(reader) + ParseObjectInfo(git.ObjectHashSHA1, reader) return 0 } diff --git a/internal/git/catfile/object_info_reader_test.go b/internal/git/catfile/object_info_reader_test.go index a4e9b3c54..7d9614602 100644 --- a/internal/git/catfile/object_info_reader_test.go +++ b/internal/git/catfile/object_info_reader_test.go @@ -1,12 +1,12 @@ -//go:build !gitaly_test_sha256 - package catfile import ( "bufio" "errors" "fmt" + "io" "os" + "strconv" "strings" "testing" @@ -20,79 +20,114 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" ) -func TestParseObjectInfoSuccess(t *testing.T) { - testCases := []struct { - desc string - input string - output *ObjectInfo - notFound bool +func TestParseObjectInfo_success(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + desc string + input string + expectedErr error + expectedObjectInfo *ObjectInfo }{ { desc: "existing object", - input: "7c9373883988204e5a9f72c4a5119cbcefc83627 commit 222\n", - output: &ObjectInfo{ - Oid: "7c9373883988204e5a9f72c4a5119cbcefc83627", + input: fmt.Sprintf("%s commit 222\n", gittest.DefaultObjectHash.EmptyTreeOID), + expectedObjectInfo: &ObjectInfo{ + Oid: gittest.DefaultObjectHash.EmptyTreeOID, Type: "commit", Size: 222, }, }, { - desc: "non existing object", - input: "bla missing\n", - notFound: true, + desc: "non existing object", + input: "bla missing\n", + expectedErr: NotFoundError{fmt.Errorf("object not found")}, }, - } - - for _, tc := range testCases { + } { t.Run(tc.desc, func(t *testing.T) { reader := bufio.NewReader(strings.NewReader(tc.input)) - output, err := ParseObjectInfo(reader) - if tc.notFound { - require.True(t, IsNotFound(err), "expect NotFoundError") - return - } - require.NoError(t, err) - require.Equal(t, tc.output, output) + objectInfo, err := ParseObjectInfo(gittest.DefaultObjectHash, reader) + require.Equal(t, tc.expectedErr, err) + require.Equal(t, tc.expectedObjectInfo, objectInfo) }) } } -func TestParseObjectInfoErrors(t *testing.T) { - testCases := []struct { - desc string - input string - }{ - {desc: "missing newline", input: "7c9373883988204e5a9f72c4a5119cbcefc83627 commit 222"}, - {desc: "too few words", input: "7c9373883988204e5a9f72c4a5119cbcefc83627 commit\n"}, - {desc: "too many words", input: "7c9373883988204e5a9f72c4a5119cbcefc83627 commit 222 bla\n"}, - {desc: "parse object size", input: "7c9373883988204e5a9f72c4a5119cbcefc83627 commit bla\n"}, - } +func TestParseObjectInfo_errors(t *testing.T) { + t.Parallel() + + oid := gittest.DefaultObjectHash.EmptyTreeOID - for _, tc := range testCases { + for _, tc := range []struct { + desc string + input string + expectedErr error + }{ + { + desc: "missing newline", + input: fmt.Sprintf("%s commit 222", oid), + expectedErr: fmt.Errorf("read info line: %w", io.EOF), + }, + { + desc: "too few words", + input: fmt.Sprintf("%s commit\n", oid), + expectedErr: fmt.Errorf("invalid info line: %q", oid+" commit"), + }, + { + desc: "too many words", + input: fmt.Sprintf("%s commit 222 bla\n", oid), + expectedErr: fmt.Errorf("invalid info line: %q", oid+" commit 222 bla"), + }, + { + desc: "invalid object hash", + input: "7c9373883988204e5a9f72c4 commit 222 bla\n", + expectedErr: fmt.Errorf("invalid info line: %q", "7c9373883988204e5a9f72c4 commit 222 bla"), + }, + { + desc: "parse object size", + input: fmt.Sprintf("%s commit bla\n", oid), + expectedErr: fmt.Errorf("parse object size: %w", &strconv.NumError{ + Func: "ParseInt", + Num: "bla", + Err: strconv.ErrSyntax, + }), + }, + } { t.Run(tc.desc, func(t *testing.T) { reader := bufio.NewReader(strings.NewReader(tc.input)) - _, err := ParseObjectInfo(reader) - require.Error(t, err) + _, err := ParseObjectInfo(gittest.DefaultObjectHash, reader) + require.Equal(t, tc.expectedErr, err) }) } } func TestObjectInfoReader(t *testing.T) { - ctx := testhelper.Context(t) + t.Parallel() - cfg, repoProto, repoPath := testcfg.BuildWithRepo(t) + ctx := testhelper.Context(t) + cfg := testcfg.Build(t) + repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) + + commitID := gittest.WriteCommit(t, cfg, repoPath, + gittest.WithBranch("main"), + gittest.WithMessage("commit message"), + gittest.WithTreeEntries(gittest.TreeEntry{Path: "README", Mode: "100644", Content: "something"}), + ) + gittest.WriteTag(t, cfg, repoPath, "v1.1.1", commitID.Revision(), gittest.WriteTagConfig{ + Message: "annotated tag", + }) oiByRevision := make(map[string]*ObjectInfo) for _, revision := range []string{ - "refs/heads/master", - "refs/heads/master^{tree}", - "refs/heads/master:README", + "refs/heads/main", + "refs/heads/main^{tree}", + "refs/heads/main:README", "refs/tags/v1.1.1", } { revParseOutput := gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", revision) - objectID, err := git.ObjectHashSHA1.FromHex(text.ChompBytes(revParseOutput)) + objectID, err := gittest.DefaultObjectHash.FromHex(text.ChompBytes(revParseOutput)) require.NoError(t, err) objectType := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "cat-file", "-t", revision)) @@ -113,23 +148,23 @@ func TestObjectInfoReader(t *testing.T) { }{ { desc: "commit by ref", - revision: "refs/heads/master", - expectedInfo: oiByRevision["refs/heads/master"], + revision: "refs/heads/main", + expectedInfo: oiByRevision["refs/heads/main"], }, { desc: "commit by ID", - revision: oiByRevision["refs/heads/master"].Oid.Revision(), - expectedInfo: oiByRevision["refs/heads/master"], + revision: oiByRevision["refs/heads/main"].Oid.Revision(), + expectedInfo: oiByRevision["refs/heads/main"], }, { desc: "tree", - revision: oiByRevision["refs/heads/master^{tree}"].Oid.Revision(), - expectedInfo: oiByRevision["refs/heads/master^{tree}"], + revision: oiByRevision["refs/heads/main^{tree}"].Oid.Revision(), + expectedInfo: oiByRevision["refs/heads/main^{tree}"], }, { desc: "blob", - revision: oiByRevision["refs/heads/master:README"].Oid.Revision(), - expectedInfo: oiByRevision["refs/heads/master:README"], + revision: oiByRevision["refs/heads/main:README"].Oid.Revision(), + expectedInfo: oiByRevision["refs/heads/main:README"], }, { desc: "tag", @@ -162,7 +197,7 @@ func TestObjectInfoReader(t *testing.T) { // Verify that we do another request no matter whether the previous call // succeeded or failed. - _, err = reader.Info(ctx, "refs/heads/master") + _, err = reader.Info(ctx, "refs/heads/main") require.NoError(t, err) require.Equal(t, float64(expectedRequests+1), testutil.ToFloat64(counter.WithLabelValues("info"))) @@ -171,9 +206,11 @@ func TestObjectInfoReader(t *testing.T) { } func TestObjectInfoReader_queue(t *testing.T) { - ctx := testhelper.Context(t) + t.Parallel() - cfg, repoProto, repoPath := testcfg.BuildWithRepo(t) + ctx := testhelper.Context(t) + cfg := testcfg.Build(t) + repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) blobOID := gittest.WriteBlob(t, cfg, repoPath, []byte("foobar")) blobInfo := ObjectInfo{ @@ -182,11 +219,16 @@ func TestObjectInfoReader_queue(t *testing.T) { Size: int64(len("foobar")), } - commitOID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents("master")) + commitOID := gittest.WriteCommit(t, cfg, repoPath) commitInfo := ObjectInfo{ Oid: commitOID, Type: "commit", - Size: 225, + Size: func() int64 { + if gittest.ObjectHashIsSHA256() { + return 201 + } + return 177 + }(), } t.Run("read single info", func(t *testing.T) { diff --git a/internal/git/catfile/object_reader.go b/internal/git/catfile/object_reader.go index 516050301..243e70865 100644 --- a/internal/git/catfile/object_reader.go +++ b/internal/git/catfile/object_reader.go @@ -73,10 +73,16 @@ func newObjectReader( return nil, err } + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return nil, fmt.Errorf("detecting object hash: %w", err) + } + objectReader := &objectReader{ cmd: batchCmd, counter: counter, queue: requestQueue{ + objectHash: objectHash, isObjectQueue: true, stdout: bufio.NewReader(batchCmd), stdin: bufio.NewWriter(batchCmd), diff --git a/internal/git/catfile/object_reader_test.go b/internal/git/catfile/object_reader_test.go index 28da876f6..9aade3b47 100644 --- a/internal/git/catfile/object_reader_test.go +++ b/internal/git/catfile/object_reader_test.go @@ -1,5 +1,3 @@ -//go:build !gitaly_test_sha256 - package catfile import ( @@ -22,17 +20,25 @@ import ( func TestObjectReader_reader(t *testing.T) { ctx := testhelper.Context(t) - cfg, repoProto, repoPath := testcfg.BuildWithRepo(t) + cfg := testcfg.Build(t) + repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) - commitID, err := git.ObjectHashSHA1.FromHex(text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", "refs/heads/master"))) - require.NoError(t, err) - commitContents := gittest.Exec(t, cfg, "-C", repoPath, "cat-file", "-p", "refs/heads/master") + commitID := gittest.WriteCommit(t, cfg, repoPath, + gittest.WithBranch("main"), + gittest.WithMessage("commit message"), + gittest.WithTreeEntries(gittest.TreeEntry{Path: "README", Mode: "100644", Content: "something"}), + ) + gittest.WriteTag(t, cfg, repoPath, "v1.1.1", commitID.Revision(), gittest.WriteTagConfig{ + Message: "annotated tag", + }) + + commitContents := gittest.Exec(t, cfg, "-C", repoPath, "cat-file", "-p", commitID.String()) t.Run("read existing object by ref", func(t *testing.T) { reader, err := newObjectReader(ctx, newRepoExecutor(t, cfg, repoProto), nil) require.NoError(t, err) - object, err := reader.Object(ctx, "refs/heads/master") + object, err := reader.Object(ctx, "refs/heads/main") require.NoError(t, err) data, err := io.ReadAll(object) @@ -49,8 +55,7 @@ func TestObjectReader_reader(t *testing.T) { data, err := io.ReadAll(object) require.NoError(t, err) - - require.Contains(t, string(data), "Merge branch 'cherry-pick-ce369011' into 'master'\n") + require.Equal(t, data, commitContents) }) t.Run("read missing ref", func(t *testing.T) { @@ -104,9 +109,9 @@ func TestObjectReader_reader(t *testing.T) { require.NoError(t, err) for objectType, revision := range map[string]git.Revision{ - "commit": "refs/heads/master", - "tree": "refs/heads/master^{tree}", - "blob": "refs/heads/master:README", + "commit": "refs/heads/main", + "tree": "refs/heads/main^{tree}", + "blob": "refs/heads/main:README", "tag": "refs/tags/v1.1.1", } { require.Equal(t, float64(0), testutil.ToFloat64(counter.WithLabelValues(objectType))) @@ -125,7 +130,8 @@ func TestObjectReader_reader(t *testing.T) { func TestObjectReader_queue(t *testing.T) { ctx := testhelper.Context(t) - cfg, repoProto, repoPath := testcfg.BuildWithRepo(t) + cfg := testcfg.Build(t) + repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) foobarBlob := gittest.WriteBlob(t, cfg, repoPath, []byte("foobar")) barfooBlob := gittest.WriteBlob(t, cfg, repoPath, []byte("barfoo")) @@ -420,9 +426,11 @@ func TestObjectReader_queue(t *testing.T) { } func TestObjectReader_replaceRefs(t *testing.T) { - cfg, repoProto, repoPath := testcfg.BuildWithRepo(t) ctx := testhelper.Context(t) + cfg := testcfg.Build(t) + repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) + originalOID := gittest.WriteBlob(t, cfg, repoPath, []byte("original")) replacedOID := gittest.WriteBlob(t, cfg, repoPath, []byte("replaced")) diff --git a/internal/git/catfile/parser_test.go b/internal/git/catfile/parser_test.go index 9e8746252..59afbb127 100644 --- a/internal/git/catfile/parser_test.go +++ b/internal/git/catfile/parser_test.go @@ -1,23 +1,25 @@ -//go:build !gitaly_test_sha256 - package catfile import ( "bytes" + "fmt" "strings" "testing" "time" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git" + "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" "google.golang.org/protobuf/types/known/timestamppb" ) func TestParser_ParseCommit(t *testing.T) { + t.Parallel() + info := &ObjectInfo{ - Oid: "a984dfa4dee018c6d5f5f57ffec0d0e22763df16", + Oid: gittest.DefaultObjectHash.EmptyTreeOID, Type: "commit", } @@ -28,7 +30,7 @@ func TestParser_ParseCommit(t *testing.T) { // Once a repository contains a pathological object it can be hard to get // rid of it. Because of this I think it's nicer to ignore such objects // than to throw hard errors. - testCases := []struct { + for _, tc := range []struct { desc string in string out *gitalypb.GitCommit @@ -119,9 +121,7 @@ fF3T79iV8paT4/OfX8Ygg= }, }, }, - } - - for _, tc := range testCases { + } { t.Run(tc.desc, func(t *testing.T) { info.Size = int64(len(tc.in)) out, err := NewParser().ParseCommit(newStaticObject(tc.in, "commit", info.Oid)) @@ -132,6 +132,8 @@ fF3T79iV8paT4/OfX8Ygg= } func TestParseCommitAuthor(t *testing.T) { + t.Parallel() + for _, tc := range []struct { desc string author string @@ -177,6 +179,10 @@ func TestParseCommitAuthor(t *testing.T) { } func TestParser_ParseTag(t *testing.T) { + t.Parallel() + + oid := gittest.DefaultObjectHash.EmptyTreeOID.String() + for _, tc := range []struct { desc string oid git.ObjectID @@ -186,7 +192,7 @@ func TestParser_ParseTag(t *testing.T) { }{ { desc: "tag without a message", - contents: "object c92faf3e0a557270141be67f206d7cdb99bfc3a2\ntype commit\ntag v2.6.16.28\ntagger Adrian Bunk <bunk@stusta.de> 1156539089 +0200", + contents: fmt.Sprintf("object %s\ntype commit\ntag v2.6.16.28\ntagger Adrian Bunk <bunk@stusta.de> 1156539089 +0200", oid), oid: "1234", expectedTag: &gitalypb.Tag{ Id: "1234", @@ -201,13 +207,13 @@ func TestParser_ParseTag(t *testing.T) { }, }, expectedTagged: taggedObject{ - objectID: "c92faf3e0a557270141be67f206d7cdb99bfc3a2", + objectID: oid, objectType: "commit", }, }, { desc: "tag with message", - contents: "object c92faf3e0a557270141be67f206d7cdb99bfc3a2\ntype commit\ntag v2.6.16.28\ntagger Adrian Bunk <bunk@stusta.de> 1156539089 +0200\n\nmessage", + contents: fmt.Sprintf("object %s\ntype commit\ntag v2.6.16.28\ntagger Adrian Bunk <bunk@stusta.de> 1156539089 +0200\n\nmessage", oid), oid: "1234", expectedTag: &gitalypb.Tag{ Id: "1234", @@ -224,14 +230,14 @@ func TestParser_ParseTag(t *testing.T) { }, }, expectedTagged: taggedObject{ - objectID: "c92faf3e0a557270141be67f206d7cdb99bfc3a2", + objectID: oid, objectType: "commit", }, }, { desc: "tag with empty message", oid: "1234", - contents: "object c92faf3e0a557270141be67f206d7cdb99bfc3a2\ntype commit\ntag v2.6.16.28\ntagger Adrian Bunk <bunk@stusta.de> 1156539089 +0200\n\n", + contents: fmt.Sprintf("object %s\ntype commit\ntag v2.6.16.28\ntagger Adrian Bunk <bunk@stusta.de> 1156539089 +0200\n\n", oid), expectedTag: &gitalypb.Tag{ Id: "1234", Name: []byte("v2.6.16.28"), @@ -246,14 +252,14 @@ func TestParser_ParseTag(t *testing.T) { }, }, expectedTagged: taggedObject{ - objectID: "c92faf3e0a557270141be67f206d7cdb99bfc3a2", + objectID: oid, objectType: "commit", }, }, { desc: "tag with message with empty line", oid: "1234", - contents: "object c92faf3e0a557270141be67f206d7cdb99bfc3a2\ntype commit\ntag v2.6.16.28\ntagger Adrian Bunk <bunk@stusta.de> 1156539089 +0200\n\nHello world\n\nThis is a message", + contents: fmt.Sprintf("object %s\ntype commit\ntag v2.6.16.28\ntagger Adrian Bunk <bunk@stusta.de> 1156539089 +0200\n\nHello world\n\nThis is a message", oid), expectedTag: &gitalypb.Tag{ Id: "1234", Name: []byte("v2.6.16.28"), @@ -269,13 +275,13 @@ func TestParser_ParseTag(t *testing.T) { }, }, expectedTagged: taggedObject{ - objectID: "c92faf3e0a557270141be67f206d7cdb99bfc3a2", + objectID: oid, objectType: "commit", }, }, { desc: "tag with message with empty line and right side new line", - contents: "object c92faf3e0a557270141be67f206d7cdb99bfc3a2\ntype commit\ntag v2.6.16.28\ntagger Adrian Bunk <bunk@stusta.de> 1156539089 +0200\n\nHello world\n\nThis is a message\n\n", + contents: fmt.Sprintf("object %s\ntype commit\ntag v2.6.16.28\ntagger Adrian Bunk <bunk@stusta.de> 1156539089 +0200\n\nHello world\n\nThis is a message\n\n", oid), oid: "1234", expectedTag: &gitalypb.Tag{ Id: "1234", @@ -292,13 +298,13 @@ func TestParser_ParseTag(t *testing.T) { }, }, expectedTagged: taggedObject{ - objectID: "c92faf3e0a557270141be67f206d7cdb99bfc3a2", + objectID: oid, objectType: "commit", }, }, { desc: "tag with missing date and body", - contents: "object 422081655f743e03b01ee29a2eaf26aab0ee7eda\ntype commit\ntag syslinux-3.11-pre6\ntagger hpa <hpa>\n", + contents: fmt.Sprintf("object %s\ntype commit\ntag syslinux-3.11-pre6\ntagger hpa <hpa>\n", oid), oid: "1234", expectedTag: &gitalypb.Tag{ Id: "1234", @@ -309,14 +315,14 @@ func TestParser_ParseTag(t *testing.T) { }, }, expectedTagged: taggedObject{ - objectID: "422081655f743e03b01ee29a2eaf26aab0ee7eda", + objectID: oid, objectType: "commit", }, }, { desc: "tag signed with SSH", oid: "1234", - contents: `object c92faf3e0a557270141be67f206d7cdb99bfc3a2 + contents: fmt.Sprintf(`object %s type commit tag v2.6.16.28 tagger Adrian Bunk <bunk@stusta.de> 1156539089 +0200 @@ -327,7 +333,7 @@ U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgtc+Qk8jhMwVZk/jFEFCM16LNQb 30q5kK30bbetfjyTMAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5 AAAAQLSyv010gOFwIs9QTtDvlfIEWiAw2iQL/T9usGcxHXn/W5l0cOFCd7O+WaMDg0t0nW fF3T79iV8paT4/OfX8Ygg= ------END SSH SIGNATURE-----`, +-----END SSH SIGNATURE-----`, oid), expectedTag: &gitalypb.Tag{ Id: "1234", Name: []byte("v2.6.16.28"), @@ -350,7 +356,7 @@ fF3T79iV8paT4/OfX8Ygg= SignatureType: gitalypb.SignatureType_SSH, }, expectedTagged: taggedObject{ - objectID: "c92faf3e0a557270141be67f206d7cdb99bfc3a2", + objectID: oid, objectType: "commit", }, }, diff --git a/internal/git/catfile/request_queue.go b/internal/git/catfile/request_queue.go index 08da8b048..232d731b1 100644 --- a/internal/git/catfile/request_queue.go +++ b/internal/git/catfile/request_queue.go @@ -21,6 +21,10 @@ const ( ) type requestQueue struct { + // objectHash is the object hash used by the repository the request queue has been + // spawned for. + objectHash git.ObjectHash + // outstandingRequests is the number of requests which have been queued up. Gets incremented // on request, and decremented when starting to read an object (not when that object has // been fully consumed). @@ -212,5 +216,5 @@ func (q *requestQueue) readInfo() (*ObjectInfo, error) { return nil, fmt.Errorf("concurrent read on request queue") } - return ParseObjectInfo(q.stdout) + return ParseObjectInfo(q.objectHash, q.stdout) } diff --git a/internal/git/catfile/request_queue_test.go b/internal/git/catfile/request_queue_test.go index 78952829d..9dede16aa 100644 --- a/internal/git/catfile/request_queue_test.go +++ b/internal/git/catfile/request_queue_test.go @@ -1,5 +1,3 @@ -//go:build !gitaly_test_sha256 - package catfile import ( @@ -7,6 +5,7 @@ import ( "fmt" "io" "os" + "strings" "testing" "github.com/stretchr/testify/require" @@ -17,8 +16,12 @@ import ( ) func TestRequestQueue_ReadObject(t *testing.T) { + t.Parallel() + ctx := testhelper.Context(t) + oid := git.ObjectID(strings.Repeat("1", gittest.DefaultObjectHash.EncodedLen())) + t.Run("ReadInfo on ReadObject queue", func(t *testing.T) { _, queue := newInterceptedQueue(ctx, t, "#!/bin/sh\nread\n") @@ -47,9 +50,9 @@ func TestRequestQueue_ReadObject(t *testing.T) { }) t.Run("read with unconsumed object", func(t *testing.T) { - _, queue := newInterceptedQueue(ctx, t, `#!/bin/sh - echo "1111111111111111111111111111111111111111 commit 464" - `) + _, queue := newInterceptedQueue(ctx, t, fmt.Sprintf(`#!/bin/sh + echo "%s commit 464" + `, oid)) // We queue two revisions... require.NoError(t, queue.RequestRevision("foo")) @@ -95,9 +98,9 @@ func TestRequestQueue_ReadObject(t *testing.T) { }) t.Run("read with missing object", func(t *testing.T) { - _, queue := newInterceptedQueue(ctx, t, `#!/bin/sh - echo "1111111111111111111111111111111111111111 missing" - `) + _, queue := newInterceptedQueue(ctx, t, fmt.Sprintf(`#!/bin/sh + echo "%s missing" + `, oid)) require.NoError(t, queue.RequestRevision("foo")) @@ -110,10 +113,10 @@ func TestRequestQueue_ReadObject(t *testing.T) { }) t.Run("read single object", func(t *testing.T) { - _, queue := newInterceptedQueue(ctx, t, `#!/bin/sh - echo "1111111111111111111111111111111111111111 blob 10" + _, queue := newInterceptedQueue(ctx, t, fmt.Sprintf(`#!/bin/sh + echo "%s blob 10" echo "1234567890" - `) + `, oid)) require.NoError(t, queue.RequestRevision("foo")) require.True(t, queue.isDirty()) @@ -121,7 +124,7 @@ func TestRequestQueue_ReadObject(t *testing.T) { object, err := queue.ReadObject() require.NoError(t, err) require.Equal(t, ObjectInfo{ - Oid: "1111111111111111111111111111111111111111", + Oid: oid, Type: "blob", Size: 10, }, object.ObjectInfo) @@ -134,12 +137,14 @@ func TestRequestQueue_ReadObject(t *testing.T) { }) t.Run("read multiple objects", func(t *testing.T) { - _, queue := newInterceptedQueue(ctx, t, `#!/bin/sh - echo "1111111111111111111111111111111111111111 blob 10" + secondOID := git.ObjectID(strings.Repeat("2", gittest.DefaultObjectHash.EncodedLen())) + + _, queue := newInterceptedQueue(ctx, t, fmt.Sprintf(`#!/bin/sh + echo "%s blob 10" echo "1234567890" - echo "2222222222222222222222222222222222222222 commit 10" + echo "%s commit 10" echo "0987654321" - `) + `, oid, secondOID)) require.NoError(t, queue.RequestRevision("foo")) require.NoError(t, queue.RequestRevision("foo")) @@ -151,7 +156,7 @@ func TestRequestQueue_ReadObject(t *testing.T) { }{ { info: ObjectInfo{ - Oid: "1111111111111111111111111111111111111111", + Oid: oid, Type: "blob", Size: 10, }, @@ -159,7 +164,7 @@ func TestRequestQueue_ReadObject(t *testing.T) { }, { info: ObjectInfo{ - Oid: "2222222222222222222222222222222222222222", + Oid: secondOID, Type: "commit", Size: 10, }, @@ -181,10 +186,10 @@ func TestRequestQueue_ReadObject(t *testing.T) { }) t.Run("truncated object", func(t *testing.T) { - _, queue := newInterceptedQueue(ctx, t, `#!/bin/sh - echo "1111111111111111111111111111111111111111 blob 10" + _, queue := newInterceptedQueue(ctx, t, fmt.Sprintf(`#!/bin/sh + echo "%s blob 10" printf "123" - `) + `, oid)) require.NoError(t, queue.RequestRevision("foo")) require.True(t, queue.isDirty()) @@ -192,7 +197,7 @@ func TestRequestQueue_ReadObject(t *testing.T) { object, err := queue.ReadObject() require.NoError(t, err) require.Equal(t, ObjectInfo{ - Oid: "1111111111111111111111111111111111111111", + Oid: oid, Type: "blob", Size: 10, }, object.ObjectInfo) @@ -215,8 +220,12 @@ func TestRequestQueue_ReadObject(t *testing.T) { } func TestRequestQueue_RequestRevision(t *testing.T) { + t.Parallel() + ctx := testhelper.Context(t) + oid := git.ObjectID(strings.Repeat("1", gittest.DefaultObjectHash.EncodedLen())) + requireRevision := func(t *testing.T, queue *requestQueue, rev git.Revision) { object, err := queue.ReadObject() require.NoError(t, err) @@ -242,11 +251,11 @@ func TestRequestQueue_RequestRevision(t *testing.T) { }) t.Run("single request", func(t *testing.T) { - _, queue := newInterceptedQueue(ctx, t, `#!/bin/sh + _, queue := newInterceptedQueue(ctx, t, fmt.Sprintf(`#!/bin/sh read revision - echo "1111111111111111111111111111111111111111 blob ${#revision}" + echo "%s blob ${#revision}" echo "${revision}" - `) + `, oid)) require.NoError(t, queue.RequestRevision("foo")) require.NoError(t, queue.Flush()) @@ -255,13 +264,13 @@ func TestRequestQueue_RequestRevision(t *testing.T) { }) t.Run("multiple request", func(t *testing.T) { - _, queue := newInterceptedQueue(ctx, t, `#!/bin/sh + _, queue := newInterceptedQueue(ctx, t, fmt.Sprintf(`#!/bin/sh while read revision do - echo "1111111111111111111111111111111111111111 blob ${#revision}" + echo "%s blob ${#revision}" echo "${revision}" done - `) + `, oid)) require.NoError(t, queue.RequestRevision("foo")) require.NoError(t, queue.RequestRevision("bar")) @@ -276,7 +285,7 @@ func TestRequestQueue_RequestRevision(t *testing.T) { }) t.Run("multiple request with intermediate flushing", func(t *testing.T) { - _, queue := newInterceptedQueue(ctx, t, `#!/bin/sh + _, queue := newInterceptedQueue(ctx, t, fmt.Sprintf(`#!/bin/sh while read revision do read flush @@ -286,10 +295,10 @@ func TestRequestQueue_RequestRevision(t *testing.T) { exit 1 fi - echo "1111111111111111111111111111111111111111 blob ${#revision}" + echo "%s blob ${#revision}" echo "${revision}" done - `) + `, oid)) for _, revision := range []git.Revision{ "foo", @@ -305,7 +314,8 @@ func TestRequestQueue_RequestRevision(t *testing.T) { } func newInterceptedQueue(ctx context.Context, t *testing.T, script string) (ObjectReader, *requestQueue) { - cfg, repo, _ := testcfg.BuildWithRepo(t) + cfg := testcfg.Build(t) + repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0]) commandFactory := gittest.NewInterceptingCommandFactory(ctx, t, cfg, func(execEnv git.ExecutionEnvironment) string { return script diff --git a/internal/git/catfile/tag_test.go b/internal/git/catfile/tag_test.go index bcb0ef05b..efc54e20c 100644 --- a/internal/git/catfile/tag_test.go +++ b/internal/git/catfile/tag_test.go @@ -1,10 +1,7 @@ -//go:build !gitaly_test_sha256 - package catfile import ( "fmt" - "path/filepath" "strings" "testing" @@ -19,40 +16,33 @@ import ( func TestGetTag(t *testing.T) { ctx := testhelper.Context(t) - cfg, objectReader, testRepo := setupObjectReader(t, ctx) - - testRepoPath := filepath.Join(cfg.Storages[0].Path, testRepo.RelativePath) + cfg, objectReader, _, repoPath := setupObjectReader(t, ctx) + commitID := gittest.WriteCommit(t, cfg, repoPath) - testCases := []struct { + for _, tc := range []struct { tagName string - rev git.Revision message string }{ { tagName: fmt.Sprintf("%s-v1.0.2", t.Name()), - rev: "master^^^^", message: strings.Repeat("a", helper.MaxCommitOrTagMessageSize+1) + "\n", }, { tagName: fmt.Sprintf("%s-v1.0.0", t.Name()), - rev: "master^^^", message: "Prod Release v1.0.0\n", }, { tagName: fmt.Sprintf("%s-v1.0.1", t.Name()), - rev: "master^^", message: strings.Repeat("a", helper.MaxCommitOrTagMessageSize+1) + "\n", }, - } - - for _, testCase := range testCases { - t.Run(testCase.tagName, func(t *testing.T) { - tagID := gittest.WriteTag(t, cfg, testRepoPath, testCase.tagName, testCase.rev, gittest.WriteTagConfig{Message: testCase.message}) + } { + t.Run(tc.tagName, func(t *testing.T) { + tagID := gittest.WriteTag(t, cfg, repoPath, tc.tagName, commitID.Revision(), gittest.WriteTagConfig{Message: tc.message}) - tag, err := GetTag(ctx, objectReader, git.Revision(tagID), testCase.tagName) + tag, err := GetTag(ctx, objectReader, git.Revision(tagID), tc.tagName) require.NoError(t, err) - require.Equal(t, testCase.message, string(tag.Message)) - require.Equal(t, testCase.tagName, string(tag.GetName())) + require.Equal(t, tc.message, string(tag.Message)) + require.Equal(t, tc.tagName, string(tag.GetName())) }) } } diff --git a/internal/git/catfile/testhelper_test.go b/internal/git/catfile/testhelper_test.go index e222a52c0..dc8b7994f 100644 --- a/internal/git/catfile/testhelper_test.go +++ b/internal/git/catfile/testhelper_test.go @@ -1,5 +1,3 @@ -//go:build !gitaly_test_sha256 - package catfile import ( @@ -57,10 +55,11 @@ func (e *repoExecutor) ObjectHash(ctx context.Context) (git.ObjectHash, error) { return gittest.DefaultObjectHash, nil } -func setupObjectReader(t *testing.T, ctx context.Context) (config.Cfg, ObjectReader, *gitalypb.Repository) { +func setupObjectReader(t *testing.T, ctx context.Context) (config.Cfg, ObjectReader, *gitalypb.Repository, string) { t.Helper() - cfg, repo, _ := testcfg.BuildWithRepo(t) + cfg := testcfg.Build(t) + repo, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) repoExecutor := newRepoExecutor(t, cfg, repo) cache := newCache(1*time.Hour, 1000, helper.NewTimerTicker(defaultEvictionInterval)) @@ -70,7 +69,7 @@ func setupObjectReader(t *testing.T, ctx context.Context) (config.Cfg, ObjectRea require.NoError(t, err) t.Cleanup(cancel) - return cfg, objectReader, repo + return cfg, objectReader, repo, repoPath } type staticObject struct { diff --git a/internal/git/gitpipe/catfile_info.go b/internal/git/gitpipe/catfile_info.go index cefd81422..8b4792378 100644 --- a/internal/git/gitpipe/catfile_info.go +++ b/internal/git/gitpipe/catfile_info.go @@ -204,7 +204,7 @@ func CatfileInfoAllObjects( reader := bufio.NewReader(cmd) for { - objectInfo, err := catfile.ParseObjectInfo(reader) + objectInfo, err := catfile.ParseObjectInfo(git.ObjectHashSHA1, reader) if err != nil { if errors.Is(err, io.EOF) { break diff --git a/internal/gitaly/service/commit/find_commit_test.go b/internal/gitaly/service/commit/find_commit_test.go index 583c5f755..3ecfd38e0 100644 --- a/internal/gitaly/service/commit/find_commit_test.go +++ b/internal/gitaly/service/commit/find_commit_test.go @@ -13,9 +13,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" - "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/timestamppb" ) @@ -246,26 +244,48 @@ func TestFailedFindCommitRequest(t *testing.T) { invalidRepo := &gitalypb.Repository{StorageName: "fake", RelativePath: "path"} - testCases := []struct { - description string + for _, tc := range []struct { + desc string revision []byte repo *gitalypb.Repository + expectedErr error }{ - {repo: invalidRepo, revision: []byte("master"), description: "Invalid repo"}, - {repo: repo, revision: []byte(""), description: "Empty revision"}, - {repo: repo, revision: []byte("-master"), description: "Invalid revision"}, - {repo: repo, revision: []byte("mas:ter"), description: "Invalid revision"}, - } - - for _, testCase := range testCases { - t.Run(testCase.description, func(t *testing.T) { + { + desc: "Invalid repo", + repo: invalidRepo, + revision: []byte("master"), + expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"fake\"", + "repo scoped: invalid Repository", + )), + }, + { + desc: "Empty revision", + repo: repo, + revision: []byte(""), + expectedErr: helper.ErrInvalidArgumentf("empty revision"), + }, + { + desc: "Invalid revision", + repo: repo, + revision: []byte("-master"), + expectedErr: helper.ErrInvalidArgumentf("revision can't start with '-'"), + }, + { + desc: "Invalid revision", + repo: repo, + revision: []byte("mas:ter"), + expectedErr: helper.ErrInvalidArgumentf("revision can't contain ':'"), + }, + } { + t.Run(tc.desc, func(t *testing.T) { request := &gitalypb.FindCommitRequest{ - Repository: testCase.repo, - Revision: testCase.revision, + Repository: tc.repo, + Revision: tc.revision, } _, err := client.FindCommit(ctx, request) - require.Equal(t, codes.InvalidArgument, status.Code(err), "default lookup should fail") + testhelper.RequireGrpcError(t, tc.expectedErr, err) }) } } diff --git a/internal/gitaly/service/commit/last_commit_for_path_test.go b/internal/gitaly/service/commit/last_commit_for_path_test.go index 4fccef5f3..84a7297e5 100644 --- a/internal/gitaly/service/commit/last_commit_for_path_test.go +++ b/internal/gitaly/service/commit/last_commit_for_path_test.go @@ -8,9 +8,9 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" - "google.golang.org/grpc/codes" ) func TestSuccessfulLastCommitForPathRequest(t *testing.T) { @@ -81,39 +81,52 @@ func TestFailedLastCommitForPathRequest(t *testing.T) { invalidRepo := &gitalypb.Repository{StorageName: "fake", RelativePath: "path"} - testCases := []struct { - desc string - request *gitalypb.LastCommitForPathRequest - code codes.Code + for _, tc := range []struct { + desc string + request *gitalypb.LastCommitForPathRequest + expectedErr error }{ { - desc: "Invalid repository", - request: &gitalypb.LastCommitForPathRequest{Repository: invalidRepo}, - code: codes.InvalidArgument, + desc: "Invalid repository", + request: &gitalypb.LastCommitForPathRequest{ + Repository: invalidRepo, + Revision: []byte("some-branch"), + }, + expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"fake\"", + "repo scoped: invalid Repository", + )), }, { - desc: "Repository is nil", - request: &gitalypb.LastCommitForPathRequest{Revision: []byte("some-branch")}, - code: codes.InvalidArgument, + desc: "Repository is nil", + request: &gitalypb.LastCommitForPathRequest{ + Revision: []byte("some-branch"), + }, + expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"\"", + "repo scoped: empty Repository", + )), }, { - desc: "Revision is missing", - request: &gitalypb.LastCommitForPathRequest{Repository: repo, Path: []byte("foo/bar")}, - code: codes.InvalidArgument, + desc: "Revision is missing", + request: &gitalypb.LastCommitForPathRequest{ + Repository: repo, Path: []byte("foo/bar"), + }, + expectedErr: helper.ErrInvalidArgumentf("empty revision"), }, { desc: "Revision is invalid", request: &gitalypb.LastCommitForPathRequest{ - Repository: repo, Path: []byte("foo/bar"), Revision: []byte("--output=/meow"), + Repository: repo, + Path: []byte("foo/bar"), + Revision: []byte("--output=/meow"), }, - code: codes.InvalidArgument, + expectedErr: helper.ErrInvalidArgumentf("revision can't start with '-'"), }, - } - - for _, testCase := range testCases { - t.Run(testCase.desc, func(t *testing.T) { - _, err := client.LastCommitForPath(ctx, testCase.request) - testhelper.RequireGrpcCode(t, err, testCase.code) + } { + t.Run(tc.desc, func(t *testing.T) { + _, err := client.LastCommitForPath(ctx, tc.request) + testhelper.RequireGrpcError(t, tc.expectedErr, err) }) } } diff --git a/internal/gitaly/service/commit/stats_test.go b/internal/gitaly/service/commit/stats_test.go index 747f0349b..7f506f65d 100644 --- a/internal/gitaly/service/commit/stats_test.go +++ b/internal/gitaly/service/commit/stats_test.go @@ -3,13 +3,15 @@ package commit import ( + "fmt" + "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" - "google.golang.org/grpc/codes" ) func TestCommitStatsSuccess(t *testing.T) { @@ -80,44 +82,61 @@ func TestCommitStatsFailure(t *testing.T) { t.Parallel() ctx := testhelper.Context(t) - _, repo, _, client := setupCommitServiceWithRepo(ctx, t) + cfg, repo, _, client := setupCommitServiceWithRepo(ctx, t) - tests := []struct { - desc string - repo *gitalypb.Repository - revision []byte - err codes.Code + for _, tc := range []struct { + desc string + request *gitalypb.CommitStatsRequest + expectedErr error }{ { - desc: "repo not found", - repo: &gitalypb.Repository{StorageName: repo.GetStorageName(), RelativePath: "bar.git"}, - revision: []byte("test-do-not-touch"), - err: codes.NotFound, + desc: "repo not found", + request: &gitalypb.CommitStatsRequest{ + Repository: &gitalypb.Repository{ + StorageName: repo.GetStorageName(), + RelativePath: "bar.git", + }, + Revision: []byte("test-do-not-touch"), + }, + expectedErr: helper.ErrNotFoundf(gitalyOrPraefect( + fmt.Sprintf("GetRepoPath: not a git repository: %q", filepath.Join(cfg.Storages[0].Path, "bar.git")), + "accessor call: route repository accessor: consistent storages: repository \"default\"/\"bar.git\" not found", + )), }, { - desc: "storage not found", - repo: &gitalypb.Repository{StorageName: "foo", RelativePath: "bar.git"}, - revision: []byte("test-do-not-touch"), - err: codes.InvalidArgument, + desc: "storage not found", + request: &gitalypb.CommitStatsRequest{ + Repository: &gitalypb.Repository{ + StorageName: "foo", + RelativePath: "bar.git", + }, + Revision: []byte("test-do-not-touch"), + }, + expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"foo\"", + "repo scoped: invalid Repository", + )), }, { - desc: "ref not found", - repo: repo, - revision: []byte("non/existing"), - err: codes.Internal, + desc: "ref not found", + request: &gitalypb.CommitStatsRequest{ + Repository: repo, + Revision: []byte("non/existing"), + }, + expectedErr: helper.ErrInternalf("object not found"), }, { - desc: "invalid revision", - repo: repo, - revision: []byte("--outpu=/meow"), - err: codes.InvalidArgument, + desc: "invalid revision", + request: &gitalypb.CommitStatsRequest{ + Repository: repo, + Revision: []byte("--outpu=/meow"), + }, + expectedErr: helper.ErrInvalidArgumentf("revision can't start with '-'"), }, - } - - for _, tc := range tests { + } { t.Run(tc.desc, func(t *testing.T) { - _, err := client.CommitStats(ctx, &gitalypb.CommitStatsRequest{Repository: tc.repo, Revision: tc.revision}) - testhelper.RequireGrpcCode(t, err, tc.err) + _, err := client.CommitStats(ctx, tc.request) + testhelper.RequireGrpcError(t, tc.expectedErr, err) }) } } diff --git a/internal/gitaly/service/commit/testhelper_test.go b/internal/gitaly/service/commit/testhelper_test.go index 9a33d8645..7d6734978 100644 --- a/internal/gitaly/service/commit/testhelper_test.go +++ b/internal/gitaly/service/commit/testhelper_test.go @@ -142,3 +142,10 @@ func getAllCommits(t testing.TB, getter func() (gitCommitsGetter, error)) []*git commits = append(commits, resp.GetCommits()...) } } + +func gitalyOrPraefect(gitalyMsg, praefectMsg string) string { + if testhelper.IsPraefectEnabled() { + return praefectMsg + } + return gitalyMsg +} diff --git a/internal/gitaly/service/commit/tree_entries_test.go b/internal/gitaly/service/commit/tree_entries_test.go index 659c36651..82be60290 100644 --- a/internal/gitaly/service/commit/tree_entries_test.go +++ b/internal/gitaly/service/commit/tree_entries_test.go @@ -4,7 +4,6 @@ package commit import ( "errors" - "fmt" "io" "strconv" "testing" @@ -12,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -649,21 +649,68 @@ func TestGetTreeEntries_validation(t *testing.T) { revision := []byte("d42783470dc29fde2cf459eb3199ee1d7e3f3a72") path := []byte("a/b/c") - rpcRequests := []*gitalypb.GetTreeEntriesRequest{ - {Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, Revision: revision, Path: path}, // Repository doesn't exist - {Repository: nil, Revision: revision, Path: path}, // Repository is nil - {Repository: repo, Revision: nil, Path: path}, // Revision is empty - {Repository: repo, Revision: revision}, // Path is empty - {Repository: repo, Revision: []byte("--output=/meow"), Path: path}, // Revision is invalid - } - - for _, rpcRequest := range rpcRequests { - t.Run(fmt.Sprintf("%v", rpcRequest), func(t *testing.T) { - c, err := client.GetTreeEntries(ctx, rpcRequest) + for _, tc := range []struct { + desc string + request *gitalypb.GetTreeEntriesRequest + expectedErr error + }{ + { + desc: "repository does not exist", + request: &gitalypb.GetTreeEntriesRequest{ + Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, + Revision: revision, + Path: path, + }, + expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"fake\"", + "repo scoped: invalid Repository", + )), + }, + { + desc: "repository is nil", + request: &gitalypb.GetTreeEntriesRequest{ + Repository: nil, + Revision: revision, + Path: path, + }, + expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"\"", + "repo scoped: empty Repository", + )), + }, + { + desc: "revision is empty", + request: &gitalypb.GetTreeEntriesRequest{ + Repository: repo, + Revision: nil, + Path: path, + }, + expectedErr: helper.ErrInvalidArgumentf("TreeEntry: empty revision"), + }, + { + desc: "path is empty", + request: &gitalypb.GetTreeEntriesRequest{ + Repository: repo, + Revision: revision, + }, + expectedErr: helper.ErrInvalidArgumentf("TreeEntry: empty Path"), + }, + { + desc: "revision is invalid", + request: &gitalypb.GetTreeEntriesRequest{ + Repository: repo, + Revision: []byte("--output=/meow"), + Path: path, + }, + expectedErr: helper.ErrInvalidArgumentf("TreeEntry: revision can't start with '-'"), + }, + } { + t.Run(tc.desc, func(t *testing.T) { + stream, err := client.GetTreeEntries(ctx, tc.request) require.NoError(t, err) - err = drainTreeEntriesResponse(c) - testhelper.RequireGrpcCode(t, err, codes.InvalidArgument) + err = drainTreeEntriesResponse(stream) + testhelper.RequireGrpcError(t, tc.expectedErr, err) }) } } diff --git a/internal/gitaly/service/commit/tree_entry_test.go b/internal/gitaly/service/commit/tree_entry_test.go index 1396e64c4..8d408fc48 100644 --- a/internal/gitaly/service/commit/tree_entry_test.go +++ b/internal/gitaly/service/commit/tree_entry_test.go @@ -9,9 +9,9 @@ import ( "testing" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" - "google.golang.org/grpc/codes" ) type treeEntry struct { @@ -160,75 +160,79 @@ func TestFailedTreeEntry(t *testing.T) { revision := []byte("d42783470dc29fde2cf459eb3199ee1d7e3f3a72") path := []byte("a/b/c") - testCases := []struct { - name string - req *gitalypb.TreeEntryRequest - expectedCode codes.Code + for _, tc := range []struct { + name string + req *gitalypb.TreeEntryRequest + expectedErr error }{ { - name: "Repository doesn't exist", - req: &gitalypb.TreeEntryRequest{Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, Revision: revision, Path: path}, - expectedCode: codes.InvalidArgument, + name: "Repository doesn't exist", + req: &gitalypb.TreeEntryRequest{Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, Revision: revision, Path: path}, + expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"fake\"", + "repo scoped: invalid Repository", + )), }, { - name: "Repository is nil", - req: &gitalypb.TreeEntryRequest{Repository: nil, Revision: revision, Path: path}, - expectedCode: codes.InvalidArgument, + name: "Repository is nil", + req: &gitalypb.TreeEntryRequest{Repository: nil, Revision: revision, Path: path}, + expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"\"", + "repo scoped: empty Repository", + )), }, { - name: "Revision is empty", - req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: nil, Path: path}, - expectedCode: codes.InvalidArgument, + name: "Revision is empty", + req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: nil, Path: path}, + expectedErr: helper.ErrInvalidArgumentf("TreeEntry: empty revision"), }, { - name: "Path is empty", - req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: revision}, - expectedCode: codes.InvalidArgument, + name: "Path is empty", + req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: revision}, + expectedErr: helper.ErrInvalidArgumentf("TreeEntry: empty Path"), }, { - name: "Revision is invalid", - req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("--output=/meow"), Path: path}, - expectedCode: codes.InvalidArgument, + name: "Revision is invalid", + req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("--output=/meow"), Path: path}, + expectedErr: helper.ErrInvalidArgumentf("TreeEntry: revision can't start with '-'"), }, { - name: "Limit is negative", - req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: revision, Path: path, Limit: -1}, - expectedCode: codes.InvalidArgument, + name: "Limit is negative", + req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: revision, Path: path, Limit: -1}, + expectedErr: helper.ErrInvalidArgumentf("TreeEntry: negative Limit"), }, { - name: "MaximumSize is negative", - req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: revision, Path: path, MaxSize: -1}, - expectedCode: codes.InvalidArgument, + name: "MaximumSize is negative", + req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: revision, Path: path, MaxSize: -1}, + expectedErr: helper.ErrInvalidArgumentf("TreeEntry: negative MaxSize"), }, { - name: "Object bigger than MaxSize", - req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("913c66a37b4a45b9769037c55c2d238bd0942d2e"), Path: []byte("MAINTENANCE.md"), MaxSize: 10}, - expectedCode: codes.FailedPrecondition, + name: "Object bigger than MaxSize", + req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("913c66a37b4a45b9769037c55c2d238bd0942d2e"), Path: []byte("MAINTENANCE.md"), MaxSize: 10}, + expectedErr: helper.ErrFailedPreconditionf("TreeEntry: object size (1367) is bigger than the maximum allowed size (10)"), }, { - name: "Path is outside of repository", - req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("913c66a37b4a45b9769037c55c2d238bd0942d2e"), Path: []byte("../bar/.gitkeep")}, // Git blows up on paths like this - expectedCode: codes.NotFound, + name: "Path is outside of repository", + req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("913c66a37b4a45b9769037c55c2d238bd0942d2e"), Path: []byte("../bar/.gitkeep")}, // Git blows up on paths like this + expectedErr: helper.ErrNotFoundf("not found: ../bar/.gitkeep"), }, { - name: "Missing file with space in path", - req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("deadfacedeadfacedeadfacedeadfacedeadface"), Path: []byte("with space/README.md")}, - expectedCode: codes.NotFound, + name: "Missing file with space in path", + req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("deadfacedeadfacedeadfacedeadfacedeadface"), Path: []byte("with space/README.md")}, + expectedErr: helper.ErrNotFoundf("not found: with space/README.md"), }, { - name: "Missing file", - req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("e63f41fe459e62e1228fcef60d7189127aeba95a"), Path: []byte("missing.rb")}, - expectedCode: codes.NotFound, + name: "Missing file", + req: &gitalypb.TreeEntryRequest{Repository: repo, Revision: []byte("e63f41fe459e62e1228fcef60d7189127aeba95a"), Path: []byte("missing.rb")}, + expectedErr: helper.ErrNotFoundf("not found: missing.rb"), }, - } - - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - c, err := client.TreeEntry(ctx, testCase.req) + } { + t.Run(tc.name, func(t *testing.T) { + c, err := client.TreeEntry(ctx, tc.req) require.NoError(t, err) err = drainTreeEntryResponse(c) - testhelper.RequireGrpcCode(t, err, testCase.expectedCode) + testhelper.RequireGrpcError(t, tc.expectedErr, err) }) } } diff --git a/internal/gitaly/service/ref/refs_test.go b/internal/gitaly/service/ref/refs_test.go index 404ea6af8..5e5101dcb 100644 --- a/internal/gitaly/service/ref/refs_test.go +++ b/internal/gitaly/service/ref/refs_test.go @@ -517,9 +517,13 @@ func TestEmptyFindLocalBranchesRequest(t *testing.T) { _, recvError = c.Recv() } - if helper.GrpcCode(recvError) != codes.InvalidArgument { - t.Fatal(recvError) - } + testhelper.RequireGrpcError(t, + helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"\"", + "repo scoped: empty Repository", + )), + recvError, + ) } func TestSuccessfulFindAllBranchesRequest(t *testing.T) { @@ -661,13 +665,18 @@ func TestSuccessfulFindAllBranchesRequestWithMergedBranches(t *testing.T) { func TestInvalidFindAllBranchesRequest(t *testing.T) { _, client := setupRefServiceWithoutRepo(t) - testCases := []struct { + for _, tc := range []struct { description string request *gitalypb.FindAllBranchesRequest + expectedErr error }{ { description: "Empty request", request: &gitalypb.FindAllBranchesRequest{}, + expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"\"", + "repo scoped: empty Repository", + )), }, { description: "Invalid repo", @@ -677,10 +686,12 @@ func TestInvalidFindAllBranchesRequest(t *testing.T) { RelativePath: "repo", }, }, + expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"fake\"", + "repo scoped: invalid Repository", + )), }, - } - - for _, tc := range testCases { + } { t.Run(tc.description, func(t *testing.T) { ctx := testhelper.Context(t) c, err := client.FindAllBranches(ctx, tc.request) @@ -691,7 +702,7 @@ func TestInvalidFindAllBranchesRequest(t *testing.T) { _, recvError = c.Recv() } - testhelper.RequireGrpcCode(t, recvError, codes.InvalidArgument) + testhelper.RequireGrpcError(t, tc.expectedErr, recvError) }) } } diff --git a/internal/gitaly/service/repository/archive_test.go b/internal/gitaly/service/repository/archive_test.go index 5f18f8679..ce90c525e 100644 --- a/internal/gitaly/service/repository/archive_test.go +++ b/internal/gitaly/service/repository/archive_test.go @@ -466,10 +466,15 @@ func TestGetArchive_environment(t *testing.T) { ctx := testhelper.Context(t) cfg := testcfg.Build(t) - gitCmdFactory := gittest.NewInterceptingCommandFactory(ctx, t, cfg, func(git.ExecutionEnvironment) string { - return `#!/bin/sh + // Intercept commands to git-archive(1) to print the environment. Note that we continue to + // execute any other Git commands so that the command factory behaves as expected. + gitCmdFactory := gittest.NewInterceptingCommandFactory(ctx, t, cfg, func(execEnv git.ExecutionEnvironment) string { + return fmt.Sprintf(`#!/bin/bash + if [[ ! "$@" =~ "archive" ]]; then + exec %q "$@" + fi env | grep -E '^GL_|CORRELATION|GITALY_' - ` + `, execEnv.BinaryPath) }) testcfg.BuildGitalyHooks(t, cfg) diff --git a/internal/gitaly/service/repository/raw_changes_test.go b/internal/gitaly/service/repository/raw_changes_test.go index eaf281bbf..b91d1ecdc 100644 --- a/internal/gitaly/service/repository/raw_changes_test.go +++ b/internal/gitaly/service/repository/raw_changes_test.go @@ -11,9 +11,9 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" - "google.golang.org/grpc/codes" ) func TestGetRawChanges(t *testing.T) { @@ -164,51 +164,50 @@ func TestGetRawChangesFailures(t *testing.T) { ctx := testhelper.Context(t) _, repo, _, client := setupRepositoryService(ctx, t) - testCases := []struct { - oldRev string - newRev string - code codes.Code - omitRepository bool + for _, tc := range []struct { + desc string + request *gitalypb.GetRawChangesRequest + expectedErr error }{ { - oldRev: "", - newRev: "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863", - code: codes.InvalidArgument, + desc: "missing from-revision", + request: &gitalypb.GetRawChangesRequest{ + Repository: repo, + FromRevision: "", + ToRevision: "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863", + }, + expectedErr: helper.ErrInvalidArgumentf("invalid 'from' revision: %q", ""), }, { - oldRev: "cfe32cf61b73a0d5e9f13e774abde7ff789b1660", - newRev: "913c66a37b4a45b9769037c55c2d238bd0942d2e", - code: codes.InvalidArgument, - omitRepository: true, + desc: "missing repository", + request: &gitalypb.GetRawChangesRequest{ + FromRevision: "cfe32cf61b73a0d5e9f13e774abde7ff789b1660", + ToRevision: "913c66a37b4a45b9769037c55c2d238bd0942d2e", + }, + expectedErr: helper.ErrInvalidArgumentf(gitalyOrPraefect( + "GetStorageByName: no such storage: \"\"", + "repo scoped: empty Repository", + )), }, { - // A Gitaly commit, unresolvable in gitlab-test - oldRev: "32800ed8206c0087f65e90a1a396b76d3c33f648", - newRev: "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863", - code: codes.InvalidArgument, + desc: "missing commit", + request: &gitalypb.GetRawChangesRequest{ + Repository: repo, + // A Gitaly commit, unresolvable in gitlab-test + FromRevision: "32800ed8206c0087f65e90a1a396b76d3c33f648", + ToRevision: "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863", + }, + expectedErr: helper.ErrInvalidArgumentf("invalid 'from' revision: %q", "32800ed8206c0087f65e90a1a396b76d3c33f648"), }, - } - - for _, tc := range testCases { - t.Run(fmt.Sprintf("old:%s,new:%s", tc.oldRev, tc.newRev), func(t *testing.T) { - req := &gitalypb.GetRawChangesRequest{ - Repository: repo, - FromRevision: tc.oldRev, - ToRevision: tc.newRev, - } - - if tc.omitRepository { - req.Repository = nil - } - - resp, err := client.GetRawChanges(ctx, req) + } { + t.Run(fmt.Sprintf(tc.desc), func(t *testing.T) { + stream, err := client.GetRawChanges(ctx, tc.request) require.NoError(t, err) for err == nil { - _, err = resp.Recv() + _, err = stream.Recv() } - - testhelper.RequireGrpcCode(t, err, tc.code) + testhelper.RequireGrpcError(t, tc.expectedErr, err) }) } } |