Automatic merge of gitlab-org/gitaly master

7 files changed, 162 insertions, 132 deletions

diff --git a/internal/git/housekeeping/optimization_strategy.go b/internal/git/housekeeping/optimization_strategy.go
index c88be1661..8d26aff13 100644
--- a/internal/git/housekeeping/optimization_strategy.go
+++ b/internal/git/housekeeping/optimization_strategy.go
@@ -91,10 +91,12 @@ func NewHeuristicalOptimizationStrategy(ctx context.Context, repo *localrepo.Rep
 		strategy.hasBloomFilters = commitGraphInfo.HasBloomFilters
 	}
 
-	strategy.packfileSize, strategy.packfileCount, err = packfileSizeAndCount(repo)
+	packfilesInfo, err := stats.PackfilesInfoForRepository(repo)
 	if err != nil {
 		return strategy, fmt.Errorf("checking largest packfile size: %w", err)
 	}
+	strategy.packfileCount = packfilesInfo.Count
+	strategy.packfileSize = packfilesInfo.Size
 
 	strategy.looseObjectCount, err = countLooseObjects(repo, time.Now())
 	if err != nil {
@@ -209,47 +211,6 @@ func (s HeuristicalOptimizationStrategy) ShouldRepackObjects() (bool, RepackObje
 	return false, RepackObjectsConfig{}
 }
 
-func packfileSizeAndCount(repo *localrepo.Repo) (uint64, uint64, error) {
-	repoPath, err := repo.Path()
-	if err != nil {
-		return 0, 0, fmt.Errorf("getting repository path: %w", err)
-	}
-
-	entries, err := os.ReadDir(filepath.Join(repoPath, "objects", "pack"))
-	if err != nil {
-		if errors.Is(err, os.ErrNotExist) {
-			return 0, 0, nil
-		}
-
-		return 0, 0, err
-	}
-
-	var totalSize uint64
-	var count uint64
-
-	for _, entry := range entries {
-		if !strings.HasSuffix(entry.Name(), ".pack") {
-			continue
-		}
-
-		entryInfo, err := entry.Info()
-		if err != nil {
-			if errors.Is(err, os.ErrNotExist) {
-				continue
-			}
-
-			return 0, 0, fmt.Errorf("getting packfile info: %w", err)
-		}
-
-		count++
-		if entryInfo.Size() > 0 {
-			totalSize += uint64(entryInfo.Size())
-		}
-	}
-
-	return totalSize, count, nil
-}
-
 // countLooseObjects counts the number of loose objects in the repository. If a cutoff date is
 // given, then this function will only take into account objects which are older than the given
 // point in time.
diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 9b2e69e60..22f311a8c 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -18,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/transaction/voting"
 )
 
