Merge branch 'pks-housekeeping-skip-repacking-empty-repos' into 'master'

housekeeping: Skip repacking empty repositories

See merge request gitlab-org/gitaly!4438

3 files changed, 24 insertions, 23 deletions

diff --git a/internal/git/housekeeping/optimize_repository.go b/internal/git/housekeeping/optimize_repository.go
index f4fc8419e..39257fd96 100644
--- a/internal/git/housekeeping/optimize_repository.go
+++ b/internal/git/housekeeping/optimize_repository.go
@@ -138,6 +138,22 @@ func needsRepacking(repo *localrepo.Repo) (bool, RepackObjectsConfig, error) {
 		return false, RepackObjectsConfig{}, fmt.Errorf("getting repository path: %w", err)
 	}
 
+	largestPackfileSize, packfileCount, err := packfileSizeAndCount(repo)
+	if err != nil {
+		return false, RepackObjectsConfig{}, fmt.Errorf("checking largest packfile size: %w", err)
+	}
+
+	looseObjectCount, err := estimateLooseObjectCount(repo, time.Now())
+	if err != nil {
+		return false, RepackObjectsConfig{}, fmt.Errorf("estimating loose object count: %w", err)
+	}
+
+	// If there are neither packfiles nor loose objects in this repository then there is no need
+	// to repack anything.
+	if packfileCount == 0 && looseObjectCount == 0 {
+		return false, RepackObjectsConfig{}, nil
+	}
+
 	altFile, err := repo.InfoAlternatesPath()
 	if err != nil {
 		return false, RepackObjectsConfig{}, helper.ErrInternal(err)
@@ -190,11 +206,6 @@ func needsRepacking(repo *localrepo.Repo) (bool, RepackObjectsConfig, error) {
 		}, nil
 	}
 
-	largestPackfileSize, packfileCount, err := packfileSizeAndCount(repo)
-	if err != nil {
-		return false, RepackObjectsConfig{}, fmt.Errorf("checking largest packfile size: %w", err)
-	}
-
 	// Whenever we do an incremental repack we create a new packfile, and as a result Git may
 	// have to look into every one of the packfiles to find objects. This is less efficient the
 	// more packfiles we have, but we cannot repack the whole repository every time either given
@@ -229,11 +240,6 @@ func needsRepacking(repo *localrepo.Repo) (bool, RepackObjectsConfig, error) {
 		}, nil
 	}
 
-	looseObjectCount, err := estimateLooseObjectCount(repo, time.Now())
-	if err != nil {
-		return false, RepackObjectsConfig{}, fmt.Errorf("estimating loose object count: %w", err)
-	}
-
 	// Most Git commands do not write packfiles directly, but instead write loose objects into
 	// the object database. So while we now know that there ain't too many packfiles, we still
 	// need to check whether we have too many objects.
diff --git a/internal/git/housekeeping/optimize_repository_test.go b/internal/git/housekeeping/optimize_repository_test.go
index 086e3572e..8aad4dd8f 100644
--- a/internal/git/housekeeping/optimize_repository_test.go
+++ b/internal/git/housekeeping/optimize_repository_test.go
@@ -48,20 +48,11 @@ func TestNeedsRepacking(t *testing.T) {
 		expectedConfig RepackObjectsConfig
 	}{
 		{
-			desc: "empty repo",
+			desc: "empty repo does nothing",
 			setup: func(t *testing.T) *gitalypb.Repository {
 				repoProto, _ := gittest.InitRepo(t, cfg, cfg.Storages[0])
 				return repoProto
 			},
-			// This is a bug: if the repo is empty then we wouldn't ever generate a
-			// packfile, but we determine a repack is needed because it's missing a
-			// bitmap. It's a rather benign bug though: git-repack(1) will exit
-			// immediately because it knows that there's nothing to repack.
-			expectedNeeded: true,
-			expectedConfig: RepackObjectsConfig{
-				FullRepack:  true,
-				WriteBitmap: true,
-			},
 		},
 		{
 			desc: "missing bitmap",
@@ -510,15 +501,13 @@ func TestOptimizeRepository(t *testing.T) {
 		expectedMetrics string
 	}{
 		{
-			desc: "empty repository tries to write bitmap",
+			desc: "empty repository does nothing",
 			setup: func(t *testing.T) *gitalypb.Repository {
 				repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0])
 				return repo
 			},
 			expectedMetrics: `# HELP gitaly_housekeeping_tasks_total Total number of housekeeping tasks performed in the repository
 # TYPE gitaly_housekeeping_tasks_total counter
-gitaly_housekeeping_tasks_total{housekeeping_task="packed_objects_full", status="success"} 1
-gitaly_housekeeping_tasks_total{housekeeping_task="written_bitmap", status="success"} 1
 gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
 `,
 		},
diff --git a/internal/gitaly/service/repository/optimize_test.go b/internal/gitaly/service/repository/optimize_test.go
index 0bc1932d5..a843b5668 100644
--- a/internal/gitaly/service/repository/optimize_test.go
+++ b/internal/gitaly/service/repository/optimize_test.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
@@ -107,6 +108,11 @@ func TestOptimizeRepository(t *testing.T) {
 		)
 	}
 
+	// Write a blob whose OID is known to have a "17" prefix, which is required such that
+	// OptimizeRepository would try to repack at all.
+	blobOIDWith17Prefix := gittest.WriteBlob(t, cfg, testRepoPath, []byte("32"))
+	require.True(t, strings.HasPrefix(blobOIDWith17Prefix.String(), "17"))
+
 	bitmaps, err := filepath.Glob(filepath.Join(testRepoPath, "objects", "pack", "*.bitmap"))
 	require.NoError(t, err)
 	require.Empty(t, bitmaps)