Merge branch 'pks-check-objects-exist-improvements' into 'master'

commit: Various fixes for `CheckObjectsExist()`

See merge request gitlab-org/gitaly!4510

9 files changed, 300 insertions, 128 deletions

diff --git a/internal/git/catfile/parser.go b/internal/git/catfile/parser.go
index 41e8e5029..d5d5488f5 100644
--- a/internal/git/catfile/parser.go
+++ b/internal/git/catfile/parser.go
@@ -7,6 +7,7 @@ import (
 	"io"
 	"strconv"
 	"strings"
+	"time"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
@@ -124,6 +125,11 @@ func (p *parser) ParseCommit(object git.Object) (*gitalypb.GitCommit, error) {
 
 const maxUnixCommitDate = 1 << 53
 
+// fallbackTimeValue is the value returned in case there is a parse error. It's the maximum
+// time value possible in golang. See
+// https://gitlab.com/gitlab-org/gitaly/issues/556#note_40289573
+var fallbackTimeValue = time.Unix(1<<63-62135596801, 999999999)
+
 func parseCommitAuthor(line string) *gitalypb.CommitAuthor {
 	author := &gitalypb.CommitAuthor{}
 
@@ -149,7 +155,7 @@ func parseCommitAuthor(line string) *gitalypb.CommitAuthor {
 
 	sec, err := strconv.ParseInt(secSplit[0], 10, 64)
 	if err != nil || sec > maxUnixCommitDate || sec < 0 {
-		sec = git.FallbackTimeValue.Unix()
+		sec = fallbackTimeValue.Unix()
 	}
 
 	author.Date = &timestamppb.Timestamp{Seconds: sec}
diff --git a/internal/git/helper_test.go b/internal/git/helper_test.go
index 5734c024c..c8ce23220 100644
--- a/internal/git/helper_test.go
+++ b/internal/git/helper_test.go
@@ -12,30 +12,6 @@ func TestMain(m *testing.M) {
 	testhelper.Run(m)
 }
 
-func TestValidateRevision(t *testing.T) {
-	testCases := []struct {
-		rev string
-		ok  bool
-	}{
-		{rev: "foo/bar", ok: true},
-		{rev: "-foo/bar", ok: false},
-		{rev: "foo bar", ok: false},
-		{rev: "foo\x00bar", ok: false},
-		{rev: "foo/bar:baz", ok: false},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.rev, func(t *testing.T) {
-			err := ValidateRevision([]byte(tc.rev))
-			if tc.ok {
-				require.NoError(t, err)
-			} else {
-				require.Error(t, err)
-			}
-		})
-	}
-}
-
 func newCommandFactory(tb testing.TB, cfg config.Cfg, opts ...ExecCommandFactoryOption) *ExecCommandFactory {
 	gitCmdFactory, cleanup, err := NewExecCommandFactory(cfg, opts...)
 	require.NoError(tb, err)
diff --git a/internal/git/proto.go b/internal/git/revision.go
index 97dac733b..89d1e05d4 100644
--- a/internal/git/proto.go
+++ b/internal/git/revision.go
@@ -3,14 +3,8 @@ package git
 import (
 	"bytes"
 	"fmt"
-	"time"
 )
 
-// FallbackTimeValue is the value returned by `SafeTimeParse` in case it
-// encounters a parse error. It's the maximum time value possible in golang.
-// See https://gitlab.com/gitlab-org/gitaly/issues/556#note_40289573
-var FallbackTimeValue = time.Unix(1<<63-62135596801, 999999999)
-
 func validateRevision(revision []byte, allowEmpty bool) error {
 	if !allowEmpty && len(revision) == 0 {
 		return fmt.Errorf("empty revision")
@@ -18,7 +12,7 @@ func validateRevision(revision []byte, allowEmpty bool) error {
 	if bytes.HasPrefix(revision, []byte("-")) {
 		return fmt.Errorf("revision can't start with '-'")
 	}
-	if bytes.Contains(revision, []byte(" ")) {
+	if bytes.ContainsAny(revision, " \t\n\r") {
 		return fmt.Errorf("revision can't contain whitespace")
 	}
 	if bytes.Contains(revision, []byte("\x00")) {
diff --git a/internal/git/revision_test.go b/internal/git/revision_test.go
new file mode 100644
index 000000000..62751690a
--- /dev/null
+++ b/internal/git/revision_test.go
@@ -0,0 +1,77 @@
+package git
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidateRevision(t *testing.T) {
+	for _, tc := range []struct {
+		desc        string
+		revision    string
+		expectedErr error
+	}{
+		{
+			desc:        "empty revision",
+			revision:    "",
+			expectedErr: fmt.Errorf("empty revision"),
+		},
+		{
+			desc:     "valid revision",
+			revision: "foo/bar",
+		},
+		{
+			desc:        "leading dash",
+			revision:    "-foo/bar",
+			expectedErr: fmt.Errorf("revision can't start with '-'"),
+		},
+		{
+			desc:     "intermediate dash",
+			revision: "foo-bar",
+		},
+		{
+			desc:        "space",
+			revision:    "foo bar",
+			expectedErr: fmt.Errorf("revision can't contain whitespace"),
+		},
+		{
+			desc:        "newline",
+			revision:    "foo\nbar",
+			expectedErr: fmt.Errorf("revision can't contain whitespace"),
+		},
+		{
+			desc:        "tab",
+			revision:    "foo\tbar",
+			expectedErr: fmt.Errorf("revision can't contain whitespace"),
+		},
+		{
+			desc:        "carriage-return",
+			revision:    "foo\rbar",
+			expectedErr: fmt.Errorf("revision can't contain whitespace"),
+		},
+		{
+			desc:        "NUL-byte",
+			revision:    "foo\x00bar",
+			expectedErr: fmt.Errorf("revision can't contain NUL"),
+		},
+		{
+			desc:        "colon",
+			revision:    "foo/bar:baz",
+			expectedErr: fmt.Errorf("revision can't contain ':'"),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			require.Equal(t, tc.expectedErr, ValidateRevision([]byte(tc.revision)))
+
+			// `ValidateRevision()` and `ValidateRevisionAllowEmpty()` behave the same,
+			// except in the case where the revision is empty. In that case, the latter
+			// does not return an error.
+			if tc.revision == "" {
+				tc.expectedErr = nil
+			}
+			require.Equal(t, tc.expectedErr, ValidateRevisionAllowEmpty([]byte(tc.revision)))
+		})
+	}
+}
diff --git a/internal/gitaly/service/commit/check_objects_exist.go b/internal/gitaly/service/commit/check_objects_exist.go
index 83512b340..653c57132 100644
--- a/internal/gitaly/service/commit/check_objects_exist.go
+++ b/internal/gitaly/service/commit/check_objects_exist.go
@@ -19,11 +19,17 @@ func (s *server) CheckObjectsExist(
 
 	request, err := stream.Recv()
 	if err != nil {
-		return err
+		if err == io.EOF {
+			// 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`.
+			return err
+		}
+		return helper.ErrInternalf("receiving initial request: %w", err)
 	}
 
-	if err := validateCheckObjectsExistRequest(request); err != nil {
-		return err
+	if request.GetRepository() == nil {
+		return helper.ErrInvalidArgumentf("empty Repository")
 	}
 
 	objectInfoReader, cancel, err := s.catfileCache.ObjectInfoReader(
@@ -31,24 +37,39 @@ func (s *server) CheckObjectsExist(
 		s.localrepo(request.GetRepository()),
 	)
 	if err != nil {
-		return err
+		return helper.ErrInternalf("creating object info reader: %w", err)
 	}
 	defer cancel()
 
 	chunker := chunk.New(&checkObjectsExistSender{stream: stream})
 	for {
-		request, err := stream.Recv()
+		// Note: we have already fetched the first request containing revisions further up,
+		// so we only fetch the next request at the end of this loop.
+		for _, revision := range request.GetRevisions() {
+			if err := git.ValidateRevision(revision); err != nil {
+				return helper.ErrInvalidArgumentf("invalid revision %q: %w", revision, err)
+			}
+		}
+
+		if err := checkObjectsExist(ctx, request, objectInfoReader, chunker); err != nil {
+			return helper.ErrInternalf("checking object existence: %w", err)
+		}
+
+		request, err = stream.Recv()
 		if err != nil {
 			if err == io.EOF {
-				return chunker.Flush()
+				break
 			}
-			return err
-		}
 
-		if err = checkObjectsExist(ctx, request, objectInfoReader, chunker); err != nil {
-			return err
+			return helper.ErrInternalf("receiving request: %w", err)
 		}
 	}
+
+	if err := chunker.Flush(); err != nil {
+		return helper.ErrInternalf("flushing results: %w", err)
+	}
+
+	return nil
 }
 
 type checkObjectsExistSender struct {
@@ -63,7 +84,7 @@ func (c *checkObjectsExistSender) Send() error {
 }
 
 func (c *checkObjectsExistSender) Reset() {
-	c.revisions = make([]*gitalypb.CheckObjectsExistResponse_RevisionExistence, 0)
+	c.revisions = c.revisions[:0]
 }
 
 func (c *checkObjectsExistSender) Append(m proto.Message) {
@@ -88,22 +109,12 @@ func checkObjectsExist(
 			if catfile.IsNotFound(err) {
 				revisionExistence.Exists = false
 			} else {
-				return err
+				return helper.ErrInternalf("reading object info: %w", err)
 			}
 		}
 
 		if err := chunker.Send(&revisionExistence); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
-func validateCheckObjectsExistRequest(in *gitalypb.CheckObjectsExistRequest) error {
-	for _, revision := range in.GetRevisions() {
-		if err := git.ValidateRevision(revision); err != nil {
-			return helper.ErrInvalidArgument(err)
+			return helper.ErrInternalf("adding to chunker: %w", err)
 		}
 	}
 
diff --git a/internal/gitaly/service/commit/check_objects_exist_test.go b/internal/gitaly/service/commit/check_objects_exist_test.go
index 97fed524f..f93267bf6 100644
--- a/internal/gitaly/service/commit/check_objects_exist_test.go
+++ b/internal/gitaly/service/commit/check_objects_exist_test.go
@@ -1,121 +1,224 @@
 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:     "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{},
+				},
+			},
+			expectedErr: func() error {
+				if testhelper.IsPraefectEnabled() {
+					return helper.ErrInvalidArgumentf("repo scoped: empty Repository")
+				}
+				return helper.ErrInvalidArgumentf("empty Repository")
+			}(),
+			expectedResults: map[string]bool{},
+		},
+		{
+			desc: "request without revisions",
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+				},
+			},
+			expectedResults: map[string]bool{},
+		},
+		{
 			desc: "commit ids and refs that exist",
-			input: [][]byte{
-				[]byte(commitID1),
-				[]byte("master"),
-				[]byte(commitID2),
-				[]byte(commitID3),
-				[]byte("feature"),
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+					Revisions: [][]byte{
+						[]byte(commitID1),
+						[]byte("master"),
+						[]byte(commitID2),
+						[]byte(commitID3),
+						[]byte("feature"),
+					},
+				},
 			},
-			revisionsExistence: map[string]bool{
+			expectedResults: map[string]bool{
+				commitID1.String(): true,
 				"master":           true,
 				commitID2.String(): true,
 				commitID3.String(): true,
 				"feature":          true,
 			},
-			returnCode: codes.OK,
 		},
 		{
 			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,
+			expectedResults: map[string]bool{
+				commitID1.String():    true,
+				"master":              true,
+				commitID2.String():    true,
+				commitID3.String():    true,
+				"feature":             true,
+				"refs/does/not/exist": false,
 			},
-			returnCode: codes.OK,
 		},
 		{
-			desc:               "empty input",
-			input:              [][]byte{},
-			returnCode:         codes.OK,
-			revisionsExistence: 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),
+					},
+				},
+			},
+			expectedResults: map[string]bool{
+				commitID1.String():    true,
+				commitID2.String():    true,
+				commitID3.String():    true,
+				"refs/does/not/exist": false,
+			},
 		},
 		{
-			desc:       "invalid input",
-			input:      [][]byte{[]byte("-not-a-rev")},
-			returnCode: codes.InvalidArgument,
+			desc: "invalid input",
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+					Revisions: [][]byte{
+						[]byte("-not-a-rev"),
+					},
+				},
+			},
+			expectedErr:     helper.ErrInvalidArgumentf("invalid revision %q: revision can't start with '-'", "-not-a-rev"),
+			expectedResults: map[string]bool{},
 		},
-	}
-
-	for _, tc := range testCases {
+		{
+			desc: "input with whitespace",
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+					Revisions: [][]byte{
+						[]byte(fmt.Sprintf("%s\n%s", commitID1, commitID2)),
+					},
+				},
+			},
+			expectedErr:     helper.ErrInvalidArgumentf("invalid revision %q: revision can't contain whitespace", fmt.Sprintf("%s\n%s", commitID1, commitID2)),
+			expectedResults: map[string]bool{},
+		},
+		{
+			desc: "chunked invalid input",
+			requests: []*gitalypb.CheckObjectsExistRequest{
+				{
+					Repository: repo,
+					Revisions: [][]byte{
+						[]byte(commitID1),
+					},
+				},
+				{
+					Revisions: [][]byte{
+						[]byte("-not-a-rev"),
+					},
+				},
+			},
+			expectedErr:     helper.ErrInvalidArgumentf("invalid revision %q: revision can't start with '-'", "-not-a-rev"),
+			expectedResults: map[string]bool{},
+		},
+	} {
 		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,
diff --git a/proto/commit.proto b/proto/commit.proto
index 2311ac78d..5c5de630b 100644
--- a/proto/commit.proto
+++ b/proto/commit.proto
@@ -576,7 +576,9 @@ message GetCommitMessagesResponse {
   bytes message = 2;
 }
 
-// CheckObjectsExistRequest is a request for the CheckObjectsExist RPC.
+// CheckObjectsExistRequest is a request for the CheckObjectsExist RPC. Only
+// the initial request must contain a repository, the repository of all
+// subsequent requests will be ignored.
 message CheckObjectsExistRequest {
   // Repository is the repository in which existence of objects and refs
   // are checked.
diff --git a/proto/go/gitalypb/commit.pb.go b/proto/go/gitalypb/commit.pb.go
index 09d6de63f..d5b5689a4 100644
--- a/proto/go/gitalypb/commit.pb.go
+++ b/proto/go/gitalypb/commit.pb.go
@@ -3401,7 +3401,9 @@ func (x *GetCommitMessagesResponse) GetMessage() []byte {
 	return nil
 }
 
-// CheckObjectsExistRequest is a request for the CheckObjectsExist RPC.
+// CheckObjectsExistRequest is a request for the CheckObjectsExist RPC. Only
+// the initial request must contain a repository, the repository of all
+// subsequent requests will be ignored.
 type CheckObjectsExistRequest struct {
 	state         protoimpl.MessageState
 	sizeCache     protoimpl.SizeCache