Maintain pool packfiles

7 files changed, 185 insertions, 76 deletions

diff --git a/changelogs/unreleased/jv-dedup-delta-islands.yml b/changelogs/unreleased/jv-dedup-delta-islands.yml
new file mode 100644
index 000000000..2b03f1dc6
--- /dev/null
+++ b/changelogs/unreleased/jv-dedup-delta-islands.yml
@@ -0,0 +1,5 @@
+---
+title: Maintain pool packfiles
+merge_request: 1316
+author:
+type: performance
diff --git a/internal/service/repository/delta_islands_test.go b/internal/git/gittest/delta_islands.go
index 5efd49451..7640be6d3 100644
--- a/internal/service/repository/delta_islands_test.go
+++ b/internal/git/gittest/delta_islands.go
@@ -1,4 +1,4 @@
-package repository
+package gittest
 
 import (
 	"bytes"
@@ -14,16 +14,9 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 )
 
-type deltaIslandOutcome int
-
-const (
-	expectDeltaIslands deltaIslandOutcome = iota
-	expectNoDeltaIslands
-)
-
-// testDeltaIslands is based on the tests in
+// TestDeltaIslands is based on the tests in
 // https://github.com/git/git/blob/master/t/t5320-delta-islands.sh .
-func testDeltaIslands(t *testing.T, repoPath string, outcome deltaIslandOutcome, repack func() error) {
+func TestDeltaIslands(t *testing.T, repoPath string, repack func() error) {
 	gitVersion, err := git.Version()
 	require.NoError(t, err)
 
@@ -60,16 +53,10 @@ func testDeltaIslands(t *testing.T, repoPath string, outcome deltaIslandOutcome,
 
 	require.NoError(t, repack(), "repack after delta island setup")
 
-	switch outcome {
-	case expectDeltaIslands:
-		assert.Equal(t, blob2ID, deltaBase(t, repoPath, blob1ID), "blob 1 delta base should be blob 2 after repack")
+	assert.Equal(t, blob2ID, deltaBase(t, repoPath, blob1ID), "blob 1 delta base should be blob 2 after repack")
 
-		// blob2 is the bigger of the two so it should be the delta base
-		assert.Equal(t, git.NullSHA, deltaBase(t, repoPath, blob2ID), "blob 2 should not be delta compressed after repack")
-	case expectNoDeltaIslands:
-		assert.Equal(t, badBlobID, deltaBase(t, repoPath, blob1ID), "expect blob 1 delta base to still be bad blob after repack")
-		assert.Equal(t, badBlobID, deltaBase(t, repoPath, blob2ID), "expect blob 2 delta base to still be bad blob after repack")
-	}
+	// blob2 is the bigger of the two so it should be the delta base
+	assert.Equal(t, git.NullSHA, deltaBase(t, repoPath, blob2ID), "blob 2 should not be delta compressed after repack")
 }
 
 func commitBlob(t *testing.T, repoPath, ref string, content []byte) string {
diff --git a/internal/git/gittest/hashcache.go b/internal/git/gittest/hashcache.go
new file mode 100644
index 000000000..b09c58dfc
--- /dev/null
+++ b/internal/git/gittest/hashcache.go
@@ -0,0 +1,27 @@
+package gittest
+
+import (
+	"encoding/binary"
+	"io"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestBitmapHasHashcache checks if the named pack bitmap file contains
+// "hash cache" data. See
+// https://github.com/git/git/blob/master/Documentation/technical/bitmap-format.txt
+func TestBitmapHasHashcache(t *testing.T, bitmap string) {
+	bitmapFile, err := os.Open(bitmap)
+	require.NoError(t, err)
+	defer bitmapFile.Close()
+
+	b := make([]byte, 8)
+	_, err = io.ReadFull(bitmapFile, b)
+	require.NoError(t, err)
+
+	const hashCacheFlag = 0x4
+	flags := binary.BigEndian.Uint16(b[6:])
+	require.Equal(t, uint16(hashCacheFlag), flags&hashCacheFlag, "expect BITMAP_OPT_HASH_CACHE to be set")
+}
diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 30df30ba1..b173e02b8 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -14,6 +14,11 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
+const (
+	sourceRemote       = "origin"
+	sourceRefNamespace = "refs/remotes/" + sourceRemote
+)
+
 // FetchFromOrigin initializes the pool and fetches the objects from its origin repository
 func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repository) error {
 	if err := o.Init(ctx); err != nil {
@@ -35,7 +40,7 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos
 
 	var originExists bool
 	for remoteReader.Scan() {
-		if remoteReader.Text() == "origin" {
+		if remoteReader.Text() == sourceRemote {
 			originExists = true
 		}
 	}
@@ -45,12 +50,12 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos
 
 	var setOriginCmd *command.Command
 	if originExists {
-		setOriginCmd, err = git.Command(ctx, o, "remote", "set-url", "origin", originPath)
+		setOriginCmd, err = git.Command(ctx, o, "remote", "set-url", sourceRemote, originPath)
 		if err != nil {
 			return err
 		}
 	} else {
-		setOriginCmd, err = git.Command(ctx, o, "remote", "add", "origin", originPath)
+		setOriginCmd, err = git.Command(ctx, o, "remote", "add", sourceRemote, originPath)
 		if err != nil {
 			return err
 		}
@@ -60,7 +65,8 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos
 		return err
 	}
 
-	fetchCmd, err := git.Command(ctx, o, "fetch", "--quiet", "origin")
+	refSpec := fmt.Sprintf("+refs/*:%s/*", sourceRefNamespace)
+	fetchCmd, err := git.Command(ctx, o, "fetch", "--quiet", sourceRemote, refSpec)
 	if err != nil {
 		return err
 	}
@@ -73,7 +79,7 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos
 		return err
 	}
 
-	return nil
+	return repackPool(ctx, o)
 }
 
 const danglingObjectNamespace = "refs/dangling"
@@ -122,7 +128,22 @@ func rescueDanglingObjects(ctx context.Context, repo repository.GitRepo) error {
 		return fmt.Errorf("git fsck: %v", err)
 	}
 
-	if err := updater.Wait(); err != nil {
+	return updater.Wait()
+}
+
+func repackPool(ctx context.Context, pool repository.GitRepo) error {
+	repackArgs := []string{
+		"-c", "pack.island=" + sourceRefNamespace + "/heads",
+		"-c", "pack.island=" + sourceRefNamespace + "/tags",
+		"-c", "pack.writeBitmapHashCache=true",
+		"repack", "-aidb",
+	}
+	repackCmd, err := git.Command(ctx, pool, repackArgs...)
+	if err != nil {
+		return err
+	}
+
+	if err := repackCmd.Wait(); err != nil {
 		return err
 	}
 
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index 85dfb144a..5af01b220 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -5,10 +5,13 @@ import (
 	"encoding/hex"
 	"fmt"
 	"io"
+	"io/ioutil"
+	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 )
@@ -86,3 +89,107 @@ func TestFetchFromOriginDangling(t *testing.T) {
 		require.Contains(t, refsAfterLines, fmt.Sprintf("refs/dangling/%s %s", id, id))
 	}
 }
+
+func TestFetchFromOriginDeltaIslands(t *testing.T) {
+	source, sourcePath, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	pool, err := NewObjectPool(source.StorageName, testhelper.NewTestObjectPoolName(t))
+	require.NoError(t, err)
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	require.NoError(t, pool.FetchFromOrigin(ctx, source), "seed pool")
+	require.NoError(t, pool.Link(ctx, source))
+
+	gittest.TestDeltaIslands(t, sourcePath, func() error {
+		// This should create a new packfile with good delta chains in the pool
+		if err := pool.FetchFromOrigin(ctx, source); err != nil {
+			return err
+		}
+
+		// Make sure the old packfile, with bad delta chains, is deleted from the source repo
+		testhelper.MustRunCommand(t, nil, "git", "-C", sourcePath, "repack", "-ald")
+
+		return nil
+	})
+}
+
+func TestFetchFromOriginBitmapHashCache(t *testing.T) {
+	source, _, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	pool, err := NewObjectPool(source.StorageName, testhelper.NewTestObjectPoolName(t))
+	require.NoError(t, err)
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	require.NoError(t, pool.FetchFromOrigin(ctx, source), "seed pool")
+
+	packDir := filepath.Join(pool.FullPath(), "objects/pack")
+	packEntries, err := ioutil.ReadDir(packDir)
+	require.NoError(t, err)
+
+	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)
+}
+
+func TestFetchFromOriginRefUpdates(t *testing.T) {
+	source, sourcePath, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	pool, err := NewObjectPool(source.StorageName, testhelper.NewTestObjectPoolName(t))
+	require.NoError(t, err)
+	poolPath := pool.FullPath()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	require.NoError(t, pool.FetchFromOrigin(ctx, source), "seed pool")
+
+	oldRefs := map[string]string{
+		"heads/csv":   "3dd08961455abf80ef9115f4afdc1c6f968b503c",
+		"tags/v1.1.0": "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b",
+	}
+
+	for ref, oid := range oldRefs {
+		require.Equal(t, oid, resolveRef(t, sourcePath, "refs/"+ref), "look up %q in source", ref)
+		require.Equal(t, oid, resolveRef(t, poolPath, "refs/remotes/origin/"+ref), "look up %q in pool", ref)
+	}
+
+	newRefs := map[string]string{
+		"heads/csv":   "46abbb087fcc0fd02c340f0f2f052bd2c7708da3",
+		"tags/v1.1.0": "646ece5cfed840eca0a4feb21bcd6a81bb19bda3",
+	}
+
+	for ref, newOid := range newRefs {
+		require.NotEqual(t, newOid, oldRefs[ref], "sanity check of new refs")
+	}
+
+	for ref, oid := range newRefs {
+		testhelper.MustRunCommand(t, nil, "git", "-C", sourcePath, "update-ref", "refs/"+ref, oid)
+		require.Equal(t, oid, resolveRef(t, sourcePath, "refs/"+ref), "look up %q in source after update", ref)
+	}
+
+	require.NoError(t, pool.FetchFromOrigin(ctx, source), "update pool")
+
+	for ref, oid := range newRefs {
+		require.Equal(t, oid, resolveRef(t, poolPath, "refs/remotes/origin/"+ref), "look up %q in pool after update", ref)
+	}
+}
+
+func resolveRef(t *testing.T, repo string, ref string) string {
+	out := testhelper.MustRunCommand(t, nil, "git", "-C", repo, "rev-parse", ref)
+	return text.ChompBytes(out)
+}
diff --git a/internal/service/repository/gc_test.go b/internal/service/repository/gc_test.go
index 029d5e885..0cf9e5d58 100644
--- a/internal/service/repository/gc_test.go
+++ b/internal/service/repository/gc_test.go
@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
@@ -307,20 +308,8 @@ func TestGarbageCollectDeltaIslands(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	testCases := []struct {
-		desc    string
-		outcome deltaIslandOutcome
-		ctx     context.Context
-	}{
-		{desc: "are created by default", outcome: expectDeltaIslands, ctx: ctx},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			testDeltaIslands(t, testRepoPath, tc.outcome, func() error {
-				_, err := client.GarbageCollect(tc.ctx, &gitalypb.GarbageCollectRequest{Repository: testRepo})
-				return err
-			})
-		})
-	}
+	gittest.TestDeltaIslands(t, testRepoPath, func() error {
+		_, err := client.GarbageCollect(ctx, &gitalypb.GarbageCollectRequest{Repository: testRepo})
+		return err
+	})
 }
diff --git a/internal/service/repository/repack_test.go b/internal/service/repository/repack_test.go
index 84f875591..08d94960b 100644
--- a/internal/service/repository/repack_test.go
+++ b/internal/service/repository/repack_test.go
@@ -2,8 +2,6 @@ package repository
 
 import (
 	"context"
-	"encoding/binary"
-	"os"
 	"os/exec"
 	"path"
 	"path/filepath"
@@ -13,6 +11,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
 )
@@ -154,7 +153,7 @@ func TestRepackFullSuccess(t *testing.T) {
 				if len(bmPath) == 0 {
 					t.Errorf("No bitmaps found")
 				}
-				require.True(t, doBitmapsContainHashCache(t, bmPath), "bitmap file should contain hash cache")
+				doBitmapsContainHashCache(t, bmPath)
 			} else {
 				if len(bmPath) != 0 {
 					t.Errorf("Bitmap found: %v", bmPath)
@@ -164,26 +163,12 @@ func TestRepackFullSuccess(t *testing.T) {
 	}
 }
 
-func doBitmapsContainHashCache(t *testing.T, bitmapPaths []string) bool {
+func doBitmapsContainHashCache(t *testing.T, bitmapPaths []string) {
 	// for each bitmap file, check the 2-byte flag as documented in
 	// https://github.com/git/git/blob/master/Documentation/technical/bitmap-format.txt
 	for _, bitmapPath := range bitmapPaths {
-		b := make([]byte, 8)
-		bitmapFile, err := os.Open(bitmapPath)
-		require.NoError(t, err)
-		defer bitmapFile.Close()
-
-		read, err := bitmapFile.Read(b)
-		require.NoError(t, err)
-		require.Equal(t, 8, read)
-
-		flags := binary.BigEndian.Uint16(b[6:])
-		if flags != 5 {
-			return false
-		}
+		gittest.TestBitmapHasHashcache(t, bitmapPath)
 	}
-
-	return true
 }
 
 func TestRepackFullFailure(t *testing.T) {
@@ -227,20 +212,8 @@ func TestRepackFullDeltaIslands(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	testCases := []struct {
-		desc    string
-		outcome deltaIslandOutcome
-		ctx     context.Context
-	}{
-		{desc: "are created by default", outcome: expectDeltaIslands, ctx: ctx},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			testDeltaIslands(t, testRepoPath, tc.outcome, func() error {
-				_, err := client.RepackFull(tc.ctx, &gitalypb.RepackFullRequest{Repository: testRepo})
-				return err
-			})
-		})
-	}
+	gittest.TestDeltaIslands(t, testRepoPath, func() error {
+		_, err := client.RepackFull(ctx, &gitalypb.RepackFullRequest{Repository: testRepo})
+		return err
+	})
 }