commit: Do not ignore revisions of the initial request

The `CheckObjectsExist()` RPC is a streaming RPC: the caller may send
one or more requests to check sets of revisions. This is done such that
this RPC call also works in the context where millions of revisions need
to be checked in a single call, which could otherwise exceed the gRPC
memory limitations.

The current behaviour is a tad weird though: revisions sent as part of
the initial request are completely ignored. This doesn't feel right as
the caller would now be forced to _always_ send at least two messages.
It's also not documented to be the case, and last but not least all our
other streaming RPCs don't do it this way either.

So let's fix this behaviour and also honor revisions sent of the initial
request.

Changelog: fixed

4 files changed, 49 insertions, 19 deletions

diff --git a/internal/gitaly/service/commit/check_objects_exist.go b/internal/gitaly/service/commit/check_objects_exist.go
index a4224f206..4b72bb13e 100644
--- a/internal/gitaly/service/commit/check_objects_exist.go
+++ b/internal/gitaly/service/commit/check_objects_exist.go
@@ -37,14 +37,8 @@ func (s *server) CheckObjectsExist(
 
 	chunker := chunk.New(&checkObjectsExistSender{stream: stream})
 	for {
-		request, err := stream.Recv()
-		if err != nil {
-			if err == io.EOF {
-				return chunker.Flush()
-			}
-			return err
-		}
-
+		// 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)
@@ -54,7 +48,22 @@ func (s *server) CheckObjectsExist(
 		if err = checkObjectsExist(ctx, request, objectInfoReader, chunker); err != nil {
 			return err
 		}
+
+		request, err = stream.Recv()
+		if err != nil {
+			if err == io.EOF {
+				break
+			}
+
+			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 {
diff --git a/internal/gitaly/service/commit/check_objects_exist_test.go b/internal/gitaly/service/commit/check_objects_exist_test.go
index 99b1328a0..efe069bff 100644
--- a/internal/gitaly/service/commit/check_objects_exist_test.go
+++ b/internal/gitaly/service/commit/check_objects_exist_test.go
@@ -82,8 +82,13 @@ func TestCheckObjectsExist(t *testing.T) {
 					},
 				},
 			},
-			// This is a bug: revisions of the first message are not checked.
-			expectedResults: map[string]bool{},
+			expectedResults: map[string]bool{
+				commitID1.String(): true,
+				"master":           true,
+				commitID2.String(): true,
+				commitID3.String(): true,
+				"feature":          true,
+			},
 		},
 		{
 			desc: "ref and objects missing",
@@ -100,8 +105,14 @@ func TestCheckObjectsExist(t *testing.T) {
 					},
 				},
 			},
-			// This is a bug: revisions of the first message are not checked.
-			expectedResults: map[string]bool{},
+			expectedResults: map[string]bool{
+				commitID1.String():    true,
+				"master":              true,
+				commitID2.String():    true,
+				commitID3.String():    true,
+				"feature":             true,
+				"refs/does/not/exist": false,
+			},
 		},
 		{
 			desc: "chunked input",
@@ -128,8 +139,8 @@ func TestCheckObjectsExist(t *testing.T) {
 					},
 				},
 			},
-			// This is a bug: revisions of the first message are not checked.
 			expectedResults: map[string]bool{
+				commitID1.String():    true,
 				commitID2.String():    true,
 				commitID3.String():    true,
 				"refs/does/not/exist": false,
@@ -145,7 +156,7 @@ func TestCheckObjectsExist(t *testing.T) {
 					},
 				},
 			},
-			// This is a bug: revisions of the first message are not checked.
+			expectedErr:     helper.ErrInvalidArgumentf("invalid revision %q: revision can't start with '-'", "-not-a-rev"),
 			expectedResults: map[string]bool{},
 		},
 		{
@@ -158,9 +169,15 @@ func TestCheckObjectsExist(t *testing.T) {
 					},
 				},
 			},
-			// This is a bug: revisions of the first message are not checked.
-			expectedErr:     nil,
-			expectedResults: map[string]bool{},
+			// This is a bug: instead of disallowing revisions with newlines, we just
+			// handle them funnily. Git things we're asking it for two different
+			// revisions, and we expect only one output. So the result is that this
+			// weirdly formatted revision looks like it would exist. Instead, we should
+			// raise an error.
+			expectedErr: nil,
+			expectedResults: map[string]bool{
+				fmt.Sprintf("%s\n%s", commitID1, commitID2): true,
+			},
 		},
 		{
 			desc: "chunked invalid input",
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