repack: Create commit graph with Bloom filters

Existence of the commit graph improves performance of
some git operations. The more can be gained from the
commit graph with Bloom filters support. With this commit
we change repack to run if repository has no bitmap or
if the Bloom filters are missing.

Closes: https://gitlab.com/gitlab-org/gitaly/-/issues/3696

Changelog: added

2 files changed, 26 insertions, 9 deletions

diff --git a/internal/gitaly/service/repository/optimize.go b/internal/gitaly/service/repository/optimize.go
index 4394f5f5c..5776d0e47 100644
--- a/internal/gitaly/service/repository/optimize.go
+++ b/internal/gitaly/service/repository/optimize.go
@@ -11,9 +11,9 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
-// repackIfNoBitmap uses the bitmap index as a heuristic to determine whether the repository needs a
-// full repack. So only if there is none will the full repack be started.
-func (s *server) repackIfNoBitmap(ctx context.Context, repository *gitalypb.Repository) error {
+// repackIfNeeded uses a set of heuristics to determine whether the repository needs a
+// full repack and, if so, repacks it.
+func (s *server) repackIfNeeded(ctx context.Context, repository *gitalypb.Repository) error {
 	repoPath, err := s.locator.GetRepoPath(repository)
 	if err != nil {
 		return err
@@ -23,7 +23,13 @@ func (s *server) repackIfNoBitmap(ctx context.Context, repository *gitalypb.Repo
 	if err != nil {
 		return helper.ErrInternal(err)
 	}
-	if hasBitmap {
+
+	missingBloomFilters, err := stats.IsMissingBloomFilters(repoPath)
+	if err != nil {
+		return helper.ErrInternal(err)
+	}
+
+	if hasBitmap && !missingBloomFilters {
 		return nil
 	}
 
@@ -32,15 +38,17 @@ func (s *server) repackIfNoBitmap(ctx context.Context, repository *gitalypb.Repo
 		return helper.ErrInternal(err)
 	}
 
-	// repositories with alternates should never have a bitmap, as Git will otherwise complain about
+	// Repositories with alternates should never have a bitmap, as Git will otherwise complain about
 	// multiple bitmaps being present in parent and alternate repository.
-	if _, err = os.Stat(altFile); !os.IsNotExist(err) {
-		return nil
+	// In case of an error it still tries it is best to optimise the repository.
+	createBitMap := false
+	if _, err := os.Stat(altFile); os.IsNotExist(err) {
+		createBitMap = true
 	}
 
 	if _, err = s.RepackFull(ctx, &gitalypb.RepackFullRequest{
 		Repository:   repository,
-		CreateBitmap: true,
+		CreateBitmap: createBitMap,
 	}); err != nil {
 		return err
 	}
@@ -49,7 +57,7 @@ func (s *server) repackIfNoBitmap(ctx context.Context, repository *gitalypb.Repo
 }
 
 func (s *server) optimizeRepository(ctx context.Context, repository *gitalypb.Repository) error {
-	if err := s.repackIfNoBitmap(ctx, repository); err != nil {
+	if err := s.repackIfNeeded(ctx, repository); err != nil {
 		return fmt.Errorf("could not repack: %w", err)
 	}
 
diff --git a/internal/gitaly/service/repository/optimize_test.go b/internal/gitaly/service/repository/optimize_test.go
index f7ea72c63..66be3144c 100644
--- a/internal/gitaly/service/repository/optimize_test.go
+++ b/internal/gitaly/service/repository/optimize_test.go
@@ -43,6 +43,7 @@ func TestOptimizeRepository(t *testing.T) {
 	cfg, repoProto, repoPath, client := setupRepositoryService(t)
 
 	gittest.Exec(t, cfg, "-C", repoPath, "repack", "-A", "-b")
+	gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--size-multiple", "4", "--split", "replace", "--reachable", "--changed-paths")
 
 	ctx, cancel := testhelper.Context()
 	defer cancel()
@@ -51,6 +52,10 @@ func TestOptimizeRepository(t *testing.T) {
 	require.NoError(t, err)
 	require.True(t, hasBitmap, "expect a bitmap since we just repacked with -b")
 
+	missingBloomFilters, err := stats.IsMissingBloomFilters(repoPath)
+	require.NoError(t, err)
+	require.False(t, missingBloomFilters)
+
 	// get timestamp of latest packfile
 	newestsPackfileTime := getNewestPackfileModtime(t, repoPath)
 
@@ -118,6 +123,10 @@ func TestOptimizeRepository(t *testing.T) {
 	require.NoError(t, err)
 	require.NotEmpty(t, bitmaps)
 
+	missingBloomFilters, err = stats.IsMissingBloomFilters(testRepoPath)
+	require.NoError(t, err)
+	require.False(t, missingBloomFilters)
+
 	// Empty directories should exist because they're too recent.
 	require.DirExists(t, emptyRef)
 	require.DirExists(t, mrRefs)