git: Pass rev-list arguments via stdin

Modifies the ObjectsSize RPC to pass arguments to the `rev-list` command
via stdin instead of the command line. This is now possible after the
upgrade to Git v2.42 as pseudo-options are now supported when the
`--stdin` mode is used.

The RPC logic is restructured such that we create the command and wait
for execution in separate steps so revisions can directly be written to
stdin, without the use of an intermediary buffer.

Additionally, a test has been added to ensure that revisions containing
newlines are rejected by existing logic, since we now rely on newlines
to delimit successive revisions and options.

4 files changed, 45 insertions, 25 deletions

diff --git a/internal/gitaly/service/repository/objects_size.go b/internal/gitaly/service/repository/objects_size.go
index 665cb87ef..8bbccb407 100644
--- a/internal/gitaly/service/repository/objects_size.go
+++ b/internal/gitaly/service/repository/objects_size.go
@@ -15,9 +15,8 @@ import (
 )
 
 // revisionNotFoundRegexp is used to parse the standard error of git-rev-list(1) in order to figure out whether the
-// actual error condition was that the revision could not be found. Checking for ambiguous arguments is kind of awkward,
-// but we can't really get around that until we have migrated to `--stdin` with Git v2.42 and later.
-var revisionNotFoundRegexp = regexp.MustCompile("^fatal: ambiguous argument '([^']*)': unknown revision or path not in the working tree.")
+// actual error condition was that the revision could not be found.
+var revisionNotFoundRegexp = regexp.MustCompile("^fatal: bad revision '([^']*)'")
 
 // ObjectsSize calculates the on-disk object size via a graph walk. The intent of this RPC is to
 // calculate the on-disk size as accurately as possible.