@@ -26,14 +25,8 @@ var objectPoolRefspec = fmt.Sprintf("+refs/*:%s/*", git.ObjectPoolRefNamespace)
 
 // FetchFromOrigin initializes the pool and fetches the objects from its origin repository
 func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *localrepo.Repo) error {
-	if featureflag.ObjectPoolDontInitOnFetch.IsDisabled(ctx) {
-		if err := o.Init(ctx); err != nil {
-			return fmt.Errorf("initializing object pool: %w", err)
-		}
-	} else {
-		if !o.Exists() {
-			return helper.ErrInvalidArgumentf("object pool does not exist")
-		}
+	if !o.Exists() {
+		return helper.ErrInvalidArgumentf("object pool does not exist")
 	}
 
 	originPath, err := origin.Path()
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index c0c4f17ed..35d3d412b 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -3,7 +3,6 @@
 package objectpool
 
 import (
-	"context"
 	"fmt"
 	"path/filepath"
 	"strconv"
@@ -16,18 +15,13 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"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/testhelper"
 )
 
 func TestFetchFromOrigin_dangling(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginDangling)
-}
-
-func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 	repoPath, err := repo.Path()
@@ -88,12 +82,8 @@ func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_fsck(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginFsck)
-}
-
-func testFetchFromOriginFsck(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -120,12 +110,8 @@ func testFetchFromOriginFsck(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_deltaIslands(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginDeltaIslands)
-}
-
-func testFetchFromOriginDeltaIslands(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -146,12 +132,8 @@ func testFetchFromOriginDeltaIslands(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_bitmapHashCache(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginBitmapHashCache)
-}
-
-func testFetchFromOriginBitmapHashCache(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -171,12 +153,8 @@ func testFetchFromOriginBitmapHashCache(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_refUpdates(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginRefUpdates)
-}
-
-func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -225,12 +203,8 @@ func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_refs(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginRefs)
-}
-
-func testFetchFromOriginRefs(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	// Initialize the object pool and verify that it ain't yet got any references.
@@ -271,21 +245,11 @@ func testFetchFromOriginRefs(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_missingPool(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginMissingPool)
-}
-
-func testFetchFromOriginMissingPool(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
-	err := pool.FetchFromOrigin(ctx, repo)
-	if featureflag.ObjectPoolDontInitOnFetch.IsEnabled(ctx) {
-		require.Equal(t, helper.ErrInvalidArgumentf("object pool does not exist"), err)
-		require.False(t, pool.Exists())
-	} else {
-		require.NoError(t, err)
-		require.True(t, pool.Exists())
-	}
+	require.Equal(t, helper.ErrInvalidArgumentf("object pool does not exist"), pool.FetchFromOrigin(ctx, repo))
+	require.False(t, pool.Exists())
 }
diff --git a/internal/git/stats/objects_info.go b/internal/git/stats/objects_info.go
index 66df37fde..eedd8316d 100644
--- a/internal/git/stats/objects_info.go
+++ b/internal/git/stats/objects_info.go
@@ -3,6 +3,7 @@ package stats
 import (
 	"bufio"
 	"context"
+	"errors"
 	"fmt"
 	"io/fs"
 	"os"
@@ -187,3 +188,68 @@ func ObjectsInfoForRepository(ctx context.Context, repo *localrepo.Repo) (Object
 
 	return info, nil
 }
+
+// PackfilesInfo contains information about packfiles.
+type PackfilesInfo struct {
+	// Count is the number of loose objects, including stale ones.
+	Count uint64 `json:"count"`
+	// Size is the total size of all loose objects in bytes, including stale ones.
+	Size uint64 `json:"size"`
+	// GarbageCount is the number of garbage files.
+	GarbageCount uint64 `json:"garbage_count"`
+	// GarbageSize is the total size of all garbage files in bytes.
+	GarbageSize uint64 `json:"garbage_size"`
+}
+
+// PackfilesInfoForRepository derives various information about packfiles for the given repository.
+func PackfilesInfoForRepository(repo *localrepo.Repo) (PackfilesInfo, error) {
+	repoPath, err := repo.Path()
+	if err != nil {
+		return PackfilesInfo{}, fmt.Errorf("getting repository path: %w", err)
+	}
+
+	entries, err := os.ReadDir(filepath.Join(repoPath, "objects", "pack"))
+	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			return PackfilesInfo{}, nil
+		}
+
+		return PackfilesInfo{}, err
+	}
+
+	var info PackfilesInfo
+	for _, entry := range entries {
+		entryInfo, err := entry.Info()
+		if err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				continue
+			}
+
+			return PackfilesInfo{}, fmt.Errorf("getting packfile info: %w", err)
+		}
+
+		// We're overly lenient here and only verify for known prefixes. This would already
+		// catch things like temporary packfiles, but it wouldn't catch other bogus files.
+		// This is on purpose though because Git has grown more and more metadata-style file
+		// formats, and we don't want to copy the list here.
+		if !strings.HasPrefix(entry.Name(), "pack-") {
+			info.GarbageCount++
+			if entryInfo.Size() > 0 {
+				info.GarbageSize += uint64(entryInfo.Size())
+			}
+
+			continue
+		}
+
+		if !strings.HasSuffix(entry.Name(), ".pack") {
+			continue
+		}
+
+		info.Count++
+		if entryInfo.Size() > 0 {
+			info.Size += uint64(entryInfo.Size())
+		}
+	}
+
+	return info, nil
+}
diff --git a/internal/git/stats/objects_info_test.go b/internal/git/stats/objects_info_test.go
index 2a0c5a674..85404477b 100644
--- a/internal/git/stats/objects_info_test.go
+++ b/internal/git/stats/objects_info_test.go
@@ -325,3 +325,80 @@ func TestObjectsInfoForRepository(t *testing.T) {
 		})
 	}
 }
