repository: Fix cleanups not running if there is no bitmap

When running `OptimizeRepository`, we're conditionally executing a full
repack of the git repository. This repack happens only in case where the
repository does not have a bitmap yet and where there isn't any
alternates file either. Otherwise, the repository has either been fully
repacked already in the past or it is part of an object pool, where
object pool members aren't supposed to have bitmaps anyway.

In case OptimizeRepository errors when stat'ing the alternates file
though, it will exit early. This early exit simply swallows the error,
which is probably fine. But we also skip other optimizations like
deletion of empty reference directories, which doesn't feel sensible.

Fix the issue by extracting the repack code into a separate function.
This allows us to easily swallow the error while still continuing with
the other cleanups.

1 files changed, 29 insertions, 15 deletions

diff --git a/internal/gitaly/service/repository/optimize.go b/internal/gitaly/service/repository/optimize.go
index 839442885..988cf47a0 100644
--- a/internal/gitaly/service/repository/optimize.go
+++ b/internal/gitaly/service/repository/optimize.go
@@ -107,32 +107,46 @@ func (s *server) removeRefEmptyDirs(ctx context.Context, repository *gitalypb.Re
 	return nil
 }
 
-func (s *server) optimizeRepository(ctx context.Context, repository *gitalypb.Repository) error {
+// 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 {
 	repoPath, err := s.locator.GetRepoPath(repository)
 	if err != nil {
 		return err
 	}
+
 	hasBitmap, err := stats.HasBitmap(repoPath)
 	if err != nil {
 		return helper.ErrInternal(err)
 	}
+	if hasBitmap {
+		return nil
+	}
 
-	if !hasBitmap {
-		altFile, err := s.locator.InfoAlternatesPath(repository)
-		if err != nil {
-			return helper.ErrInternal(err)
-		}
+	altFile, err := s.locator.InfoAlternatesPath(repository)
+	if err != nil {
+		return helper.ErrInternal(err)
+	}
 
-		// 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
-		}
+	// 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
+	}
 
-		_, err = s.RepackFull(ctx, &gitalypb.RepackFullRequest{Repository: repository, CreateBitmap: true})
-		if err != nil {
-			return err
-		}
+	if _, err = s.RepackFull(ctx, &gitalypb.RepackFullRequest{
+		Repository:   repository,
+		CreateBitmap: true,
+	}); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (s *server) optimizeRepository(ctx context.Context, repository *gitalypb.Repository) error {
+	if err := s.repackIfNoBitmap(ctx, repository); err != nil {
+		return fmt.Errorf("could not repack: %w", err)
 	}
 
 	if err := s.removeRefEmptyDirs(ctx, repository); err != nil {