catfile: Use `ObjectReader` behind the feature flag4420-utilize-git-cat-file-batch-command-mode-2

Now that we finally have `ObjectReader` ready to be utilized, we use
this behind the `CatfileBatchCommand` featureflag in `catfile/cache.go`.

We also add the required featuresets for tests and a top level flip of
the featureflag in `testhelper.go`, since this code is used by most of
the RPCs.

4 files changed, 64 insertions, 13 deletions

diff --git a/internal/git/catfile/cache.go b/internal/git/catfile/cache.go
index 493e7b9d3..744c80363 100644
--- a/internal/git/catfile/cache.go
+++ b/internal/git/catfile/cache.go
@@ -14,6 +14,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/labkit/correlation"
 )
 
@@ -171,16 +172,31 @@ func (c *ProcessCache) Stop() {
 
 // ObjectReader creates a new ObjectReader process for the given repository.
 func (c *ProcessCache) ObjectReader(ctx context.Context, repo git.RepositoryExecutor) (ObjectContentReader, func(), error) {
-	cacheable, cancel, err := c.getOrCreateProcess(ctx, repo, &c.objectReaders, func(ctx context.Context) (cacheable, error) {
-		return newObjectContentReader(ctx, repo, c.catfileLookupCounter)
-	}, "catfile.ObjectReader")
+	var cached cacheable
+	var err error
+	var cancel func()
+
+	version, err := repo.GitVersion(ctx)
+	if err != nil {
+		return nil, nil, fmt.Errorf("git version: %w", err)
+	}
+
+	if featureflag.CatfileBatchCommand.IsEnabled(ctx) && version.IsCatfileBatchCommandSupported() {
+		cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectReaders, func(ctx context.Context) (cacheable, error) {
+			return newObjectReader(ctx, repo, c.catfileLookupCounter)
+		}, "catfile.ObjectReader")
+	} else {
+		cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectReaders, func(ctx context.Context) (cacheable, error) {
+			return newObjectContentReader(ctx, repo, c.catfileLookupCounter)
+		}, "catfile.ObjectContentReader")
+	}
 	if err != nil {
 		return nil, nil, err
 	}
 
-	objectReader, ok := cacheable.(ObjectContentReader)
+	objectReader, ok := cached.(ObjectContentReader)
 	if !ok {
-		return nil, nil, fmt.Errorf("expected object reader, got %T", cacheable)
+		return nil, nil, fmt.Errorf("expected object reader, got %T", cached)
 	}
 
 	return objectReader, cancel, nil
@@ -188,16 +204,31 @@ func (c *ProcessCache) ObjectReader(ctx context.Context, repo git.RepositoryExec
 
 // ObjectInfoReader creates a new ObjectInfoReader process for the given repository.
 func (c *ProcessCache) ObjectInfoReader(ctx context.Context, repo git.RepositoryExecutor) (ObjectInfoReader, func(), error) {
-	cacheable, cancel, err := c.getOrCreateProcess(ctx, repo, &c.objectInfoReaders, func(ctx context.Context) (cacheable, error) {
-		return newObjectInfoReader(ctx, repo, c.catfileLookupCounter)
-	}, "catfile.ObjectInfoReader")
+	var cached cacheable
+	var err error
+	var cancel func()
+
+	version, err := repo.GitVersion(ctx)
+	if err != nil {
+		return nil, nil, fmt.Errorf("git version: %w", err)
+	}
+
+	if featureflag.CatfileBatchCommand.IsEnabled(ctx) && version.IsCatfileBatchCommandSupported() {
+		cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectInfoReaders, func(ctx context.Context) (cacheable, error) {
+			return newObjectReader(ctx, repo, c.catfileLookupCounter)
+		}, "catfile.ObjectReader")
+	} else {
+		cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectInfoReaders, func(ctx context.Context) (cacheable, error) {
+			return newObjectInfoReader(ctx, repo, c.catfileLookupCounter)
+		}, "catfile.ObjectInfoReader")
+	}
 	if err != nil {
 		return nil, nil, err
 	}
 
-	objectInfoReader, ok := cacheable.(ObjectInfoReader)
+	objectInfoReader, ok := cached.(ObjectInfoReader)
 	if !ok {
-		return nil, nil, fmt.Errorf("expected object info reader, got %T", cacheable)
+		return nil, nil, fmt.Errorf("expected object info reader, got %T", cached)
 	}
 
 	return objectInfoReader, cancel, nil
diff --git a/internal/git/catfile/cache_test.go b/internal/git/catfile/cache_test.go
index e2ca83332..129143fc3 100644
--- a/internal/git/catfile/cache_test.go
+++ b/internal/git/catfile/cache_test.go
@@ -13,6 +13,7 @@ import (
 	"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"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/labkit/correlation"
@@ -203,7 +204,12 @@ func TestCache_autoExpiry(t *testing.T) {
 }
 
 func TestCache_ObjectReader(t *testing.T) {
-	ctx := testhelper.Context(t)
+	t.Parallel()
+
+	testhelper.NewFeatureSets(featureflag.CatfileBatchCommand, featureflag.GitV238).Run(t, testCacheObjectReader)
+}
+
+func testCacheObjectReader(t *testing.T, ctx context.Context) {
 	cfg := testcfg.Build(t)
 
 	repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
@@ -303,7 +309,12 @@ func TestCache_ObjectReader(t *testing.T) {
 }
 
 func TestCache_ObjectInfoReader(t *testing.T) {
-	ctx := testhelper.Context(t)
+	t.Parallel()
+
+	testhelper.NewFeatureSets(featureflag.CatfileBatchCommand, featureflag.GitV238).Run(t, testCacheObjectInfoReader)
+}
+
+func testCacheObjectInfoReader(t *testing.T, ctx context.Context) {
 	cfg := testcfg.Build(t)
 
 	repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
diff --git a/internal/git/catfile/tag_test.go b/internal/git/catfile/tag_test.go
index efc54e20c..fa9da5763 100644
--- a/internal/git/catfile/tag_test.go
+++ b/internal/git/catfile/tag_test.go
@@ -1,6 +1,7 @@
 package catfile
 
 import (
+	"context"
 	"fmt"
 	"strings"
 	"testing"
@@ -9,13 +10,18 @@ import (
 	"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/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
 
 func TestGetTag(t *testing.T) {
-	ctx := testhelper.Context(t)
+	t.Parallel()
 
+	testhelper.NewFeatureSets(featureflag.CatfileBatchCommand).Run(t, testGetTag)
+}
+
+func testGetTag(t *testing.T, ctx context.Context) {
 	cfg, objectReader, _, repoPath := setupObjectReader(t, ctx)
 	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index c9015ee93..f5b4eeb66 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -205,6 +205,9 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context {
 	// PraefectUseYamuxConfigurationForGitaly gets tested in Praefect when routing RPCs and thus it affects many tests.
 	// Let's randomly select which connection we use so both sets of connections get tested somewhat.
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.PraefectUseYamuxConfigurationForGitaly, rnd.Int()%2 == 0)
+	// CatfileBatchCommand affects many tests since most of them rely on catfile for content/info
+	// information about objects.
+	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.CatfileBatchCommand, rnd.Int()%2 == 0)
 
 	for _, opt := range opts {
 		ctx = opt(ctx)