gittest: Iterate on delta islands test

In 04b39de9f (objectpool: Fix test for usage of delta islands,
2022-07-13) we have refactored the tests for delta islands so that they
work as expected in the context of object pools. The result still isn't
quite satisfactory though as it puts too many assumptions about how the
repacking function is supposed to work in the test itself. Thus, the
test is about to break as soon as we start pruning pool references.

Refactor the test again. This time, we acknowledge that the repo we're
doing the setup in can be different from the one we're ultimately
repacking to account for the behaviour of FetchIntoObjectPool.

5 files changed, 28 insertions, 23 deletions

diff --git a/internal/git/gittest/delta_islands.go b/internal/git/gittest/delta_islands.go
index c582d0813..485afe95e 100644
--- a/internal/git/gittest/delta_islands.go
+++ b/internal/git/gittest/delta_islands.go
@@ -11,8 +11,19 @@ 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, isPoolRepo bool, repack func() error) {
+// delta islands. Based on https://github.com/git/git/blob/master/t/t5320-delta-islands.sh. Note
+// that this function accepts two different repository paths: one repo to modify that shall grow the
+// new references and objects, and one repository that we ultimately end up repacking. In the
+// general case these should refer to the same repository, but for object pools these may be the
+// pool member and the pool, respectively.
+func TestDeltaIslands(
+	t *testing.T,
+	cfg config.Cfg,
+	repoPathToModify string,
+	repoPathToRepack string,
+	isPoolRepo bool,
+	repack func() error,
+) {
 	t.Helper()
 
 	// Create blobs that we expect Git to use delta compression on.
@@ -32,25 +43,25 @@ func TestDeltaIslands(t *testing.T, cfg config.Cfg, repoPath string, isPoolRepo
 	}
 
 	// Make the first two blobs reachable via references that are part of the delta island.
-	blob1ID := commitBlob(t, cfg, repoPath, refsPrefix+"/heads/branch1", blob1)
-	blob2ID := commitBlob(t, cfg, repoPath, refsPrefix+"/tags/tag2", blob2)
+	blob1ID := commitBlob(t, cfg, repoPathToModify, refsPrefix+"/heads/branch1", blob1)
+	blob2ID := commitBlob(t, cfg, repoPathToModify, 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, refsPrefix+"/bad/ref3", badBlob)
+	badBlobID := commitBlob(t, cfg, repoPathToModify, 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
 	// delta base for both blob1 and blob2 should be the bad blob.
-	Exec(t, cfg, "-C", repoPath, "repack", "-ad")
-	require.Equal(t, badBlobID, deltaBase(t, cfg, repoPath, blob1ID), "expect blob 1 delta base to be bad blob after test setup")
-	require.Equal(t, badBlobID, deltaBase(t, cfg, repoPath, blob2ID), "expect blob 2 delta base to be bad blob after test setup")
+	Exec(t, cfg, "-C", repoPathToModify, "repack", "-ad")
+	require.Equal(t, badBlobID, deltaBase(t, cfg, repoPathToModify, blob1ID), "expect blob 1 delta base to be bad blob after test setup")
+	require.Equal(t, badBlobID, deltaBase(t, cfg, repoPathToModify, blob2ID), "expect blob 2 delta base to be bad blob after test setup")
 
 	// Now we run the repacking function provided to us by the caller. We expect it to use delta
 	// chains, and thus neither of the two blobs should use the bad blob as delta base.
 	require.NoError(t, repack(), "repack after delta island setup")
-	require.Equal(t, blob2ID, deltaBase(t, cfg, repoPath, blob1ID), "blob 1 delta base should be blob 2 after repack")
-	require.Equal(t, DefaultObjectHash.ZeroOID.String(), deltaBase(t, cfg, repoPath, blob2ID), "blob 2 should not be delta compressed after repack")
+	require.Equal(t, blob2ID, deltaBase(t, cfg, repoPathToRepack, blob1ID), "blob 1 delta base should be blob 2 after repack")
+	require.Equal(t, DefaultObjectHash.ZeroOID.String(), deltaBase(t, cfg, repoPathToRepack, blob2ID), "blob 2 should not be delta compressed after repack")
 }
 
 func commitBlob(t *testing.T, cfg config.Cfg, repoPath, ref string, content string) string {
diff --git a/internal/git/housekeeping/objects_test.go b/internal/git/housekeeping/objects_test.go
index 6156c65bf..156a58ff2 100644
--- a/internal/git/housekeeping/objects_test.go
+++ b/internal/git/housekeeping/objects_test.go
@@ -60,7 +60,7 @@ func TestRepackObjects(t *testing.T) {
 		})
 		repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
-		gittest.TestDeltaIslands(t, cfg, repoPath, IsPoolRepository(repoProto), func() error {
+		gittest.TestDeltaIslands(t, cfg, repoPath, repoPath, IsPoolRepository(repoProto), func() error {
 			return RepackObjects(ctx, repo, RepackObjectsConfig{
 				FullRepack: true,
 			})
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index 514927555..53fc9a3e6 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -128,16 +128,10 @@ func TestFetchFromOrigin_deltaIslands(t *testing.T) {
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo), "seed pool")
 	require.NoError(t, pool.Link(ctx, repo))
 
-	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"))
-
+	// The setup of delta islands is done in the normal repository, and thus we pass `false`
+	// for `isPoolRepo`. Verification whether we correctly handle repacking though happens in
+	// the pool repository.
+	gittest.TestDeltaIslands(t, cfg, repoPath, pool.FullPath(), false, func() error {
 		return pool.FetchFromOrigin(ctx, repo)
 	})
 }
diff --git a/internal/gitaly/service/repository/gc_test.go b/internal/gitaly/service/repository/gc_test.go
index feaeab138..3dcdb3a02 100644
--- a/internal/gitaly/service/repository/gc_test.go
+++ b/internal/gitaly/service/repository/gc_test.go
@@ -536,7 +536,7 @@ func TestGarbageCollectDeltaIslands(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, client := setupRepositoryService(ctx, t)
 
-	gittest.TestDeltaIslands(t, cfg, repoPath, false, func() error {
+	gittest.TestDeltaIslands(t, cfg, repoPath, 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 e89f25cb9..eb3d8b912 100644
--- a/internal/gitaly/service/repository/repack_test.go
+++ b/internal/gitaly/service/repository/repack_test.go
@@ -300,7 +300,7 @@ func TestRepackFullDeltaIslands(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, client := setupRepositoryService(ctx, t)
 
-	gittest.TestDeltaIslands(t, cfg, repoPath, false, func() error {
+	gittest.TestDeltaIslands(t, cfg, repoPath, repoPath, false, func() error {
 		//nolint:staticcheck
 		_, err := client.RepackFull(ctx, &gitalypb.RepackFullRequest{Repository: repo})
 		return err