featureflag: Remove LocalrepoReadObjectCached flag

The feature flag "localrepo_read_object_cached" was enabled on
production for a while without issues, so we can default to using the
catfile cache and remove the old code.

Issue: https://gitlab.com/gitlab-org/gitaly/-/issues/4662

5 files changed, 33 insertions, 109 deletions

diff --git a/internal/featureflag/ff_localrepo_read_object_cached.go b/internal/featureflag/ff_localrepo_read_object_cached.go
deleted file mode 100644
index 40e60d1f1..000000000
--- a/internal/featureflag/ff_localrepo_read_object_cached.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// LocalrepoReadObjectCached enables the use of the catfile cache for
-// localrepo.ReadObject
-var LocalrepoReadObjectCached = NewFeatureFlag(
-	"localrepo_read_object_cached",
-	"v15.7",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4662",
-	false,
-)
diff --git a/internal/git/localrepo/objects.go b/internal/git/localrepo/objects.go
index 58a5abf30..2b6084cc9 100644
--- a/internal/git/localrepo/objects.go
+++ b/internal/git/localrepo/objects.go
@@ -11,7 +11,6 @@ import (
 	"time"
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/command"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/text"
@@ -207,40 +206,6 @@ func (repo *Repo) ReadObjectInfo(ctx context.Context, rev git.Revision) (*catfil
 // ReadObject reads an object from the repository's object database. InvalidObjectError
 // is returned if the oid does not refer to a valid object.
 func (repo *Repo) ReadObject(ctx context.Context, oid git.ObjectID) ([]byte, error) {
-	if featureflag.LocalrepoReadObjectCached.IsEnabled(ctx) {
-		return repo.readObjectCached(ctx, oid)
-	}
-
-	const msgInvalidObject = "fatal: Not a valid object name "
-
-	stdout := &bytes.Buffer{}
-	stderr := &bytes.Buffer{}
-	cmd, err := repo.Exec(ctx,
-		git.Command{
-			Name:  "cat-file",
-			Flags: []git.Option{git.Flag{Name: "-p"}},
-			Args:  []string{oid.String()},
-		},
-		git.WithStdout(stdout),
-		git.WithStderr(stderr),
-	)
-	if err != nil {
-		return nil, err
-	}
-
-	if err := cmd.Wait(); err != nil {
-		msg := text.ChompBytes(stderr.Bytes())
-		if strings.HasPrefix(msg, msgInvalidObject) {
-			return nil, InvalidObjectError(strings.TrimPrefix(msg, msgInvalidObject))
-		}
-
-		return nil, errorWithStderr(err, stderr.Bytes())
-	}
-
-	return stdout.Bytes(), nil
-}
-
-func (repo *Repo) readObjectCached(ctx context.Context, oid git.ObjectID) ([]byte, error) {
 	objectReader, cancel, err := repo.catfileCache.ObjectReader(ctx, repo)
 	if err != nil {
 		return nil, fmt.Errorf("create object reader: %w", err)
diff --git a/internal/git/localrepo/objects_test.go b/internal/git/localrepo/objects_test.go
index 036ef2a2e..d9075e5d4 100644
--- a/internal/git/localrepo/objects_test.go
+++ b/internal/git/localrepo/objects_test.go
@@ -2,7 +2,6 @@ package localrepo
 
 import (
 	"bytes"
-	"context"
 	"fmt"
 	"io"
 	"os"
@@ -13,7 +12,6 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"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/catfile"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
@@ -33,10 +31,7 @@ func (fn ReaderFunc) Read(b []byte) (int, error) { return fn(b) }
 func TestRepo_WriteBlob(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.LocalrepoReadObjectCached).Run(t, testRepoWriteBlob)
-}
-
-func testRepoWriteBlob(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	_, repo, repoPath := setupRepo(t)
 
 	for _, tc := range []struct {
@@ -248,10 +243,7 @@ tagger root <root@localhost> 12345 -0100
 func TestRepo_ReadObject(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.LocalrepoReadObjectCached).Run(t, testRepoReadObject)
-}
-
-func testRepoReadObject(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath := setupRepo(t)
 	blobID := gittest.WriteBlob(t, cfg, repoPath, []byte("content"))
 
@@ -326,10 +318,7 @@ func TestRepoReadObjectInfo(t *testing.T) {
 func TestRepo_ReadObject_catfileCount(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.LocalrepoReadObjectCached).Run(t, testRepoReadObjectCatfileCount)
-}
-
-func testRepoReadObjectCatfileCount(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 
 	gitCmdFactory := gittest.NewCountingCommandFactory(t, cfg)
@@ -348,17 +337,13 @@ func testRepoReadObjectCatfileCount(t *testing.T, ctx context.Context) {
 
 	blobID := gittest.WriteBlob(t, cfg, repoPath, []byte("content"))
 
-	expected := 10
-	for i := 0; i < expected; i++ {
+	for i := 0; i < 10; i++ {
 		content, err := repo.ReadObject(ctx, blobID)
 		require.NoError(t, err)
 		require.Equal(t, "content", string(content))
 	}
 
-	if featureflag.LocalrepoReadObjectCached.IsEnabled(ctx) {
-		expected = 1
-	}
-	gitCmdFactory.RequireCommandCount(t, "cat-file", expected)
+	gitCmdFactory.RequireCommandCount(t, "cat-file", 1)
 }
 
 func TestRepo_ReadCommit(t *testing.T) {
diff --git a/internal/gitaly/service/repository/license_test.go b/internal/gitaly/service/repository/license_test.go
index 3688fd728..19f7c5b48 100644
--- a/internal/gitaly/service/repository/license_test.go
+++ b/internal/gitaly/service/repository/license_test.go
@@ -3,14 +3,11 @@
 package repository
 
 import (
-	"context"
 	"os"
 	"testing"
 
 	"github.com/go-enry/go-license-detector/v4/licensedb"
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
@@ -18,8 +15,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
-	"gitlab.com/gitlab-org/labkit/correlation"
-	"google.golang.org/grpc/metadata"
 )
 
 const (
@@ -344,41 +339,32 @@ func BenchmarkFindLicense(b *testing.B) {
 		gittest.WithTreeEntries(treeEntries...))
 	gittest.Exec(b, cfg, "-C", repoStressPath, "symbolic-ref", "HEAD", "refs/heads/main")
 
-	testhelper.NewFeatureSets(featureflag.LocalrepoReadObjectCached).Bench(b, func(b *testing.B, ctx context.Context) {
-		ctx = correlation.ContextWithCorrelation(ctx, "1")
-		ctx = testhelper.MergeOutgoingMetadata(ctx,
-			metadata.Pairs(catfile.SessionIDField, "1"),
-		)
-
-		for _, tc := range []struct {
-			desc string
-			repo *gitalypb.Repository
-		}{
-			{
-				desc: "gitlab-org/gitlab.git",
-				repo: repoGitLab,
-			},
-			{
-				desc: "stress.git",
-				repo: repoStress,
-			},
-		} {
-			// Preheat
-			_, err := client.FindLicense(ctx, &gitalypb.FindLicenseRequest{Repository: tc.repo})
-			require.NoError(b, err)
-			gitCmdFactory.ResetCount()
-
-			b.Run(tc.desc, func(b *testing.B) {
-				for i := 0; i < b.N; i++ {
-					resp, err := client.FindLicense(ctx, &gitalypb.FindLicenseRequest{Repository: tc.repo})
-					require.NoError(b, err)
-					require.Equal(b, "mit", resp.GetLicenseShortName())
-				}
-
-				if featureflag.LocalrepoReadObjectCached.IsEnabled(ctx) {
-					gitCmdFactory.RequireCommandCount(b, "cat-file", 0)
-				}
-			})
-		}
-	})
+	for _, tc := range []struct {
+		desc string
+		repo *gitalypb.Repository
+	}{
+		{
+			desc: "gitlab-org/gitlab.git",
+			repo: repoGitLab,
+		},
+		{
+			desc: "stress.git",
+			repo: repoStress,
+		},
+	} {
+		// Preheat
+		_, err := client.FindLicense(ctx, &gitalypb.FindLicenseRequest{Repository: tc.repo})
+		require.NoError(b, err)
+		gitCmdFactory.ResetCount()
+
+		b.Run(tc.desc, func(b *testing.B) {
+			for i := 0; i < b.N; i++ {
+				resp, err := client.FindLicense(ctx, &gitalypb.FindLicenseRequest{Repository: tc.repo})
+				require.NoError(b, err)
+				require.Equal(b, "mit", resp.GetLicenseShortName())
+			}
+
+			gitCmdFactory.RequireCommandCount(b, "cat-file", 0)
+		})
+	}
 }
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index c6132a2f6..71c7fb674 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -192,8 +192,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)
-	// Randomly enable the use of the catfile cache in localrepo.ReadObject.
-	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.LocalrepoReadObjectCached, rnd.Int()%2 == 0)
 
 	// Randomly enable either Git v2.40 or Git v2.41.
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.GitV241, rnd.Int()%2 == 0)