+
+func TestPackfileInfoForRepository(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+	cfg := testcfg.Build(t)
+
+	createRepo := func(t *testing.T) (*localrepo.Repo, string) {
+		repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
+			SkipCreationViaService: true,
+		})
+		return localrepo.NewTestRepo(t, cfg, repoProto), repoPath
+	}
+
+	requirePackfilesInfo := func(t *testing.T, repo *localrepo.Repo, expectedInfo PackfilesInfo) {
+		info, err := PackfilesInfoForRepository(repo)
+		require.NoError(t, err)
+		require.Equal(t, expectedInfo, info)
+	}
+
+	t.Run("empty repository", func(t *testing.T) {
+		repo, _ := createRepo(t)
+		requirePackfilesInfo(t, repo, PackfilesInfo{})
+	})
+
+	t.Run("single packfile", func(t *testing.T) {
+		repo, repoPath := createRepo(t)
+
+		packfileDir := filepath.Join(repoPath, "objects", "pack")
+		require.NoError(t, os.MkdirAll(packfileDir, 0o755))
+		require.NoError(t, os.WriteFile(filepath.Join(packfileDir, "pack-foo.pack"), []byte("foobar"), 0o644))
+
+		requirePackfilesInfo(t, repo, PackfilesInfo{
+			Count: 1,
+			Size:  6,
+		})
+	})
+
+	t.Run("multiple packfiles", func(t *testing.T) {
+		repo, repoPath := createRepo(t)
+
+		packfileDir := filepath.Join(repoPath, "objects", "pack")
+		require.NoError(t, os.MkdirAll(packfileDir, 0o755))
+		require.NoError(t, os.WriteFile(filepath.Join(packfileDir, "pack-foo.pack"), []byte("foobar"), 0o644))
+		require.NoError(t, os.WriteFile(filepath.Join(packfileDir, "pack-bar.pack"), []byte("123"), 0o644))
+
+		requirePackfilesInfo(t, repo, PackfilesInfo{
+			Count: 2,
+			Size:  9,
+		})
+	})
+
+	t.Run("multiple packfiles with other data structures", func(t *testing.T) {
+		repo, repoPath := createRepo(t)
+
+		packfileDir := filepath.Join(repoPath, "objects", "pack")
+		require.NoError(t, os.MkdirAll(packfileDir, 0o755))
+		for _, file := range []string{
+			"pack-bar.bar",
+			"pack-bar.pack",
+			"pack-bar.idx",
+			"pack-foo.bar",
+			"pack-foo.pack",
+			"pack-foo.idx",
+			"garbage",
+		} {
+			require.NoError(t, os.WriteFile(filepath.Join(packfileDir, file), []byte("1"), 0o644))
+		}
+
+		requirePackfilesInfo(t, repo, PackfilesInfo{
+			Count:        2,
+			Size:         2,
+			GarbageCount: 1,
+			GarbageSize:  1,
+		})
+	})
+}
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 2ac4d8208..8ec6f5624 100644
--- a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
+++ b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
@@ -24,7 +24,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"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"
@@ -40,12 +39,8 @@ import (
 
 func TestFetchIntoObjectPool_Success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).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"))
@@ -102,11 +97,6 @@ func testFetchIntoObjectPoolsuccess(t *testing.T, ctx context.Context) {
 
 func TestFetchIntoObjectPool_transactional(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolTransactional)
-}
-
-func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
 	var votes []voting.Vote
 	var votesMutex sync.Mutex
@@ -119,6 +109,7 @@ func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
 		},
 	}
 
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	cfg.SocketPath = runObjectPoolServer(
 		t, cfg, config.NewLocator(cfg),
@@ -230,12 +221,8 @@ func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
 
 func TestFetchIntoObjectPool_CollectLogStatistics(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolcollectLogStatistics)
-}
-
-func testFetchIntoObjectPoolcollectLogStatistics(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	testcfg.BuildGitalyHooks(t, cfg)
 
@@ -280,12 +267,8 @@ func testFetchIntoObjectPoolcollectLogStatistics(t *testing.T, ctx context.Conte
 
 func TestFetchIntoObjectPool_Failure(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolfailure)
-}
-
-func testFetchIntoObjectPoolfailure(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfgBuilder := testcfg.NewGitalyCfgBuilder()
 	cfg, repos := cfgBuilder.BuildWithRepoAt(t, t.Name())
 
@@ -352,12 +335,8 @@ func testFetchIntoObjectPoolfailure(t *testing.T, ctx context.Context) {
 
 func TestFetchIntoObjectPool_dfConflict(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolDfConflict)
-}
-
-func testFetchIntoObjectPoolDfConflict(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, _, client := setup(t, ctx)
 
 	pool := initObjectPool(t, cfg, cfg.Storages[0])
diff --git a/internal/metadata/featureflag/ff_object_pool_dont_init_on_fetch.go b/internal/metadata/featureflag/ff_object_pool_dont_init_on_fetch.go
deleted file mode 100644
index aa7bf57ca..000000000
--- a/internal/metadata/featureflag/ff_object_pool_dont_init_on_fetch.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// ObjectPoolDontInitOnFetch will cause FetchIntoObjectPool to not initialize the object pool when
-// it does not yet exist.
-var ObjectPoolDontInitOnFetch = NewFeatureFlag(
-	"object_pool_dont_init_on_fetch",
-	"v15.6.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4560",
-	false,
-)