@@ -35,11 +34,23 @@ func (s *server) ObjectsSize(server gitalypb.RepositoryService_ObjectsSizeServer
 
 	repo := s.localrepo(request.GetRepository())
 
-	revlistArgs := make([]string, 0, len(request.GetRevisions()))
+	var stderr, stdout strings.Builder
+	cmd, err := repo.Exec(ctx,
+		git.Command{
+			Name: "rev-list",
+			Flags: []git.Option{
+				git.Flag{Name: "--disk-usage"},
+				git.Flag{Name: "--objects"},
+				git.Flag{Name: "--stdin"},
+			},
+		},
+		git.WithStderr(&stderr),
+		git.WithStdout(&stdout),
+		git.WithSetupStdin())
+	if err != nil {
+		return fmt.Errorf("start rev-list command: %w", err)
+	}
 
-	// TODO: Git v2.41 and older do not support passing pseudo-options via `--stdin`. We have upstreamed patches to
-	// fix this in Git v2.42, so once we have upgraded our Git version we should convert this loop to instead stream
-	// to git-rev-list(1)'s standard input.
 	for i := 0; ; i++ {
 		if i != 0 && request.GetRepository() != nil {
 			return structerr.NewInvalidArgument("subsequent requests must not contain repository")
@@ -54,7 +65,11 @@ func (s *server) ObjectsSize(server gitalypb.RepositoryService_ObjectsSizeServer
 				return structerr.NewInvalidArgument("validating revision: %w", err).WithMetadata("revision", revision)
 			}
 
-			revlistArgs = append(revlistArgs, string(revision))
+			// Each revision must be separated by a newline when the `--stdin` option is used, as Git
+			// parses these differently to command-line arguments.
+			if _, err := cmd.Write([]byte(fmt.Sprintf("%s\n", revision))); err != nil {
+				return structerr.NewInvalidArgument("process revision: %w", err).WithMetadata("revision", revision)
+			}
 		}
 
 		request, err = server.Recv()
@@ -67,15 +82,7 @@ func (s *server) ObjectsSize(server gitalypb.RepositoryService_ObjectsSizeServer
 		}
 	}
 
-	var stderr, stdout strings.Builder
-	if err := repo.ExecAndWait(ctx, git.Command{
-		Name: "rev-list",
-		Flags: []git.Option{
-			git.Flag{Name: "--disk-usage"},
-			git.Flag{Name: "--objects"},
-		},
-		Args: revlistArgs,
-	}, git.WithStderr(&stderr), git.WithStdout(&stdout)); err != nil {
+	if err := cmd.Wait(); err != nil {
 		var exitErr *exec.ExitError
 		if errors.As(err, &exitErr) {
 			if matches := revisionNotFoundRegexp.FindStringSubmatch(stderr.String()); len(matches) == 2 {
diff --git a/internal/gitaly/service/repository/objects_size_test.go b/internal/gitaly/service/repository/objects_size_test.go
index 90f798245..11723ddfe 100644
--- a/internal/gitaly/service/repository/objects_size_test.go
+++ b/internal/gitaly/service/repository/objects_size_test.go
@@ -130,6 +130,27 @@ func TestObjectsSize(t *testing.T) {
 			},
 		},
 		{
+			desc: "revision with newline character",
+			setup: func(t *testing.T) setupData {
+				repo, _ := gittest.CreateRepository(t, ctx, cfg)
+
+				return setupData{
+					requests: []*gitalypb.ObjectsSizeRequest{
+						{
+							Repository: repo,
+							Revisions: [][]byte{
+								[]byte("HEAD\n"),
+							},
+						},
+					},
+					expectedErr: testhelper.WithInterceptedMetadata(
+						structerr.NewInvalidArgument("validating revision: revision can't contain whitespace"),
+						"revision", []byte("HEAD\n"),
+					),
+				}
+			},
+		},
+		{
 			desc: "missing revision",
 			setup: func(t *testing.T) setupData {
 				repo, _ := gittest.CreateRepository(t, ctx, cfg)
diff --git a/proto/go/gitalypb/repository.pb.go b/proto/go/gitalypb/repository.pb.go
index 01e2dfe9e..ea006ee5f 100644
--- a/proto/go/gitalypb/repository.pb.go
+++ b/proto/go/gitalypb/repository.pb.go
@@ -530,10 +530,6 @@ type ObjectsSizeRequest struct {
 	// revisions is the set of revisions that shall be used to compute the object size for. Supports normal revisions as
 	// well as pseudo-revisions like `--not`, `--all`, `--branches[=pattern]`, `--tags[=pattern]` and `--glob=pattern`.
 	// Please refer to the man pages gitrevisions(7) as well as git-rev-list(1) for more information.
-	//
-	// Note: due to a restriction in Git v2.41 and older all arguments need to be passed via command line parameters. So
-	// even though you can send multiple requests, the actual number of revisions that can be passed is limited by the
-	// maximum command line length. This limitation will be lifted once Gitaly has upgraded to Git v2.42 and later.
 	Revisions [][]byte `protobuf:"bytes,2,rep,name=revisions,proto3" json:"revisions,omitempty"`
 }
 
diff --git a/proto/repository.proto b/proto/repository.proto
index 27c5ce666..f79d2da24 100644
--- a/proto/repository.proto
+++ b/proto/repository.proto
@@ -503,10 +503,6 @@ message ObjectsSizeRequest {
   // revisions is the set of revisions that shall be used to compute the object size for. Supports normal revisions as
   // well as pseudo-revisions like `--not`, `--all`, `--branches[=pattern]`, `--tags[=pattern]` and `--glob=pattern`.
   // Please refer to the man pages gitrevisions(7) as well as git-rev-list(1) for more information.
-  //
-  // Note: due to a restriction in Git v2.41 and older all arguments need to be passed via command line parameters. So
-  // even though you can send multiple requests, the actual number of revisions that can be passed is limited by the
-  // maximum command line length. This limitation will be lifted once Gitaly has upgraded to Git v2.42 and later.
   repeated bytes revisions = 2;
 }