commit: Refactor `CheckObjectsExist()` tests to be more extensible

Tests for the `CheckObjectsExist()` are not quite extensible enough to
test various error conditions, most importantly in the context of
chunking. Refactor them.

2 files changed, 170 insertions, 76 deletions

diff --git a/internal/gitaly/service/commit/check_objects_exist_test.go b/internal/gitaly/service/commit/check_objects_exist_test.go
index 97fed524f..1be585c02 100644
--- a/internal/gitaly/service/commit/check_objects_exist_test.go
+++ b/internal/gitaly/service/commit/check_objects_exist_test.go
@@ -1,121 +1,214 @@
 package commit
 
 import (
+	"fmt"
 	"io"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
-	"google.golang.org/grpc/codes"
 )
 
 func TestCheckObjectsExist(t *testing.T) {
 	t.Parallel()
 
 	ctx := testhelper.Context(t)
-	cfg, repo, repoPath, client := setupCommitServiceWithRepo(ctx, t, true)
+	cfg, client := setupCommitService(ctx, t)
 
-	// write a few commitIDs we can use
-	commitID1 := gittest.WriteCommit(t, cfg, repoPath)
-	commitID2 := gittest.WriteCommit(t, cfg, repoPath)
-	commitID3 := gittest.WriteCommit(t, cfg, repoPath)
+	repo, repoPath := gittest.CreateRepository(ctx, t, cfg)
 
-	// remove a ref from the repository so we know it doesn't exist
-	gittest.Exec(t, cfg, "-C", repoPath, "update-ref", "-d", "refs/heads/many_files")
+	commitID1 := gittest.WriteCommit(t, cfg, repoPath,
+		gittest.WithBranch("master"), gittest.WithMessage("commit-1"), gittest.WithParents(),
+	)
+	commitID2 := gittest.WriteCommit(t, cfg, repoPath,
+		gittest.WithBranch("feature"), gittest.WithMessage("commit-2"), gittest.WithParents(commitID1),
+	)
+	commitID3 := gittest.WriteCommit(t, cfg, repoPath,
+		gittest.WithMessage("commit-3"), gittest.WithParents(commitID1),
+	)
 
-	nonexistingObject := "abcdefg"
-	cmd := gittest.NewCommand(t, cfg, "-C", repoPath, "rev-parse", nonexistingObject)
-	require.Error(t, cmd.Wait(), "ensure the object doesn't exist")
-
-	testCases := []struct {
-		desc               string
-		input              [][]byte
-		revisionsExistence map[string]bool
-		returnCode         codes.Code
+	for _, tc := range []struct {
+		desc            string
+		requests        []*gitalypb.CheckObjectsExistRequest
+		expectedResults map[string]bool
+		expectedErr     error
 	}{
 		{
-			desc: "commit ids and refs that exist",
-			input: [][]byte{
-				[]byte(commitID1),
-				[]byte("master"),
-				[]byte(commitID2),
-				[]byte(commitID3),
-				[]byte("feature"),
+			desc:     "no requests",
+			requests: []*gitalypb.CheckObjectsExistRequest{},
+			// Ideally, we'd return an invalid-argument error in case there aren't any
+			// requests. We can't do this though as this would diverge from Praefect's
+			// behaviour, which always returns `io.EOF`.
+			expectedResults: map[string]bool{},
+		},
+		{
+			desc: "missing repository",
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Revisions: [][]byte{},
+				},
+			},
+			// This is a bug: we don't verify there's a repository.
+			expectedErr:     helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: ""`),
+			expectedResults: map[string]bool{},
+		},
+		{
+			desc: "request without revisions",
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+				},
 			},
-			revisionsExistence: map[string]bool{
-				"master":           true,
-				commitID2.String(): true,
-				commitID3.String(): true,
-				"feature":          true,
+			expectedResults: map[string]bool{},
+		},
+		{
+			desc: "commit ids and refs that exist",
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+					Revisions: [][]byte{
+						[]byte(commitID1),
+						[]byte("master"),
+						[]byte(commitID2),
+						[]byte(commitID3),
+						[]byte("feature"),
+					},
+				},
 			},
-			returnCode: codes.OK,
+			// This is a bug: revisions of the first message are not checked.
+			expectedResults: map[string]bool{},
 		},
 		{
 			desc: "ref and objects missing",
-			input: [][]byte{
-				[]byte(commitID1),
-				[]byte("master"),
-				[]byte(commitID2),
-				[]byte(commitID3),
-				[]byte("feature"),
-				[]byte("many_files"),
-				[]byte(nonexistingObject),
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+					Revisions: [][]byte{
+						[]byte(commitID1),
+						[]byte("master"),
+						[]byte(commitID2),
+						[]byte(commitID3),
+						[]byte("feature"),
+						[]byte("refs/does/not/exist"),
+					},
+				},
 			},
-			revisionsExistence: map[string]bool{
-				"master":           true,
-				commitID2.String(): true,
-				commitID3.String(): true,
-				"feature":          true,
-				"many_files":       false,
-				nonexistingObject:  false,
+			// This is a bug: revisions of the first message are not checked.
+			expectedResults: map[string]bool{},
+		},
+		{
+			desc: "chunked input",
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+					Revisions: [][]byte{
+						[]byte(commitID1),
+					},
+				},
+				{
+					Revisions: [][]byte{
+						[]byte(commitID2),
+					},
+				},
+				{
+					Revisions: [][]byte{
+						[]byte("refs/does/not/exist"),
+					},
+				},
+				{
+					Revisions: [][]byte{
+						[]byte(commitID3),
+					},
+				},
+			},
+			// This is a bug: revisions of the first message are not checked.
+			expectedResults: map[string]bool{
+				commitID2.String():    true,
+				commitID3.String():    true,
+				"refs/does/not/exist": false,
 			},
-			returnCode: codes.OK,
 		},
 		{
-			desc:               "empty input",
-			input:              [][]byte{},
-			returnCode:         codes.OK,
-			revisionsExistence: map[string]bool{},
+			desc: "invalid input",
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+					Revisions: [][]byte{
+						[]byte("-not-a-rev"),
+					},
+				},
+			},
+			expectedErr:     helper.ErrInvalidArgumentf("revision can't start with '-'"),
+			expectedResults: map[string]bool{},
 		},
 		{
-			desc:       "invalid input",
-			input:      [][]byte{[]byte("-not-a-rev")},
-			returnCode: codes.InvalidArgument,
+			desc: "input with whitespace",
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+					Revisions: [][]byte{
+						[]byte(fmt.Sprintf("%s\n%s", commitID1, commitID2)),
+					},
+				},
+			},
+			// This is a bug: revisions of the first message are not checked.
+			expectedErr:     nil,
+			expectedResults: map[string]bool{},
 		},
-	}
-
-	for _, tc := range testCases {
+		{
+			desc: "chunked invalid input",
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+					Revisions: [][]byte{
+						[]byte(commitID1),
+					},
+				},
+				{
+					Revisions: [][]byte{
+						[]byte("-not-a-rev"),
+					},
+				},
+			},
+			// This is a bug: we only check that revisions of the first message are
+			// valid, but don't care about any of the latter ones.
+			expectedErr: nil,
+			expectedResults: map[string]bool{
+				"-not-a-rev": false,
+			},
+		},
+	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			c, err := client.CheckObjectsExist(ctx)
+			client, err := client.CheckObjectsExist(ctx)
 			require.NoError(t, err)
 
-			require.NoError(t, c.Send(
-				&gitalypb.CheckObjectsExistRequest{
-					Repository: repo,
-					Revisions:  tc.input,
-				},
-			))
-			require.NoError(t, c.CloseSend())
+			for _, request := range tc.requests {
+				require.NoError(t, client.Send(request))
+			}
+			require.NoError(t, client.CloseSend())
 
+			results := map[string]bool{}
 			for {
-				resp, err := c.Recv()
-				if tc.returnCode != codes.OK {
-					testhelper.RequireGrpcCode(t, err, tc.returnCode)
-					break
-				} else if err != nil {
-					require.Error(t, err, io.EOF)
+				var response *gitalypb.CheckObjectsExistResponse
+				response, err = client.Recv()
+				if err != nil {
 					break
 				}
 
-				actualRevisionsExistence := make(map[string]bool)
-				for _, revisionExistence := range resp.GetRevisions() {
-					actualRevisionsExistence[string(revisionExistence.GetName())] = revisionExistence.GetExists()
+				for _, revision := range response.GetRevisions() {
+					results[string(revision.GetName())] = revision.GetExists()
 				}
-				assert.Equal(t, tc.revisionsExistence, actualRevisionsExistence)
 			}
+
+			if tc.expectedErr == nil {
+				tc.expectedErr = io.EOF
+			}
+
+			testhelper.RequireGrpcError(t, tc.expectedErr, err)
+			require.Equal(t, tc.expectedResults, results)
 		})
 	}
 }
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index 54f07fe26..8b56ce272 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -839,7 +839,8 @@ func TestPostReceive_allRejected(t *testing.T) {
 			ctx context.Context,
 			repo *gitalypb.Repository,
 			pushOptions, env []string,
-			stdin io.Reader, stdout, stderr io.Writer) error {
+			stdin io.Reader, stdout, stderr io.Writer,
+		) error {
 			return errors.New("not allowed")
 		},
 		gitalyhook.NopPostReceive,