Stop creating dangling refsjv-pool-no-dangle

3 files changed, 16 insertions, 73 deletions

diff --git a/changelogs/unreleased/jv-pool-no-dangle.yml b/changelogs/unreleased/jv-pool-no-dangle.yml
index 1100bbdd6..766e038f3 100644
--- a/changelogs/unreleased/jv-pool-no-dangle.yml
+++ b/changelogs/unreleased/jv-pool-no-dangle.yml
@@ -1,5 +1,5 @@
 ---
-title: 'FetchIntoObjectPool: stop creating dangling refs, and remove existing'
+title: 'FetchIntoObjectPool: stop creating dangling refs'
 merge_request: 1459
 author:
 type: fixed
diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 76d4d1777..148581785 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -8,7 +8,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
-	"gitlab.com/gitlab-org/gitaly/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
@@ -74,10 +73,6 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos
 		return err
 	}
 
-	if err := removeDanglingRefs(ctx, o); err != nil {
-		return err
-	}
-
 	packRefs, err := git.Command(ctx, o, "pack-refs", "--all")
 	if err != nil {
 		return err
@@ -91,48 +86,22 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos
 
 const danglingObjectNamespace = "refs/dangling"
 
-// In https://gitlab.com/gitlab-org/gitaly/merge_requests/1297 we
-// introduced a mechanism to protect dangling objects in a pool from
-// being removed by a manual 'git prune' command. It turned out that
-// created way too many refs, so now we want to make sure no such refs
-// are left behind.
-func removeDanglingRefs(ctx context.Context, repo repository.GitRepo) error {
-	forEachRef, err := git.Command(ctx, repo, "for-each-ref", "--format=%(refname)", danglingObjectNamespace)
-	if err != nil {
-		return err
-	}
-
-	updater, err := updateref.New(ctx, repo)
-	if err != nil {
-		return err
-	}
-
-	scanner := bufio.NewScanner(forEachRef)
-	for scanner.Scan() {
-		if err := updater.Delete(scanner.Text()); err != nil {
-			return err
-		}
-	}
-
-	if err := scanner.Err(); err != nil {
-		return err
-	}
-
-	if err := forEachRef.Wait(); err != nil {
-		return fmt.Errorf("git for-each-ref: %v", err)
-	}
-
-	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", "-aidbk",
-	}
-	repackCmd, err := git.Command(ctx, pool, repackArgs...)
+	globalOpts := []git.Option{
+		git.ValueFlag{"-c", "pack.island=" + sourceRefNamespace + "/heads"},
+		git.ValueFlag{"-c", "pack.island=" + sourceRefNamespace + "/tags"},
+		git.ValueFlag{"-c", "pack.writeBitmapHashCache=true"},
+	}
+
+	repackCmd, err := git.SafeCmd(ctx, pool, globalOpts, git.SubCmd{
+		Name: "repack",
+		Flags: []git.Option{
+			git.Flag{"-ad"},
+			git.Flag{"--keep-unreachable"},   // Prevent data loss
+			git.Flag{"--delta-islands"},      // Performance
+			git.Flag{"--write-bitmap-index"}, // Performance
+		},
+	})
 	if err != nil {
 		return err
 	}
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index 749c0bd95..3c2cb70ec 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -20,32 +20,6 @@ const (
 	existingBlob   = "c60514b6d3d6bf4bec1030f70026e34dfbd69ad5"
 )
 
-func TestFetchFromOriginRemoveDanglingRefs(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")
-
-	baseArgs := []string{"-C", pool.FullPath()}
-
-	// Simulate "dangling refs" as created by https://gitlab.com/gitlab-org/gitaly/merge_requests/1297
-	for _, oid := range []string{existingTree, existingCommit, existingBlob} {
-		args := append(baseArgs, "update-ref", "refs/dangling/"+oid, oid)
-		testhelper.MustRunCommand(t, nil, "git", args...)
-	}
-	require.Len(t, listDanglingRefs(t, pool), 3, "test setup sanity check")
-
-	require.NoError(t, pool.FetchFromOrigin(ctx, source), "second fetch (should remove dangling refs)")
-
-	require.Empty(t, listDanglingRefs(t, pool), "dangling refs should be gone")
-}
-
 func TestFetchFromOriginKeepUnreachableObjects(t *testing.T) {
 	source, _, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()