git/housekeeping: Always enable writing cruft packs

In e4a9ded8e (housekeeping: Wire up logic to conditionally generate
cruft packs, 2023-03-03), we have wired up the logic to generate cruft
packs during repository housekeeping. Cruft packs are a rather recent
addition to Git: instead of keeping unreachable objects as loose objects
until they expire and get pruned, they are instead tracked in a separate
packfile. This both reduces the overhead of having to store the loose
objects as Git can make use of improved compression, and it speeds up
access times for objects as Git does not have to search through so many
loose objects anymore.

We have since rolled out this new feature to production successfully. So
let's remove the feature flag and thus unconditionally enable writing
cruft packs.

Changelog: changed

10 files changed, 26 insertions, 99 deletions

diff --git a/internal/git/housekeeping/optimization_strategy.go b/internal/git/housekeeping/optimization_strategy.go
index 878669ae0..79e0e7c53 100644
--- a/internal/git/housekeeping/optimization_strategy.go
+++ b/internal/git/housekeeping/optimization_strategy.go
@@ -6,7 +6,6 @@ import (
 	"time"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/stats"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 )
 
 // OptimizationStrategy is an interface to determine which parts of a repository should be
@@ -107,7 +106,7 @@ func (s HeuristicalOptimizationStrategy) ShouldRepackObjects(ctx context.Context
 		// Alternatively, we could enable writing cruft packs, but never expire the objects.
 		// This is left for another iteration though once we have determined that this is
 		// even necessary.
-		if featureflag.WriteCruftPacks.IsEnabled(ctx) && !s.info.IsObjectPool {
+		if !s.info.IsObjectPool {
 			cfg.Strategy = RepackObjectsStrategyFullWithCruft
 			cfg.CruftExpireBefore = s.expireBefore
 		}
@@ -280,7 +279,7 @@ func (s EagerOptimizationStrategy) ShouldRepackObjects(ctx context.Context) (boo
 
 	// Object pools should neither have unreachable objects, nor should we ever try to delete
 	// any if there are some. So we disable cruft packs and expiration of them for them.
-	if featureflag.WriteCruftPacks.IsEnabled(ctx) && !s.info.IsObjectPool {
+	if !s.info.IsObjectPool {
 		cfg.Strategy = RepackObjectsStrategyFullWithCruft
 		cfg.CruftExpireBefore = s.expireBefore
 	}
diff --git a/internal/git/housekeeping/optimization_strategy_test.go b/internal/git/housekeeping/optimization_strategy_test.go
index 5012827d6..14d389dd3 100644
--- a/internal/git/housekeeping/optimization_strategy_test.go
+++ b/internal/git/housekeeping/optimization_strategy_test.go
@@ -8,17 +8,13 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/stats"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
 func TestHeuristicalOptimizationStrategy_ShouldRepackObjects(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.WriteCruftPacks).Run(t, testHeuristicalOptimizationStrategyShouldRepackObjects)
-}
 
-func testHeuristicalOptimizationStrategyShouldRepackObjects(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	for _, tc := range []struct {
 		desc           string
@@ -209,7 +205,7 @@ func testHeuristicalOptimizationStrategyShouldRepackObjects(t *testing.T, ctx co
 					// the boundary of having to repack.
 					strategy.info.Packfiles.Count++
 
-					if featureflag.WriteCruftPacks.IsDisabled(ctx) || tc.isPool {
+					if tc.isPool {
 						expireBefore = time.Time{}
 					}
 
@@ -217,7 +213,7 @@ func testHeuristicalOptimizationStrategyShouldRepackObjects(t *testing.T, ctx co
 					require.True(t, repackNeeded)
 					require.Equal(t, RepackObjectsConfig{
 						Strategy: func() RepackObjectsStrategy {
-							if featureflag.WriteCruftPacks.IsEnabled(ctx) && !tc.isPool {
+							if !tc.isPool {
 								return RepackObjectsStrategyFullWithCruft
 							}
 							return RepackObjectsStrategyFullWithLooseUnreachable
@@ -447,11 +443,7 @@ func TestHeuristicalOptimizationStrategy_ShouldRepackReferences(t *testing.T) {
 func TestHeuristicalOptimizationStrategy_NeedsWriteCommitGraph(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.WriteCruftPacks).Run(t, testHeuristicalOptimizationStrategyNeedsWriteCommitGraph)
-}
-
-func testHeuristicalOptimizationStrategyNeedsWriteCommitGraph(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	for _, tc := range []struct {
 		desc           string
@@ -616,7 +608,7 @@ func testHeuristicalOptimizationStrategyNeedsWriteCommitGraph(t *testing.T, ctx
 			// a stale commit-graph. We thus need to replace the whole chain.
 			expectedNeeded: true,
 			expectedCfg: WriteCommitGraphConfig{
-				ReplaceChain: featureflag.WriteCruftPacks.IsEnabled(ctx),
+				ReplaceChain: true,
 			},
 		},
 	} {
@@ -630,12 +622,8 @@ func testHeuristicalOptimizationStrategyNeedsWriteCommitGraph(t *testing.T, ctx
 
 func TestEagerOptimizationStrategy(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.WriteCruftPacks).Run(t, testEagerOptimizationStrategy)
-}
-
-func testEagerOptimizationStrategy(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	expireBefore := time.Now()
 
 	for _, tc := range []struct {
@@ -696,7 +684,7 @@ func testEagerOptimizationStrategy(t *testing.T, ctx context.Context) {
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			var expectedExpireBefore time.Time
-			if featureflag.WriteCruftPacks.IsEnabled(ctx) && !tc.strategy.info.IsObjectPool {
+			if !tc.strategy.info.IsObjectPool {
 				expectedExpireBefore = expireBefore
 			}
 
@@ -704,7 +692,7 @@ func testEagerOptimizationStrategy(t *testing.T, ctx context.Context) {
 			require.True(t, shouldRepackObjects)
 			require.Equal(t, RepackObjectsConfig{
 				Strategy: func() RepackObjectsStrategy {
-					if featureflag.WriteCruftPacks.IsEnabled(ctx) && !tc.strategy.info.IsObjectPool {
+					if !tc.strategy.info.IsObjectPool {
 						return RepackObjectsStrategyFullWithCruft
 					}
 					return RepackObjectsStrategyFullWithLooseUnreachable
diff --git a/internal/git/housekeeping/optimize_repository_ext_test.go b/internal/git/housekeeping/optimize_repository_ext_test.go
index 87b25fce2..e43643691 100644
--- a/internal/git/housekeeping/optimize_repository_ext_test.go
+++ b/internal/git/housekeeping/optimize_repository_ext_test.go
@@ -1,7 +1,6 @@
 package housekeeping_test
 
 import (
-	"context"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -23,7 +22,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/setup"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
-	"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/gitaly/v15/internal/testhelper/testserver"
@@ -191,12 +189,8 @@ func TestPruneIfNeeded(t *testing.T) {
 
 func TestOptimizeRepository_objectPoolMember(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.WriteCruftPacks).Run(t, testOptimizeRepositoryObjectPoolMember)
-}
-
-func testOptimizeRepositoryObjectPoolMember(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 
 	txManager := transaction.NewManager(cfg, backchannel.NewRegistry())
@@ -204,13 +198,6 @@ func testOptimizeRepositoryObjectPoolMember(t *testing.T, ctx context.Context) {
 	catfileCache := catfile.NewCache(cfg)
 	defer catfileCache.Stop()
 
-	fullOrCruft := func() string {
-		if featureflag.WriteCruftPacks.IsEnabled(ctx) {
-			return "packed_objects_cruft"
-		}
-		return "packed_objects_full"
-	}()
-
 	for _, tc := range []struct {
 		desc                string
 		strategyConstructor housekeeping.OptimizationStrategyConstructor
@@ -223,8 +210,8 @@ func testOptimizeRepositoryObjectPoolMember(t *testing.T, ctx context.Context) {
 			},
 			expectedLogEntries: map[string]string{
 				"packed_refs":               "success",
-				fullOrCruft:                 "success",
 				"pruned_objects":            "success",
+				"packed_objects_cruft":      "success",
 				"written_commit_graph_full": "success",
 				"written_multi_pack_index":  "success",
 			},
diff --git a/internal/git/housekeeping/optimize_repository_test.go b/internal/git/housekeeping/optimize_repository_test.go
index 9fbed6210..842a5bd17 100644
--- a/internal/git/housekeeping/optimize_repository_test.go
+++ b/internal/git/housekeeping/optimize_repository_test.go
@@ -21,7 +21,6 @@ import (
 	gitalycfgprom "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
-	"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/gitaly/v15/proto/go/gitalypb"
@@ -223,12 +222,8 @@ func TestPackRefsIfNeeded(t *testing.T) {
 
 func TestOptimizeRepository(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.WriteCruftPacks).Run(t, testOptimizeRepository)
-}
-
-func testOptimizeRepository(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	txManager := transaction.NewManager(cfg, backchannel.NewRegistry())
 
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index ca5298213..2ee3a9b7e 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -1,7 +1,6 @@
 package objectpool
 
 import (
-	"context"
 	"fmt"
 	"path/filepath"
 	"strconv"
@@ -15,7 +14,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
@@ -107,12 +105,8 @@ func TestFetchFromOrigin_fsck(t *testing.T) {
 
 func TestFetchFromOrigin_deltaIslands(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.WriteCruftPacks).Run(t, testFetchFromOriginDeltaIslands)
-}
-
-func testFetchFromOriginDeltaIslands(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repo := setupObjectPool(t, ctx)
 	poolPath := gittest.RepositoryPath(t, pool)
 	repoPath := gittest.RepositoryPath(t, repo)
@@ -163,12 +157,8 @@ func TestFetchFromOrigin_bitmapHashCache(t *testing.T) {
 
 func TestFetchFromOrigin_refUpdates(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.WriteCruftPacks).Run(t, testFetchFromOriginRefUpdates)
-}
-
-func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repo := setupObjectPool(t, ctx)
 	repoPath, err := repo.Path()
 	require.NoError(t, err)
diff --git a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
index 27c7a0c48..df5dfe5ac 100644
--- a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
+++ b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
@@ -22,7 +22,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
-	"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/gitaly/v15/internal/testhelper/testserver"
@@ -39,12 +38,8 @@ import (
 
 func TestFetchIntoObjectPool_Success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.WriteCruftPacks).Run(t, testFetchIntoObjectPoolSuccess)
-}
-
-func testFetchIntoObjectPoolSuccess(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, _, client := setup(t, ctx)
 
 	parentID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
diff --git a/internal/gitaly/service/repository/optimize_test.go b/internal/gitaly/service/repository/optimize_test.go
index e965524e7..58652041e 100644
--- a/internal/gitaly/service/repository/optimize_test.go
+++ b/internal/gitaly/service/repository/optimize_test.go
@@ -20,7 +20,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
@@ -29,12 +28,8 @@ import (
 
 func TestOptimizeRepository(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.WriteCruftPacks).Run(t, testOptimizeRepository)
-}
-
-func testOptimizeRepository(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
 	t.Run("gitconfig credentials get pruned", func(t *testing.T) {
diff --git a/internal/gitaly/service/repository/prune_unreachable_objects.go b/internal/gitaly/service/repository/prune_unreachable_objects.go
index f19ea799c..3a8345634 100644
--- a/internal/gitaly/service/repository/prune_unreachable_objects.go
+++ b/internal/gitaly/service/repository/prune_unreachable_objects.go
@@ -8,7 +8,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/housekeeping"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
@@ -55,15 +54,13 @@ func (s *server) PruneUnreachableObjects(
 
 	// But we also have to prune unreachable objects part of cruft packs. The only way to do
 	// that is to do a full repack. So unfortunately, this is quite expensive.
-	if featureflag.WriteCruftPacks.IsEnabled(ctx) {
-		if err := housekeeping.RepackObjects(ctx, repo, housekeeping.RepackObjectsConfig{
-			Strategy:            housekeeping.RepackObjectsStrategyFullWithCruft,
-			WriteMultiPackIndex: true,
-			WriteBitmap:         len(repoInfo.Alternates) == 0,
-			CruftExpireBefore:   expireBefore,
-		}); err != nil {
-			return nil, structerr.NewInternal("repacking objects: %w", err)
-		}
+	if err := housekeeping.RepackObjects(ctx, repo, housekeeping.RepackObjectsConfig{
+		Strategy:            housekeeping.RepackObjectsStrategyFullWithCruft,
+		WriteMultiPackIndex: true,
+		WriteBitmap:         len(repoInfo.Alternates) == 0,
+		CruftExpireBefore:   expireBefore,
+	}); err != nil {
+		return nil, structerr.NewInternal("repacking objects: %w", err)
 	}
 
 	// Rewrite the commit-graph so that it doesn't contain references to pruned commits
diff --git a/internal/gitaly/service/repository/prune_unreachable_objects_test.go b/internal/gitaly/service/repository/prune_unreachable_objects_test.go
index dfcfe71bc..33f5d16e7 100644
--- a/internal/gitaly/service/repository/prune_unreachable_objects_test.go
+++ b/internal/gitaly/service/repository/prune_unreachable_objects_test.go
@@ -3,7 +3,6 @@
 package repository
 
 import (
-	"context"
 	"os"
 	"path/filepath"
 	"testing"
@@ -14,7 +13,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/stats"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
@@ -22,12 +20,8 @@ import (
 
 func TestPruneUnreachableObjects(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.WriteCruftPacks).Run(t, testPruneUnreachableObjects)
-}
-
-func testPruneUnreachableObjects(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
 	setObjectTime := func(t *testing.T, repoPath string, objectID git.ObjectID, when time.Time) {
@@ -236,11 +230,7 @@ func testPruneUnreachableObjects(t *testing.T, ctx context.Context) {
 
 		// The reachable commit should exist, but the unreachable one shouldn't.
 		gittest.RequireObjectExists(t, cfg, repoPath, reachableCommitID)
-		if featureflag.WriteCruftPacks.IsEnabled(ctx) {
-			gittest.RequireObjectNotExists(t, cfg, repoPath, unreachableCommitID)
-		} else {
-			gittest.RequireObjectExists(t, cfg, repoPath, unreachableCommitID)
-		}
+		gittest.RequireObjectNotExists(t, cfg, repoPath, unreachableCommitID)
 	})
 
 	t.Run("object pool", func(t *testing.T) {
diff --git a/internal/metadata/featureflag/ff_write_cruft_packs.go b/internal/metadata/featureflag/ff_write_cruft_packs.go
deleted file mode 100644
index 464c69faf..000000000
--- a/internal/metadata/featureflag/ff_write_cruft_packs.go
+++ /dev/null
@@ -1,9 +0,0 @@
-package featureflag
-
-// WriteCruftPacks enables writing cruft packs during repository housekeeping.
-var WriteCruftPacks = NewFeatureFlag(
-	"write_cruft_packs",
-	"v15.10.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4829",
-	false,
-)