Merge branch 'pks-objectpool-more-fixes' into 'master'

objectpool: More test refactorings

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4973

Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

7 files changed, 197 insertions, 241 deletions

diff --git a/internal/git/gittest/repo.go b/internal/git/gittest/repo.go
index c5d517ffa..162183d54 100644
--- a/internal/git/gittest/repo.go
+++ b/internal/git/gittest/repo.go
@@ -327,37 +327,3 @@ func AddWorktreeArgs(repoPath, worktreeName string) []string {
 func AddWorktree(tb testing.TB, cfg config.Cfg, repoPath string, worktreeName string) {
 	Exec(tb, cfg, AddWorktreeArgs(repoPath, worktreeName)...)
 }
-
-// FixGitLabTestRepoForCommitGraphs fixes the "gitlab-test.git" repository so that it can be used in
-// the context of commit-graphs. The test repository contains the commit ba3343b (Weird commit date,
-// 292278994-08-17). As you can already see, this commit has a commit year of 292278994, which is
-// not exactly a realistic commit date to have in normal repositories. Unfortunately, this commit
-// date causes commit-graphs to become corrupt with the following error that's likely caused by
-// an overflow:
-//
-//	commit date for commit ba3343bc4fa403a8dfbfcab7fc1a8c29ee34bd69 in commit-graph is 15668040695 != 9223372036854775
-//
-// This is not a new error, but something that has existed for quite a while already in Git. And
-// while the bug can also be easily hit in Gitaly because we do write commit-graphs in pool
-// repositories, until now we haven't because we never exercised this.
-//
-// Unfortunately, we're between a rock and a hard place: this error will be hit when running
-// git-fsck(1) to find dangling objects, which we do to rescue objects. git-fsck(1) will by default
-// verify the commit-graphs to be consistent even  with `--connectivity-only`, which causes the
-// error. But while we could in theory just disable the usage of commit-graphs by passing
-// `core.commitGraph=0`, the end result would be that the connectivity check itself may become a lot
-// slower.
-//
-// So for now we just bail on this whole topic: it's not a new bug and we can't do much about it
-// given it could regress performance. The pool members would be broken in the same way, even though
-// less visibly so because we don't git-fsck(1) in "normal" RPCs. But to make our tests work we
-// delete the reference for this specific commit so that it doesn't cause our tests to break.
-//
-// You can easily test whether this bug still exists via the following commands:
-//
-//	$ git clone _build/testrepos/gitlab-test.git
-//	$ git -C gitlab-test commit-graph write
-//	$ git -C gitlab-test commit-graph verify
-func FixGitLabTestRepoForCommitGraphs(tb testing.TB, cfg config.Cfg, repoPath string) {
-	Exec(tb, cfg, "-C", repoPath, "update-ref", "-d", "refs/heads/spooky-stuff", "ba3343bc4fa403a8dfbfcab7fc1a8c29ee34bd69")
-}
diff --git a/internal/git/gittest/tag.go b/internal/git/gittest/tag.go
index a5691b3f4..2a8f0e865 100644
--- a/internal/git/gittest/tag.go
+++ b/internal/git/gittest/tag.go
@@ -32,6 +32,8 @@ func WriteTag(
 	targetRevision git.Revision,
 	optionalConfig ...WriteTagConfig,
 ) git.ObjectID {
+	tb.Helper()
+
 	require.Less(tb, len(optionalConfig), 2, "only a single config may be passed")
 
 	var config WriteTagConfig
diff --git a/internal/git/objectpool/clone_test.go b/internal/git/objectpool/clone_test.go
index 4c6d5b0c0..5fc6d84cc 100644
--- a/internal/git/objectpool/clone_test.go
+++ b/internal/git/objectpool/clone_test.go
@@ -66,7 +66,7 @@ func TestClone_fsck(t *testing.T) {
 
 	gittest.WriteCommit(t, cfg, repoPath,
 		gittest.WithParents(),
-		gittest.WithBranch("main"),
+		gittest.WithBranch("master"),
 		gittest.WithTree(treeID),
 	)
 
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index 8874243f5..c0c4f17ed 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -5,7 +5,6 @@ package objectpool
 import (
 	"context"
 	"fmt"
-	"os"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -15,7 +14,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"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"
@@ -32,70 +30,60 @@ func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
-
-	require.NoError(t, pool.Init(ctx))
-	require.NoError(t, pool.FetchFromOrigin(ctx, repo), "seed pool")
-
-	const (
-		existingTree   = "07f8147e8e73aab6c935c296e8cdc5194dee729b"
-		existingCommit = "7975be0116940bf2ad4321f79d02a55c5f7779aa"
-		existingBlob   = "c60514b6d3d6bf4bec1030f70026e34dfbd69ad5"
-	)
-
-	// We want to have some objects that are guaranteed to be dangling. Use
-	// random data to make each object unique.
-	nonce, err := text.RandomHex(4)
+	repoPath, err := repo.Path()
 	require.NoError(t, err)
 
-	// A blob with random contents should be unique.
-	newBlob := gittest.WriteBlob(t, cfg, pool.FullPath(), []byte(nonce))
-
-	// A tree with a randomly named blob entry should be unique.
-	newTree := gittest.WriteTree(t, cfg, pool.FullPath(), []gittest.TreeEntry{
-		{Mode: "100644", OID: git.ObjectID(existingBlob), Path: nonce},
+	// Write some reachable objects into the object pool member and fetch them into the pool.
+	blobID := gittest.WriteBlob(t, cfg, repoPath, []byte("contents"))
+	treeID := gittest.WriteTree(t, cfg, repoPath, []gittest.TreeEntry{
+		{Mode: "100644", OID: blobID, Path: "reachable"},
 	})
-
-	// A commit with a random message should be unique.
-	newCommit := gittest.WriteCommit(t, cfg, pool.FullPath(),
-		gittest.WithTreeEntries(gittest.TreeEntry{
-			OID: git.ObjectID(existingTree), Path: nonce, Mode: "040000",
-		}),
+	commitID := gittest.WriteCommit(t, cfg, repoPath,
+		gittest.WithTree(treeID),
+		gittest.WithBranch("master"),
 	)
+	require.NoError(t, pool.Init(ctx))
+	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 
-	// A tag with random hex characters in its name should be unique.
-	newTagName := "tag-" + nonce
-	newTag := gittest.WriteTag(t, cfg, pool.FullPath(), newTagName, existingCommit, gittest.WriteTagConfig{
-		Message: "msg",
+	// We now write a bunch of objects into the object pool that are not referenced by anything.
+	// These are thus "dangling".
+	unreachableBlob := gittest.WriteBlob(t, cfg, pool.FullPath(), []byte("unreachable"))
+	unreachableTree := gittest.WriteTree(t, cfg, pool.FullPath(), []gittest.TreeEntry{
+		{Mode: "100644", OID: blobID, Path: "unreachable"},
 	})
+	unreachableCommit := gittest.WriteCommit(t, cfg, pool.FullPath(),
+		gittest.WithMessage("unreachable"),
+		gittest.WithTree(treeID),
+	)
+	unreachableTag := gittest.WriteTag(t, cfg, pool.FullPath(), "unreachable", commitID.Revision(), gittest.WriteTagConfig{
+		Message: "unreachable",
+	})
+	// `WriteTag()` automatically creates a reference and thus makes the annotated tag
+	// reachable. We thus delete the reference here again.
+	gittest.Exec(t, cfg, "-C", pool.FullPath(), "update-ref", "-d", "refs/tags/unreachable")
 
-	// `git tag` automatically creates a ref, so our new tag is not dangling.
-	// Deleting the ref should fix that.
-	gittest.Exec(t, cfg, "-C", pool.FullPath(), "update-ref", "-d", "refs/tags/"+newTagName)
-
+	// git-fsck(1) should report the newly created unreachable objects as dangling.
 	fsckBefore := gittest.Exec(t, cfg, "-C", pool.FullPath(), "fsck", "--connectivity-only", "--dangling")
-	fsckBeforeLines := strings.Split(string(fsckBefore), "\n")
-
-	for _, l := range []string{
-		fmt.Sprintf("dangling blob %s", newBlob),
-		fmt.Sprintf("dangling tree %s", newTree),
-		fmt.Sprintf("dangling commit %s", newCommit),
-		fmt.Sprintf("dangling tag %s", newTag),
-	} {
-		require.Contains(t, fsckBeforeLines, l, "test setup sanity check")
-	}
-
-	// We expect this second run to convert the dangling objects into
-	// non-dangling objects.
-	require.NoError(t, pool.FetchFromOrigin(ctx, repo), "second fetch")
-
-	refsAfter := gittest.Exec(t, cfg, "-C", pool.FullPath(), "for-each-ref", "--format=%(refname) %(objectname)")
-	refsAfterLines := strings.Split(string(refsAfter), "\n")
-	for _, id := range []git.ObjectID{newBlob, newTree, newCommit, newTag} {
-		require.Contains(t, refsAfterLines, fmt.Sprintf("refs/dangling/%s %s", id, id))
-	}
+	require.Equal(t, strings.Join([]string{
+		fmt.Sprintf("dangling blob %s", unreachableBlob),
+		fmt.Sprintf("dangling tag %s", unreachableTag),
+		fmt.Sprintf("dangling commit %s", unreachableCommit),
+		fmt.Sprintf("dangling tree %s", unreachableTree),
+	}, "\n"), text.ChompBytes(fsckBefore))
+
+	// We expect this second run to convert the dangling objects into non-dangling objects.
+	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 
-	require.NoFileExists(t, filepath.Join(pool.FullPath(), "info", "refs"))
-	require.NoFileExists(t, filepath.Join(pool.FullPath(), "objects", "info", "packs"))
+	// Each of the dangling objects should have gotten a new dangling reference.
+	danglingRefs := gittest.Exec(t, cfg, "-C", pool.FullPath(), "for-each-ref", "--format=%(refname) %(objectname)", "refs/dangling/")
+	require.Equal(t, strings.Join([]string{
+		fmt.Sprintf("refs/dangling/%[1]s %[1]s", unreachableBlob),
+		fmt.Sprintf("refs/dangling/%[1]s %[1]s", unreachableTree),
+		fmt.Sprintf("refs/dangling/%[1]s %[1]s", unreachableTag),
+		fmt.Sprintf("refs/dangling/%[1]s %[1]s", unreachableCommit),
+	}, "\n"), text.ChompBytes(danglingRefs))
+	// And git-fsck(1) shouldn't report the objects as dangling anymore.
+	require.Empty(t, gittest.Exec(t, cfg, "-C", pool.FullPath(), "fsck", "--connectivity-only", "--dangling"))
 }
 
 func TestFetchFromOrigin_fsck(t *testing.T) {
@@ -107,8 +95,10 @@ func testFetchFromOriginFsck(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
+
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
-	repoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath())
+	repoPath, err := repo.Path()
+	require.NoError(t, err)
 
 	require.NoError(t, pool.Init(ctx))
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo), "seed pool")
@@ -123,7 +113,7 @@ func testFetchFromOriginFsck(t *testing.T, ctx context.Context) {
 		gittest.WithBranch("branch"),
 	)
 
-	err := pool.FetchFromOrigin(ctx, repo)
+	err = pool.FetchFromOrigin(ctx, repo)
 	require.Error(t, err)
 	require.Contains(t, err.Error(), "duplicateEntries: contains duplicate file entries")
 }
@@ -163,26 +153,20 @@ func testFetchFromOriginBitmapHashCache(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
+
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
+	repoPath, err := repo.Path()
+	require.NoError(t, err)
+	gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
 
 	require.NoError(t, pool.Init(ctx))
-	require.NoError(t, pool.FetchFromOrigin(ctx, repo), "seed pool")
+	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 
-	packDir := filepath.Join(pool.FullPath(), "objects/pack")
-	packEntries, err := os.ReadDir(packDir)
+	bitmaps, err := filepath.Glob(filepath.Join(pool.FullPath(), "objects", "pack", "*.bitmap"))
 	require.NoError(t, err)
+	require.Len(t, bitmaps, 1)
 
-	var bitmap string
-	for _, ent := range packEntries {
-		if name := ent.Name(); strings.HasSuffix(name, ".bitmap") {
-			bitmap = filepath.Join(packDir, name)
-			break
-		}
-	}
-
-	require.NotEmpty(t, bitmap, "path to bitmap file")
-
-	gittest.TestBitmapHasHashcache(t, bitmap)
+	gittest.TestBitmapHasHashcache(t, bitmaps[0])
 }
 
 func TestFetchFromOrigin_refUpdates(t *testing.T) {
@@ -194,53 +178,49 @@ func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
+
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
-	repoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath())
+	repoPath, err := repo.Path()
+	require.NoError(t, err)
 
 	poolPath := pool.FullPath()
 
-	require.NoError(t, pool.Init(ctx))
-	require.NoError(t, pool.FetchFromOrigin(ctx, repo), "seed pool")
-
-	oldRefs := map[string]string{
-		"heads/csv":   "3dd08961455abf80ef9115f4afdc1c6f968b503c",
-		"tags/v1.1.0": "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b",
-	}
+	// Seed the pool member with some preliminary data.
+	oldRefs := map[string]git.ObjectID{}
+	oldRefs["heads/csv"] = gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("csv"), gittest.WithMessage("old"))
+	oldRefs["tags/v1.1.0"] = gittest.WriteTag(t, cfg, repoPath, "v1.1.0", oldRefs["heads/csv"].Revision())
 
+	// We now fetch that data into the object pool and verify that it exists as expected.
+	require.NoError(t, pool.Init(ctx))
+	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 	for ref, oid := range oldRefs {
-		require.Equal(t, oid, resolveRef(t, cfg, repoPath, "refs/"+ref), "look up %q in source", ref)
-		require.Equal(t, oid, resolveRef(t, cfg, poolPath, "refs/remotes/origin/"+ref), "look up %q in pool", ref)
+		require.Equal(t, oid, gittest.ResolveRevision(t, cfg, poolPath, "refs/remotes/origin/"+ref))
 	}
 
-	newRefs := map[string]string{
-		"heads/csv":   "46abbb087fcc0fd02c340f0f2f052bd2c7708da3",
-		"tags/v1.1.0": "646ece5cfed840eca0a4feb21bcd6a81bb19bda3",
-	}
+	// Next, we force-overwrite both old references with new objects.
+	newRefs := map[string]git.ObjectID{}
+	newRefs["heads/csv"] = gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("csv"), gittest.WithMessage("new"))
+	newRefs["tags/v1.1.0"] = gittest.WriteTag(t, cfg, repoPath, "v1.1.0", newRefs["heads/csv"].Revision(), gittest.WriteTagConfig{
+		Force: true,
+	})
 
 	// Create a bunch of additional references. This is to trigger OptimizeRepository to indeed
 	// repack the loose references as we expect it to in this test. It's debatable whether we
 	// should test this at all here given that this is business of the housekeeping package. But
 	// it's easy enough to do, so it doesn't hurt.
 	for i := 0; i < 32; i++ {
-		newRefs[fmt.Sprintf("heads/branch-%d", i)] = gittest.WriteCommit(t, cfg, repoPath,
+		branchName := fmt.Sprintf("branch-%d", i)
+		newRefs["heads/"+branchName] = gittest.WriteCommit(t, cfg, repoPath,
 			gittest.WithMessage(strconv.Itoa(i)),
-		).String()
-	}
-
-	for ref, oid := range newRefs {
-		require.NotEqual(t, oid, oldRefs[ref], "sanity check of new refs")
-		gittest.Exec(t, cfg, "-C", repoPath, "update-ref", "refs/"+ref, oid)
-		require.Equal(t, oid, resolveRef(t, cfg, repoPath, "refs/"+ref), "look up %q in source after update", ref)
+			gittest.WithBranch(branchName),
+		)
 	}
 
-	require.NoError(t, pool.FetchFromOrigin(ctx, repo), "update pool")
-
+	// Now we fetch again and verify that all references should have been updated accordingly.
+	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 	for ref, oid := range newRefs {
-		require.Equal(t, oid, resolveRef(t, cfg, poolPath, "refs/remotes/origin/"+ref), "look up %q in pool after update", ref)
+		require.Equal(t, oid, gittest.ResolveRevision(t, cfg, poolPath, "refs/remotes/origin/"+ref))
 	}
-
-	looseRefs := testhelper.MustRunCommand(t, nil, "find", filepath.Join(poolPath, "refs"), "-type", "f")
-	require.Equal(t, "", string(looseRefs), "there should be no loose refs after the fetch")
 }
 
 func TestFetchFromOrigin_refs(t *testing.T) {
@@ -251,30 +231,29 @@ func TestFetchFromOrigin_refs(t *testing.T) {
 func testFetchFromOriginRefs(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	cfg, pool, _ := setupObjectPool(t, ctx)
+	cfg, pool, repoProto := setupObjectPool(t, ctx)
+
+	// Initialize the object pool and verify that it ain't yet got any references.
 	poolPath := pool.FullPath()
+	require.NoError(t, pool.Init(ctx))
+	require.Empty(t, gittest.Exec(t, cfg, "-C", poolPath, "for-each-ref", "--format=%(refname)"))
 
-	// Init the source repo with a bunch of refs.
-	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
-		SkipCreationViaService: true,
-	})
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
+	repoPath, err := repo.Path()
+	require.NoError(t, err)
 
-	commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries())
-	for _, ref := range []string{"refs/heads/master", "refs/environments/1", "refs/tags/lightweight-tag"} {
-		gittest.Exec(t, cfg, "-C", repoPath, "update-ref", ref, commitID.String())
+	// Initialize the repository with a bunch of references.
+	commitID := gittest.WriteCommit(t, cfg, repoPath)
+	for _, ref := range []git.ReferenceName{"refs/heads/master", "refs/environments/1", "refs/tags/lightweight-tag"} {
+		gittest.WriteRef(t, cfg, repoPath, ref, commitID)
 	}
 	gittest.WriteTag(t, cfg, repoPath, "annotated-tag", commitID.Revision(), gittest.WriteTagConfig{
 		Message: "tag message",
 	})
 
-	require.NoError(t, pool.Init(ctx))
-
-	// The pool shouldn't have any refs yet.
-	require.Empty(t, gittest.Exec(t, cfg, "-C", poolPath, "for-each-ref", "--format=%(refname)"))
-
+	// Fetch from the pool member. This should pull in all references we have just created in
+	// that repository into the pool.
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
-
 	require.Equal(t,
 		[]string{
 			"refs/remotes/origin/environments/1",
@@ -285,6 +264,8 @@ func testFetchFromOriginRefs(t *testing.T, ctx context.Context) {
 		strings.Split(text.ChompBytes(gittest.Exec(t, cfg, "-C", poolPath, "for-each-ref", "--format=%(refname)")), "\n"),
 	)
 
+	// We don't want to see "FETCH_HEAD" though: it's useless and may take quite some time to
+	// write out in Git.
 	require.NoFileExists(t, filepath.Join(poolPath, "FETCH_HEAD"))
 }
 
@@ -308,8 +289,3 @@ func testFetchFromOriginMissingPool(t *testing.T, ctx context.Context) {
 		require.True(t, pool.Exists())
 	}
 }
-
-func resolveRef(t *testing.T, cfg config.Cfg, repo string, ref string) string {
-	out := gittest.Exec(t, cfg, "-C", repo, "rev-parse", ref)
-	return text.ChompBytes(out)
-}
diff --git a/internal/git/objectpool/link_test.go b/internal/git/objectpool/link_test.go
index 146ff8f06..08880c72a 100644
--- a/internal/git/objectpool/link_test.go
+++ b/internal/git/objectpool/link_test.go
@@ -12,6 +12,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/backchannel"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/transaction/txinfo"
@@ -19,6 +20,8 @@ import (
 )
 
 func TestLink(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
@@ -48,6 +51,7 @@ func TestLink(t *testing.T) {
 
 func TestLink_transactional(t *testing.T) {
 	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
 	cfg, pool, poolMemberProto := setupObjectPool(t, ctx)
@@ -72,58 +76,56 @@ func TestLink_transactional(t *testing.T) {
 	require.Equal(t, 2, len(txManager.Votes()))
 }
 
-func TestLinkRemoveBitmap(t *testing.T) {
+func TestLink_removeBitmap(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
+
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
-	require.NoError(t, pool.Init(ctx))
+	repoPath, err := repo.Path()
+	require.NoError(t, err)
 
-	testRepoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath())
+	gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
 
+	// Initialize the pool and pull in all references from the repository.
+	require.NoError(t, pool.Init(ctx))
 	poolPath := pool.FullPath()
-	gittest.Exec(t, cfg, "-C", poolPath, "fetch", testRepoPath, "+refs/*:refs/*")
+	gittest.Exec(t, cfg, "-C", poolPath, "fetch", repoPath, "+refs/*:refs/*")
 
+	// Repack both the object pool and the pool member such that they both have bitmaps.
 	gittest.Exec(t, cfg, "-C", poolPath, "repack", "-adb")
-	require.Len(t, listBitmaps(t, pool.FullPath()), 1, "pool bitmaps before")
-
-	gittest.Exec(t, cfg, "-C", testRepoPath, "repack", "-adb")
-	require.Len(t, listBitmaps(t, testRepoPath), 1, "member bitmaps before")
-
-	refsBefore := gittest.Exec(t, cfg, "-C", testRepoPath, "for-each-ref")
+	requireHasBitmap(t, poolPath, true)
+	gittest.Exec(t, cfg, "-C", repoPath, "repack", "-adb")
+	requireHasBitmap(t, repoPath, true)
 
+	// After linking the repository to its pool it should not have a bitmap anymore as Git does
+	// not allow for multiple bitmaps to exist.
 	require.NoError(t, pool.Link(ctx, repo))
+	requireHasBitmap(t, poolPath, true)
+	requireHasBitmap(t, repoPath, false)
 
-	require.Len(t, listBitmaps(t, pool.FullPath()), 1, "pool bitmaps after")
-	require.Len(t, listBitmaps(t, testRepoPath), 0, "member bitmaps after")
-
-	gittest.Exec(t, cfg, "-C", testRepoPath, "fsck")
-
-	refsAfter := gittest.Exec(t, cfg, "-C", testRepoPath, "for-each-ref")
-	require.Equal(t, refsBefore, refsAfter, "compare member refs before/after link")
+	// Sanity-check that the repository is still consistent.
+	gittest.Exec(t, cfg, "-C", repoPath, "fsck")
 }
 
-func listBitmaps(t *testing.T, repoPath string) []string {
-	entries, err := os.ReadDir(filepath.Join(repoPath, "objects/pack"))
+func requireHasBitmap(t *testing.T, repoPath string, expected bool) {
+	hasBitmap, err := stats.HasBitmap(repoPath)
 	require.NoError(t, err)
-
-	var bitmaps []string
-	for _, entry := range entries {
-		if strings.HasSuffix(entry.Name(), ".bitmap") {
-			bitmaps = append(bitmaps, entry.Name())
-		}
-	}
-
-	return bitmaps
+	require.Equal(t, expected, hasBitmap)
 }
 
-func TestLinkAbsoluteLinkExists(t *testing.T) {
+func TestLink_absoluteLinkExists(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
-	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
-	testRepoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath())
+	repo := localrepo.NewTestRepo(t, cfg, repoProto)
+	repoPath, err := repo.Path()
+	require.NoError(t, err)
 
 	require.NoError(t, pool.Remove(ctx), "make sure pool does not exist prior to creation")
 	require.NoError(t, pool.Create(ctx, repo), "create pool")
@@ -142,8 +144,8 @@ func TestLinkAbsoluteLinkExists(t *testing.T) {
 	content := testhelper.MustReadFile(t, altPath)
 	require.False(t, filepath.IsAbs(string(content)), "expected %q to be relative path", content)
 
-	testRepoObjectsPath := filepath.Join(testRepoPath, "objects")
-	require.Equal(t, fullPath, filepath.Join(testRepoObjectsPath, string(content)), "the content of the alternates file should be the relative version of the absolute pat")
+	repoObjectsPath := filepath.Join(repoPath, "objects")
+	require.Equal(t, fullPath, filepath.Join(repoObjectsPath, string(content)), "the content of the alternates file should be the relative version of the absolute pat")
 
 	require.True(t, gittest.RemoteExists(t, cfg, pool.FullPath(), "origin"), "pool remotes should include %v", repo)
 }
diff --git a/internal/git/objectpool/pool_test.go b/internal/git/objectpool/pool_test.go
index f257feb6a..7b8e0a46d 100644
--- a/internal/git/objectpool/pool_test.go
+++ b/internal/git/objectpool/pool_test.go
@@ -5,31 +5,39 @@ package objectpool
 import (
 	"os"
 	"path/filepath"
-	"strings"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 )
 
 func TestNewObjectPool(t *testing.T) {
+	t.Parallel()
+
 	cfg := testcfg.Build(t)
 
 	locator := config.NewLocator(cfg)
 
-	_, err := NewObjectPool(locator, nil, nil, nil, nil, cfg.Storages[0].Name, gittest.NewObjectPoolName(t))
-	require.NoError(t, err)
+	t.Run("successful", func(t *testing.T) {
+		_, err := NewObjectPool(locator, nil, nil, nil, nil, cfg.Storages[0].Name, gittest.NewObjectPoolName(t))
+		require.NoError(t, err)
+	})
 
-	_, err = NewObjectPool(locator, nil, nil, nil, nil, "mepmep", gittest.NewObjectPoolName(t))
-	require.Error(t, err, "creating pool in storage that does not exist should fail")
+	t.Run("unknown storage", func(t *testing.T) {
+		_, err := NewObjectPool(locator, nil, nil, nil, nil, "mepmep", gittest.NewObjectPoolName(t))
+		require.Equal(t, helper.ErrInvalidArgumentf("GetStorageByName: no such storage: %q", "mepmep"), err)
+	})
 }
 
-func TestNewFromRepoSuccess(t *testing.T) {
+func TestFromRepo_successful(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
@@ -45,47 +53,52 @@ func TestNewFromRepoSuccess(t *testing.T) {
 	require.Equal(t, pool.storageName, poolFromRepo.storageName)
 }
 
-func TestNewFromRepoNoObjectPool(t *testing.T) {
+func TestFromRepo_failures(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
-	cfg, pool, repoProto := setupObjectPool(t, ctx)
-	repo := localrepo.NewTestRepo(t, cfg, repoProto)
-	locator := config.NewLocator(cfg)
-	testRepoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath())
+	t.Run("without alternates file", func(t *testing.T) {
+		cfg, pool, repoProto := setupObjectPool(t, ctx)
+		locator := config.NewLocator(cfg)
+		repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
-	// no alternates file
-	poolFromRepo, err := FromRepo(locator, pool.gitCmdFactory, nil, nil, nil, repo)
-	require.Equal(t, ErrAlternateObjectDirNotExist, err)
-	require.Nil(t, poolFromRepo)
+		poolFromRepo, err := FromRepo(locator, pool.gitCmdFactory, nil, nil, nil, repo)
+		require.Equal(t, ErrAlternateObjectDirNotExist, err)
+		require.Nil(t, poolFromRepo)
+	})
 
-	// with an alternates file
-	testCases := []struct {
+	for _, tc := range []struct {
 		desc        string
 		fileContent []byte
 		expectedErr error
 	}{
 		{
-			desc:        "points to non existent path",
+			desc:        "alternates points to non existent path",
 			fileContent: []byte("/tmp/invalid_path"),
 			expectedErr: ErrInvalidPoolRepository,
 		},
 		{
-			desc:        "empty file",
+			desc:        "alternates is empty",
 			fileContent: nil,
 			expectedErr: nil,
 		},
 		{
-			desc:        "first line commented out",
+			desc:        "alternates is commented",
 			fileContent: []byte("#/tmp/invalid/path"),
 			expectedErr: ErrAlternateObjectDirNotExist,
 		},
-	}
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			cfg, pool, repoProto := setupObjectPool(t, ctx)
+			locator := config.NewLocator(cfg)
 
-	require.NoError(t, os.MkdirAll(filepath.Join(testRepoPath, "objects", "info"), 0o755))
+			repo := localrepo.NewTestRepo(t, cfg, repoProto)
+			repoPath, err := repo.Path()
+			require.NoError(t, err)
 
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			alternateFilePath := filepath.Join(testRepoPath, "objects", "info", "alternates")
+			require.NoError(t, os.MkdirAll(filepath.Join(repoPath, "objects", "info"), 0o755))
+			alternateFilePath := filepath.Join(repoPath, "objects", "info", "alternates")
 			require.NoError(t, os.WriteFile(alternateFilePath, tc.fileContent, 0o644))
 			poolFromRepo, err := FromRepo(locator, pool.gitCmdFactory, nil, nil, nil, repo)
 			require.Equal(t, tc.expectedErr, err)
@@ -97,40 +110,34 @@ func TestNewFromRepoNoObjectPool(t *testing.T) {
 }
 
 func TestCreate(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
-	repo := localrepo.NewTestRepo(t, cfg, repoProto)
-
-	testRepoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath())
-
-	masterSha := gittest.Exec(t, cfg, "-C", testRepoPath, "show-ref", "master")
 
-	err := pool.Create(ctx, repo)
+	repo := localrepo.NewTestRepo(t, cfg, repoProto)
+	repoPath, err := repo.Path()
 	require.NoError(t, err)
-	defer func() {
-		require.NoError(t, pool.Remove(ctx))
-	}()
-
-	require.True(t, pool.IsValid())
-
-	// No hooks
-	assert.NoDirExists(t, filepath.Join(pool.FullPath(), "hooks"))
 
-	// origin is set
-	out := gittest.Exec(t, cfg, "-C", pool.FullPath(), "remote", "get-url", "origin")
-	assert.Equal(t, testRepoPath, strings.TrimRight(string(out), "\n"))
+	commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
 
-	// refs exist
-	out = gittest.Exec(t, cfg, "-C", pool.FullPath(), "show-ref", "refs/heads/master")
-	assert.Equal(t, masterSha, out)
+	require.NoError(t, pool.Create(ctx, repo))
+	require.True(t, pool.IsValid())
 
-	// No problems
-	out = gittest.Exec(t, cfg, "-C", pool.FullPath(), "cat-file", "-s", "55bc176024cfa3baaceb71db584c7e5df900ea65")
-	assert.Equal(t, "282\n", string(out))
+	// There should not be a "hooks" directory in the pool.
+	require.NoDirExists(t, filepath.Join(pool.FullPath(), "hooks"))
+	// The "origin" remote of the pool points to the pool member.
+	require.Equal(t, repoPath, text.ChompBytes(gittest.Exec(t, cfg, "-C", pool.FullPath(), "remote", "get-url", "origin")))
+	// The "master" branch points to the same commit as in the pool member.
+	require.Equal(t, commitID, gittest.ResolveRevision(t, cfg, pool.FullPath(), "refs/heads/master"))
+	// Objects exist in the pool repository.
+	gittest.RequireObjectExists(t, cfg, pool.FullPath(), commitID)
 }
 
-func TestCreateSubDirsExist(t *testing.T) {
+func TestCreate_subdirsExist(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
@@ -147,6 +154,8 @@ func TestCreateSubDirsExist(t *testing.T) {
 }
 
 func TestRemove(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
diff --git a/internal/git/objectpool/testhelper_test.go b/internal/git/objectpool/testhelper_test.go
index 1fb791db9..336c6af97 100644
--- a/internal/git/objectpool/testhelper_test.go
+++ b/internal/git/objectpool/testhelper_test.go
@@ -26,9 +26,10 @@ func TestMain(m *testing.M) {
 func setupObjectPool(t *testing.T, ctx context.Context) (config.Cfg, *ObjectPool, *gitalypb.Repository) {
 	t.Helper()
 
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
-
-	gittest.FixGitLabTestRepoForCommitGraphs(t, cfg, repoPath)
+	cfg := testcfg.Build(t)
+	repo, _ := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
+		SkipCreationViaService: true,
+	})
 
 	gitCommandFactory := gittest.NewCommandFactory(t, cfg, git.WithSkipHooks())