housekeeping: Convert function to check for pool repos to be typesafe

The `IsPoolPath()` function can be used to check whether a given path
belongs to a pool repository or not. It is not quite clear though what
kind of parameter the function expects: is it a relative path, or is it
the full path to the object pool? This is easy to get wrong and may have
fatal consequences.

Change the interface to instead accept a `repository.GitRepo` to clarify
this.

8 files changed, 86 insertions, 47 deletions

diff --git a/internal/git/housekeeping/object_pool.go b/internal/git/housekeeping/object_pool.go
index 6af1a9973..732045e09 100644
--- a/internal/git/housekeeping/object_pool.go
+++ b/internal/git/housekeeping/object_pool.go
@@ -4,15 +4,16 @@ import (
 	"regexp"
 	"strings"
 
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/praefectutil"
 )
 
 // railsPoolDirRegexp is used to validate object pool directory structure and name as generated by Rails.
 var railsPoolDirRegexp = regexp.MustCompile(`^@pools/([0-9a-f]{2})/([0-9a-f]{2})/([0-9a-f]{64})\.git$`)
 
-// IsRailsPoolPath returns whether the relative path indicates this is a pool path generated by Rails.
-func IsRailsPoolPath(relativePath string) bool {
-	matches := railsPoolDirRegexp.FindStringSubmatch(relativePath)
+// IsRailsPoolRepository returns whether the repository is a pool repository generated by Rails.
+func IsRailsPoolRepository(repo repository.GitRepo) bool {
+	matches := railsPoolDirRegexp.FindStringSubmatch(repo.GetRelativePath())
 	if matches == nil || !strings.HasPrefix(matches[3], matches[1]+matches[2]) {
 		return false
 	}
@@ -20,8 +21,7 @@ func IsRailsPoolPath(relativePath string) bool {
 	return true
 }
 
-// IsPoolPath returns whether the relative path indicates the repository is an object
-// pool.
-func IsPoolPath(relativePath string) bool {
-	return IsRailsPoolPath(relativePath) || praefectutil.IsPoolPath(relativePath)
+// IsPoolRepository returns whether the repository is an object pool.
+func IsPoolRepository(repo repository.GitRepo) bool {
+	return IsRailsPoolRepository(repo) || praefectutil.IsPoolRepository(repo)
 }
diff --git a/internal/git/housekeeping/object_pool_test.go b/internal/git/housekeeping/object_pool_test.go
index 7c0bf5422..daf134505 100644
--- a/internal/git/housekeeping/object_pool_test.go
+++ b/internal/git/housekeeping/object_pool_test.go
@@ -6,42 +6,57 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/praefectutil"
+	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
 
-func TestIsPoolPath(t *testing.T) {
+func TestIsPoolRepository(t *testing.T) {
 	for _, tc := range []struct {
-		desc         string
-		relativePath string
-		isPoolPath   bool
+		desc       string
+		repo       *gitalypb.Repository
+		isPoolPath bool
 	}{
 		{
-			desc:         "rails pool directory",
-			relativePath: gittest.NewObjectPoolName(t),
-			isPoolPath:   true,
+			desc: "rails pool directory",
+			repo: &gitalypb.Repository{
+				RelativePath: gittest.NewObjectPoolName(t),
+			},
+			isPoolPath: true,
 		},
 		{
-			desc:         "praefect pool path",
-			relativePath: praefectutil.DerivePoolPath(1),
-			isPoolPath:   true,
+			desc: "praefect pool path",
+			repo: &gitalypb.Repository{
+				RelativePath: praefectutil.DerivePoolPath(1),
+			},
+			isPoolPath: true,
 		},
 		{
-			desc:         "praefect replica path",
-			relativePath: praefectutil.DeriveReplicaPath(1),
+			desc: "praefect replica path",
+			repo: &gitalypb.Repository{
+				RelativePath: praefectutil.DeriveReplicaPath(1),
+			},
 		},
 		{
-			desc: "empty string",
+			desc: "missing repository",
 		},
 		{
-			desc:         "rails path first to subdirs dont match full hash",
-			relativePath: "@pools/aa/bb/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff.git",
+			desc: "empty repository",
+			repo: &gitalypb.Repository{},
 		},
 		{
-			desc:         "normal repos dont match",
-			relativePath: "@hashed/" + gittest.NewRepositoryName(t, true),
+			desc: "rails path first to subdirs dont match full hash",
+			repo: &gitalypb.Repository{
+				RelativePath: "@pools/aa/bb/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff.git",
+			},
+		},
+		{
+			desc: "normal repos dont match",
+			repo: &gitalypb.Repository{
+				RelativePath: "@hashed/" + gittest.NewRepositoryName(t, true),
+			},
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			require.Equal(t, tc.isPoolPath, IsPoolPath(tc.relativePath))
+			require.Equal(t, tc.isPoolPath, IsPoolRepository(tc.repo))
 		})
 	}
 }
diff --git a/internal/git/housekeeping/optimize_repository.go b/internal/git/housekeeping/optimize_repository.go
index 0d79db70e..eedc1571f 100644
--- a/internal/git/housekeeping/optimize_repository.go
+++ b/internal/git/housekeeping/optimize_repository.go
@@ -359,7 +359,7 @@ func estimateLooseObjectCount(repo *localrepo.Repo, cutoffDate time.Time) (int64
 func pruneIfNeeded(ctx context.Context, repo *localrepo.Repo) (bool, error) {
 	// Pool repositories must never prune any objects, or otherwise we may corrupt members of
 	// that pool if they still refer to that object.
-	if IsPoolPath(repo.GetRelativePath()) {
+	if IsPoolRepository(repo) {
 		return false, nil
 	}
 
diff --git a/internal/git/objectpool/pool.go b/internal/git/objectpool/pool.go
index 1a454e67e..d1676c2af 100644
--- a/internal/git/objectpool/pool.go
+++ b/internal/git/objectpool/pool.go
@@ -57,10 +57,6 @@ func NewObjectPool(
 		return nil, err
 	}
 
-	if !housekeeping.IsPoolPath(relativePath) {
-		return nil, ErrInvalidPoolDir
-	}
-
 	pool := &ObjectPool{
 		gitCmdFactory:       gitCmdFactory,
 		txManager:           txManager,
@@ -71,6 +67,10 @@ func NewObjectPool(
 	}
 	pool.Repo = localrepo.New(locator, gitCmdFactory, catfileCache, pool)
 
+	if !housekeeping.IsPoolRepository(pool) {
+		return nil, ErrInvalidPoolDir
+	}
+
 	return pool, nil
 }
 
diff --git a/internal/praefect/delete_object_pool.go b/internal/praefect/delete_object_pool.go
index 83d8ab9dd..c10449d42 100644
--- a/internal/praefect/delete_object_pool.go
+++ b/internal/praefect/delete_object_pool.go
@@ -29,7 +29,7 @@ func DeleteObjectPoolHandler(rs datastore.RepositoryStore, conns Connections) gr
 				return nil, err
 			}
 
-			if !housekeeping.IsRailsPoolPath(repo.GetRelativePath()) {
+			if !housekeeping.IsRailsPoolRepository(repo) {
 				return nil, helper.ErrInvalidArgument(objectpool.ErrInvalidPoolDir)
 			}
 
diff --git a/internal/praefect/praefectutil/replica_path.go b/internal/praefect/praefectutil/replica_path.go
index 576762d22..1bf34b33b 100644
--- a/internal/praefect/praefectutil/replica_path.go
+++ b/internal/praefect/praefectutil/replica_path.go
@@ -6,14 +6,16 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
+
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 )
 
 // poolPathPrefix is the prefix directory where Praefect places object pools.
 const poolPathPrefix = "@cluster/pools/"
 
-// IsPoolPath returns whether the relative path indicates this is a Praefect generated object pool path.
-func IsPoolPath(relativePath string) bool {
-	return strings.HasPrefix(relativePath, poolPathPrefix)
+// IsPoolRepository returns whether the repository is a Praefect generated object pool repository.
+func IsPoolRepository(repo repository.GitRepo) bool {
+	return strings.HasPrefix(repo.GetRelativePath(), poolPathPrefix)
 }
 
 // DeriveReplicaPath derives a repository's disk storage path from its repository ID. The repository ID
diff --git a/internal/praefect/praefectutil/replica_path_test.go b/internal/praefect/praefectutil/replica_path_test.go
index be9f42e24..d168778c1 100644
--- a/internal/praefect/praefectutil/replica_path_test.go
+++ b/internal/praefect/praefectutil/replica_path_test.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
+	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
 
 func TestDeriveReplicaPath(t *testing.T) {
@@ -17,28 +18,45 @@ func TestDerivePoolPath(t *testing.T) {
 	require.Equal(t, "@cluster/pools/d4/73/2", DerivePoolPath(2))
 }
 
-func TestIsPoolPath(t *testing.T) {
+func TestIsPoolRepository(t *testing.T) {
 	for _, tc := range []struct {
-		desc         string
-		relativePath string
-		isPoolPath   bool
+		desc       string
+		repo       *gitalypb.Repository
+		isPoolPath bool
 	}{
 		{
-			desc:         "praefect pool path",
-			relativePath: DerivePoolPath(1),
-			isPoolPath:   true,
+			desc:       "missing repository",
+			isPoolPath: false,
 		},
 		{
-			desc:         "praefect replica path",
-			relativePath: DeriveReplicaPath(1),
+			desc: "empty string",
+			repo: &gitalypb.Repository{
+				RelativePath: "",
+			},
+			isPoolPath: false,
 		},
 		{
-			desc:         "rails pool path",
-			relativePath: gittest.NewObjectPoolName(t),
+			desc: "praefect pool path",
+			repo: &gitalypb.Repository{
+				RelativePath: DerivePoolPath(1),
+			},
+			isPoolPath: true,
+		},
+		{
+			desc: "praefect replica path",
+			repo: &gitalypb.Repository{
+				RelativePath: DeriveReplicaPath(1),
+			},
+		},
+		{
+			desc: "rails pool path",
+			repo: &gitalypb.Repository{
+				RelativePath: gittest.NewObjectPoolName(t),
+			},
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			require.Equal(t, tc.isPoolPath, IsPoolPath(tc.relativePath))
+			require.Equal(t, tc.isPoolPath, IsPoolRepository(tc.repo))
 		})
 	}
 }
diff --git a/internal/praefect/router_per_repository.go b/internal/praefect/router_per_repository.go
index ea3d60118..287d8d33b 100644
--- a/internal/praefect/router_per_repository.go
+++ b/internal/praefect/router_per_repository.go
@@ -10,6 +10,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/nodes"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/praefectutil"
+	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc"
 )
 
@@ -313,7 +314,10 @@ func (r *PerRepositoryRouter) RouteRepositoryCreation(ctx context.Context, virtu
 	replicaPath := relativePath
 	if featureflag.PraefectGeneratedReplicaPaths.IsEnabled(ctx) {
 		replicaPath = praefectutil.DeriveReplicaPath(id)
-		if housekeeping.IsRailsPoolPath(relativePath) {
+		if housekeeping.IsRailsPoolRepository(&gitalypb.Repository{
+			StorageName:  virtualStorage,
+			RelativePath: relativePath,
+		}) {
 			replicaPath = praefectutil.DerivePoolPath(id)
 		}
 	}