objectpool: Fix test for usage of delta islands

We have a test in our objectpool package that ought to verify whether we
set up delta islands correctly when fetching into an object pool. This
test is completely broken though: we don't check delta islands in the
pool, but instead in its primary pool member by accident.

Fix this bug and check delta islands in the pool itself. Furthermore,
stop repacking the pool objects: it destroys the purpose of the test as
we want to test exactly how objects are packed after fetching.

While at it, write a commit into the source repository before performing
the fetch so that there is actually data that has changed. This isn't
necessary right now, but is required as soon as wwe migrate maintenance
of pool repositories to use `OptimizeRepository()`.

4 files changed, 27 insertions, 17 deletions

diff --git a/internal/git/gittest/delta_islands.go b/internal/git/gittest/delta_islands.go
index 5275bff44..b54b72c73 100644
--- a/internal/git/gittest/delta_islands.go
+++ b/internal/git/gittest/delta_islands.go
@@ -12,7 +12,7 @@ import (
 
 // TestDeltaIslands checks whether functions that repack objects in a repository correctly set up
 // delta islands. Based on https://github.com/git/git/blob/master/t/t5320-delta-islands.sh.
-func TestDeltaIslands(t *testing.T, cfg config.Cfg, repoPath string, repack func() error) {
+func TestDeltaIslands(t *testing.T, cfg config.Cfg, repoPath string, isPoolRepo bool, repack func() error) {
 	t.Helper()
 
 	// Create blobs that we expect Git to use delta compression on.
@@ -24,13 +24,20 @@ func TestDeltaIslands(t *testing.T, cfg config.Cfg, repoPath string, repack func
 	// be used as delta base.
 	badBlob := blob2 + "\nbad blob"
 
+	refsPrefix := "refs"
+	if isPoolRepo {
+		// Pool repositories use different references for their delta islands, so we need to
+		// adapt accordingly.
+		refsPrefix = git.ObjectPoolRefNamespace
+	}
+
 	// Make the first two blobs reachable via references that are part of the delta island.
-	blob1ID := commitBlob(t, cfg, repoPath, "refs/heads/branch1", blob1)
-	blob2ID := commitBlob(t, cfg, repoPath, "refs/tags/tag2", blob2)
+	blob1ID := commitBlob(t, cfg, repoPath, refsPrefix+"/heads/branch1", blob1)
+	blob2ID := commitBlob(t, cfg, repoPath, refsPrefix+"/tags/tag2", blob2)
 
 	// The bad blob will only be reachable via a reference that is not covered by a delta
 	// island. Because of that it should be excluded from delta chains in the main island.
-	badBlobID := commitBlob(t, cfg, repoPath, "refs/bad/ref3", badBlob)
+	badBlobID := commitBlob(t, cfg, repoPath, refsPrefix+"/bad/ref3", badBlob)
 
 	// Repack all objects into a single pack so that we can verify that delta chains are built
 	// by Git as expected. Most notably, we don't use the delta islands here yet and thus the
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index b962ff547..92e0f36c4 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -120,22 +120,25 @@ func TestFetchFromOrigin_deltaIslands(t *testing.T) {
 	ctx := testhelper.Context(t)
 
 	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.FetchFromOrigin(ctx, repo), "seed pool")
 	require.NoError(t, pool.Link(ctx, repo))
 
-	gittest.TestDeltaIslands(t, cfg, repoPath, func() error {
-		// This should create a new packfile with good delta chains in the pool
-		if err := pool.FetchFromOrigin(ctx, repo); err != nil {
-			return err
-		}
-
-		// Make sure the old packfile, with bad delta chains, is deleted from the source repo
-		gittest.Exec(t, cfg, "-C", repoPath, "repack", "-ald")
-
-		return nil
+	gittest.TestDeltaIslands(t, cfg, pool.FullPath(), true, func() error {
+		// The first fetch has already fetched all objects into the pool repository, so
+		// there is nothing new to fetch anymore. Consequentially, FetchFromOrigin doesn't
+		// alter the object database and thus OptimizeRepository would notice that nothing
+		// needs to be optimized.
+		//
+		// We thus write a new commit into the pool member's repository so that we end up
+		// with two packfiles after the fetch.
+		gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("changed-ref"))
+
+		return pool.FetchFromOrigin(ctx, repo)
 	})
 }
 
diff --git a/internal/gitaly/service/repository/gc_test.go b/internal/gitaly/service/repository/gc_test.go
index 4c28c5058..3bd186a8c 100644
--- a/internal/gitaly/service/repository/gc_test.go
+++ b/internal/gitaly/service/repository/gc_test.go
@@ -497,7 +497,7 @@ func TestGarbageCollectDeltaIslands(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, client := setupRepositoryService(ctx, t)
 
-	gittest.TestDeltaIslands(t, cfg, repoPath, func() error {
+	gittest.TestDeltaIslands(t, cfg, repoPath, false, func() error {
 		//nolint:staticcheck
 		_, err := client.GarbageCollect(ctx, &gitalypb.GarbageCollectRequest{Repository: repo})
 		return err
diff --git a/internal/gitaly/service/repository/repack_test.go b/internal/gitaly/service/repository/repack_test.go
index 6a90cbdb5..705134269 100644
--- a/internal/gitaly/service/repository/repack_test.go
+++ b/internal/gitaly/service/repository/repack_test.go
@@ -298,7 +298,7 @@ func TestRepackFullDeltaIslands(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, client := setupRepositoryService(ctx, t)
 
-	gittest.TestDeltaIslands(t, cfg, repoPath, func() error {
+	gittest.TestDeltaIslands(t, cfg, repoPath, false, func() error {
 		//nolint:staticcheck
 		_, err := client.RepackFull(ctx, &gitalypb.RepackFullRequest{Repository: repo})
 		return err