Merge branch '4573-featureflag-enable-batch-command-for-git-cat-file-1-2' into 'master'

featureflag: Remove the `CatfileBatchCommand` flag

Closes #4573

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6210

Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>

5 files changed, 7 insertions, 32 deletions

diff --git a/internal/featureflag/ff_catfile_batch_command.go b/internal/featureflag/ff_catfile_batch_command.go
deleted file mode 100644
index 0702a592d..000000000
--- a/internal/featureflag/ff_catfile_batch_command.go
+++ /dev/null
@@ -1,9 +0,0 @@
-package featureflag
-
-// CatfileBatchCommand enables the `--batch-command` mode for git-cat-file(1).
-var CatfileBatchCommand = NewFeatureFlag(
-	"catfile_batch_command",
-	"v16.2.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4573",
-	false,
-)
diff --git a/internal/git/catfile/cache.go b/internal/git/catfile/cache.go
index d44b0dcdf..494493785 100644
--- a/internal/git/catfile/cache.go
+++ b/internal/git/catfile/cache.go
@@ -8,7 +8,6 @@ import (
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
@@ -186,7 +185,7 @@ func (c *ProcessCache) ObjectReader(ctx context.Context, repo git.RepositoryExec
 		return nil, nil, fmt.Errorf("git version: %w", err)
 	}
 
-	if featureflag.CatfileBatchCommand.IsEnabled(ctx) && version.CatfileSupportsNulTerminatedOutput() {
+	if version.CatfileSupportsNulTerminatedOutput() {
 		cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectReaders, func(ctx context.Context) (cacheable, error) {
 			return newObjectReader(ctx, repo, c.catfileLookupCounter)
 		}, "catfile.ObjectReader")
@@ -218,7 +217,7 @@ func (c *ProcessCache) ObjectInfoReader(ctx context.Context, repo git.Repository
 		return nil, nil, fmt.Errorf("git version: %w", err)
 	}
 
-	if featureflag.CatfileBatchCommand.IsEnabled(ctx) && version.CatfileSupportsNulTerminatedOutput() {
+	if version.CatfileSupportsNulTerminatedOutput() {
 		cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectReaders, func(ctx context.Context) (cacheable, error) {
 			return newObjectReader(ctx, repo, c.catfileLookupCounter)
 		}, "catfile.ObjectReader")
diff --git a/internal/git/catfile/cache_test.go b/internal/git/catfile/cache_test.go
index 26c0a439f..d4e371797 100644
--- a/internal/git/catfile/cache_test.go
+++ b/internal/git/catfile/cache_test.go
@@ -9,7 +9,6 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
@@ -206,10 +205,7 @@ func TestCache_autoExpiry(t *testing.T) {
 func TestCache_ObjectReader(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.CatfileBatchCommand).Run(t, testCacheObjectReader)
-}
-
-func testCacheObjectReader(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 
 	repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
@@ -254,7 +250,7 @@ func testCacheObjectReader(t *testing.T, ctx context.Context) {
 		cancel()
 
 		var allKeys []key
-		if featureflag.CatfileBatchCommand.IsEnabled(ctx) && version.CatfileSupportsNulTerminatedOutput() {
+		if version.CatfileSupportsNulTerminatedOutput() {
 			allKeys = keys(t, &cache.objectReaders)
 		} else {
 			allKeys = keys(t, &cache.objectContentReaders)
@@ -320,10 +316,7 @@ func testCacheObjectReader(t *testing.T, ctx context.Context) {
 func TestCache_ObjectInfoReader(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.CatfileBatchCommand).Run(t, testCacheObjectInfoReader)
-}
-
-func testCacheObjectInfoReader(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 
 	repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
@@ -367,7 +360,7 @@ func testCacheObjectInfoReader(t *testing.T, ctx context.Context) {
 		cancel()
 
 		var allKeys []key
-		if featureflag.CatfileBatchCommand.IsEnabled(ctx) && version.CatfileSupportsNulTerminatedOutput() {
+		if version.CatfileSupportsNulTerminatedOutput() {
 			allKeys = keys(t, &cache.objectReaders)
 		} else {
 			allKeys = keys(t, &cache.objectInfoReaders)
diff --git a/internal/git/catfile/tag_test.go b/internal/git/catfile/tag_test.go
index 2e8c03660..8c2e5b015 100644
--- a/internal/git/catfile/tag_test.go
+++ b/internal/git/catfile/tag_test.go
@@ -1,13 +1,11 @@
 package catfile
 
 import (
-	"context"
 	"fmt"
 	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper"
@@ -18,10 +16,7 @@ import (
 func TestGetTag(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.CatfileBatchCommand).Run(t, testGetTag)
-}
-
-func testGetTag(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	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 17b511bc4..4cf5555ff 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -202,9 +202,6 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context {
 	// deep in the call stack, so almost every test function would have to inject it into its
 	// context. The values of these flags should be randomized to increase the test coverage.
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RunCommandsInCGroup, true)
-	// CatfileBatchCommand affects many tests since most of them rely on catfile for content/info
-	// information about objects.
-	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.CatfileBatchCommand, rand.Int()%2 == 0)
 	// Randomly enable mailmap
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.MailmapOptions, rand.Int()%2 == 0)
 	// LowerBigFileThreshold is checked on every spawn of Git commands and is thus infeasible to be checked for