housekeeping: Move commit-graph policy into optimization strategypks-git-maintenance-strategies

Move both the data gathering and policy for when we want to write the
commit-graph in a repository into the optimization strategy interface
via a new `ShouldWriteCommitGraph()` method. Split up tests into three
parts:

    - The first part tests the data-gathering done by the heuristical
      optimization strategy.

    - The second part tests the policy.

    - The third part tests that we correctly honor the policy when we
      drive the mechanism.

The overall policy should remain unchanged.

4 files changed, 214 insertions, 212 deletions

diff --git a/internal/git/housekeeping/optimization_strategy.go b/internal/git/housekeeping/optimization_strategy.go
index 5a1129a72..41a38980f 100644
--- a/internal/git/housekeeping/optimization_strategy.go
+++ b/internal/git/housekeeping/optimization_strategy.go
@@ -27,6 +27,9 @@ type OptimizationStrategy interface {
 	// ShouldRepackReferences determines whether the repository's references need to be
 	// repacked.
 	ShouldRepackReferences() bool
+	// ShouldWriteCommitGraph determines whether we need to write the commit-graph and how it
+	// should be written.
+	ShouldWriteCommitGraph() (bool, WriteCommitGraphConfig)
 }
 
 // HeuristicalOptimizationStrategy is an optimization strategy that is based on a set of
@@ -40,6 +43,7 @@ type HeuristicalOptimizationStrategy struct {
 	packedRefsSize          int64
 	hasAlternate            bool
 	hasBitmap               bool
+	hasBloomFilters         bool
 	isObjectPool            bool
 }
 
@@ -71,6 +75,12 @@ func NewHeuristicalOptimizationStrategy(ctx context.Context, repo *localrepo.Rep
 		return strategy, fmt.Errorf("checking for bitmap: %w", err)
 	}
 
+	missingBloomFilters, err := stats.IsMissingBloomFilters(repoPath)
+	if err != nil {
+		return strategy, fmt.Errorf("checking for bloom filters: %w", err)
+	}
+	strategy.hasBloomFilters = !missingBloomFilters
+
 	strategy.largestPackfileSizeInMB, strategy.packfileCount, err = packfileSizeAndCount(repo)
 	if err != nil {
 		return strategy, fmt.Errorf("checking largest packfile size: %w", err)
@@ -280,18 +290,14 @@ func estimateLooseObjectCount(repo *localrepo.Repo, cutoffDate time.Time) (int64
 	return looseObjects * 256, nil
 }
 
-// needsWriteCommitGraph determines whether we need to write the commit-graph.
-func needsWriteCommitGraph(ctx context.Context, repo *localrepo.Repo, didRepack, didPrune bool) (bool, WriteCommitGraphConfig, error) {
-	looseRefs, packedRefsSize, err := countLooseAndPackedRefs(ctx, repo)
-	if err != nil {
-		return false, WriteCommitGraphConfig{}, fmt.Errorf("counting refs: %w", err)
-	}
-
+// ShouldWriteCommitGraph determines whether we need to write the commit-graph and how it should be
+// written.
+func (s HeuristicalOptimizationStrategy) ShouldWriteCommitGraph() (bool, WriteCommitGraphConfig) {
 	// If the repository doesn't have any references at all then there is no point in writing
 	// commit-graphs given that it would only contain reachable objects, of which there are
 	// none.
-	if looseRefs == 0 && packedRefsSize == 0 {
-		return false, WriteCommitGraphConfig{}, nil
+	if s.looseRefsCount == 0 && s.packedRefsSize == 0 {
+		return false, WriteCommitGraphConfig{}
 	}
 
 	// When we have pruned objects in the repository then it may happen that the commit-graph
@@ -302,38 +308,29 @@ func needsWriteCommitGraph(ctx context.Context, repo *localrepo.Repo, didRepack,
 	//
 	// To fix this case we will replace the complete commit-chain when we have pruned objects
 	// from the repository.
-	if didPrune {
+	if s.ShouldPruneObjects() {
 		return true, WriteCommitGraphConfig{
 			ReplaceChain: true,
-		}, nil
+		}
 	}
 
 	// When we repacked the repository then chances are high that we have accumulated quite some
 	// objects since the last time we wrote a commit-graph.
-	if didRepack {
-		return true, WriteCommitGraphConfig{}, nil
-	}
-
-	repoPath, err := repo.Path()
-	if err != nil {
-		return false, WriteCommitGraphConfig{}, fmt.Errorf("getting repository path: %w", err)
+	if needsRepacking, _ := s.ShouldRepackObjects(); needsRepacking {
+		return true, WriteCommitGraphConfig{}
 	}
 
 	// Bloom filters are part of the commit-graph and allow us to efficiently determine which
 	// paths have been modified in a given commit without having to look into the object
 	// database. In the past we didn't compute bloom filters at all, so we want to rewrite the
 	// whole commit-graph to generate them.
-	missingBloomFilters, err := stats.IsMissingBloomFilters(repoPath)
-	if err != nil {
-		return false, WriteCommitGraphConfig{}, fmt.Errorf("checking for bloom filters: %w", err)
-	}
-	if missingBloomFilters {
+	if !s.hasBloomFilters {
 		return true, WriteCommitGraphConfig{
 			ReplaceChain: true,
-		}, nil
+		}
 	}
 
-	return false, WriteCommitGraphConfig{}, nil
+	return false, WriteCommitGraphConfig{}
 }
 
 // ShouldPruneObjects determines whether the repository has stale objects that should be pruned.
diff --git a/internal/git/housekeeping/optimization_strategy_test.go b/internal/git/housekeeping/optimization_strategy_test.go
index c8fe9c064..97c3111bf 100644
--- a/internal/git/housekeeping/optimization_strategy_test.go
+++ b/internal/git/housekeeping/optimization_strategy_test.go
@@ -132,6 +132,67 @@ func TestNewHeuristicalOptimizationStrategy_variousParameters(t *testing.T) {
 				hasBitmap: true,
 			},
 		},
+		{
+			desc: "existing unsplit commit-graph with bloom filters",
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
+					SkipCreationViaService: true,
+				})
+				gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
+
+				// Write a non-split commit-graph with bloom filters. We should
+				// always rewrite the commit-graphs when we're not using a split
+				// commit-graph. We make sure to add bloom filters via
+				// `--changed-paths` given that it would otherwise cause us to
+				// rewrite the graph regardless of whether the graph is split or not
+				// if they were missing.
+				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--reachable", "--changed-paths")
+
+				return repoProto
+			},
+			expectedStrategy: HeuristicalOptimizationStrategy{
+				looseRefsCount: 1,
+			},
+		},
+		{
+			desc: "existing split commit-graph without bloom filters",
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
+					SkipCreationViaService: true,
+				})
+				gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
+
+				// Generate a split commit-graph, but don't enable computation of
+				// changed paths. This should trigger a rewrite so that we can
+				// recompute all graphs and generate the changed paths.
+				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--reachable", "--split")
+
+				return repoProto
+			},
+			expectedStrategy: HeuristicalOptimizationStrategy{
+				looseRefsCount: 1,
+			},
+		},
+		{
+			desc: "existing split commit-graph with bloom filters",
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
+					SkipCreationViaService: true,
+				})
+				gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
+
+				// Write a split commit-graph with bitmaps. This is the state we
+				// want to be in, so there is no write required if we didn't also
+				// repack objects.
+				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--reachable", "--split", "--changed-paths")
+
+				return repoProto
+			},
+			expectedStrategy: HeuristicalOptimizationStrategy{
+				looseRefsCount:  1,
+				hasBloomFilters: true,
+			},
+		},
 	} {
 		tc := tc
 
@@ -534,6 +595,83 @@ func TestHeuristicalOptimizationStrategy_ShouldRepackReferences(t *testing.T) {
 	}
 }
 
+func TestHeuristicalOptimizationStrategy_NeedsWriteCommitGraph(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		desc           string
+		strategy       HeuristicalOptimizationStrategy
+		expectedNeeded bool
+		expectedCfg    WriteCommitGraphConfig
+	}{
+		{
+			desc:           "empty repository",
+			expectedNeeded: false,
+		},
+		{
+			desc: "repository with objects but no refs",
+			strategy: HeuristicalOptimizationStrategy{
+				looseObjectCount: 9000,
+			},
+			expectedNeeded: false,
+		},
+		{
+			desc: "repository without bloom filters",
+			strategy: HeuristicalOptimizationStrategy{
+				looseRefsCount: 1,
+			},
+			expectedNeeded: true,
+			expectedCfg: WriteCommitGraphConfig{
+				ReplaceChain: true,
+			},
+		},
+		{
+			desc: "repository with split commit-graph with bitmap without repack",
+			strategy: HeuristicalOptimizationStrategy{
+				looseRefsCount:  1,
+				hasBloomFilters: true,
+			},
+			// We use the information about whether we repacked objects as an indicator
+			// whether something has changed in the repository. If it didn't, then we
+			// assume no new objects exist and thus we don't rewrite the commit-graph.
+			expectedNeeded: false,
+		},
+		{
+			desc: "repository with split commit-graph with bitmap with repack",
+			strategy: HeuristicalOptimizationStrategy{
+				looseRefsCount:   1,
+				hasBloomFilters:  true,
+				looseObjectCount: 9000,
+			},
+			// When we have a valid commit-graph, but objects have been repacked, we
+			// assume that there are new objects in the repository. So consequentially,
+			// we should write the commit-graphs.
+			expectedNeeded: true,
+		},
+		{
+			desc: "repository with split commit-graph with bitmap with pruned objects",
+			strategy: HeuristicalOptimizationStrategy{
+				looseRefsCount:      1,
+				hasBloomFilters:     true,
+				oldLooseObjectCount: 9000,
+			},
+			// When we have a valid commit-graph, but objects have been repacked, we
+			// assume that there are new objects in the repository. So consequentially,
+			// we should write the commit-graphs.
+			expectedNeeded: true,
+			expectedCfg: WriteCommitGraphConfig{
+				ReplaceChain: true,
+			},
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			needed, writeCommitGraphCfg := tc.strategy.ShouldWriteCommitGraph()
+			require.Equal(t, tc.expectedNeeded, needed)
+			require.Equal(t, tc.expectedCfg, writeCommitGraphCfg)
+		})
+	}
+}
+
 func TestEstimateLooseObjectCount(t *testing.T) {
 	t.Parallel()
 
@@ -626,6 +764,8 @@ type mockOptimizationStrategy struct {
 	repackObjectsCfg       RepackObjectsConfig
 	shouldPruneObjects     bool
 	shouldRepackReferences bool
+	shouldWriteCommitGraph bool
+	writeCommitGraphCfg    WriteCommitGraphConfig
 }
 
 func (m mockOptimizationStrategy) ShouldRepackObjects() (bool, RepackObjectsConfig) {
@@ -639,3 +779,7 @@ func (m mockOptimizationStrategy) ShouldPruneObjects() bool {
 func (m mockOptimizationStrategy) ShouldRepackReferences() bool {
 	return m.shouldRepackReferences
 }
+
+func (m mockOptimizationStrategy) ShouldWriteCommitGraph() (bool, WriteCommitGraphConfig) {
+	return m.shouldWriteCommitGraph, m.writeCommitGraphCfg
+}
diff --git a/internal/git/housekeeping/optimize_repository.go b/internal/git/housekeeping/optimize_repository.go
index 654793aef..a8111472f 100644
--- a/internal/git/housekeeping/optimize_repository.go
+++ b/internal/git/housekeeping/optimize_repository.go
@@ -137,7 +137,7 @@ func optimizeRepository(
 	timer.ObserveDuration()
 
 	timer = prometheus.NewTimer(m.tasksLatency.WithLabelValues("commit-graph"))
-	if didWriteCommitGraph, writeCommitGraphCfg, err := writeCommitGraphIfNeeded(ctx, repo, didRepack, didPrune); err != nil {
+	if didWriteCommitGraph, writeCommitGraphCfg, err := writeCommitGraphIfNeeded(ctx, repo, cfg.strategy); err != nil {
 		optimizations["written_commit_graph_full"] = "failure"
 		optimizations["written_commit_graph_incremental"] = "failure"
 		return fmt.Errorf("could not write commit-graph: %w", err)
@@ -170,11 +170,8 @@ func repackIfNeeded(ctx context.Context, repo *localrepo.Repo, strategy Optimiza
 }
 
 // writeCommitGraphIfNeeded writes the commit-graph if required.
-func writeCommitGraphIfNeeded(ctx context.Context, repo *localrepo.Repo, didRepack, didPrune bool) (bool, WriteCommitGraphConfig, error) {
-	needed, cfg, err := needsWriteCommitGraph(ctx, repo, didRepack, didPrune)
-	if err != nil {
-		return false, WriteCommitGraphConfig{}, fmt.Errorf("determining whether repo needs commit-graph update: %w", err)
-	}
+func writeCommitGraphIfNeeded(ctx context.Context, repo *localrepo.Repo, strategy OptimizationStrategy) (bool, WriteCommitGraphConfig, error) {
+	needed, cfg := strategy.ShouldWriteCommitGraph()
 	if !needed {
 		return false, WriteCommitGraphConfig{}, nil
 	}
diff --git a/internal/git/housekeeping/optimize_repository_test.go b/internal/git/housekeeping/optimize_repository_test.go
index 9dbd06e4f..a43d544c0 100644
--- a/internal/git/housekeeping/optimize_repository_test.go
+++ b/internal/git/housekeeping/optimize_repository_test.go
@@ -562,189 +562,48 @@ func TestPruneIfNeeded(t *testing.T) {
 }
 
 func TestWriteCommitGraphIfNeeded(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 
-	for _, tc := range []struct {
-		desc                string
-		setup               func(t *testing.T) (*gitalypb.Repository, string)
-		didRepack           bool
-		didPrune            bool
-		expectedWrite       bool
-		expectedCfg         WriteCommitGraphConfig
-		expectedCommitGraph bool
-	}{
-		{
-			desc: "empty repository",
-			setup: func(t *testing.T) (*gitalypb.Repository, string) {
-				return gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
-					SkipCreationViaService: true,
-				})
-			},
-			didRepack:     true,
-			didPrune:      true,
-			expectedWrite: false,
-		},
-		{
-			desc: "repository with objects but no refs",
-			setup: func(t *testing.T) (*gitalypb.Repository, string) {
-				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
-					SkipCreationViaService: true,
-				})
-				gittest.WriteBlob(t, cfg, repoPath, []byte("something"))
-				return repoProto, repoPath
-			},
-			didRepack:     true,
-			didPrune:      true,
-			expectedWrite: false,
-		},
-		{
-			desc: "repository without commit-graph",
-			setup: func(t *testing.T) (*gitalypb.Repository, string) {
-				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
-					SkipCreationViaService: true,
-				})
-				gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
-				return repoProto, repoPath
-			},
-			expectedWrite: true,
-			expectedCfg: WriteCommitGraphConfig{
-				ReplaceChain: true,
-			},
-			expectedCommitGraph: true,
-		},
-		{
-			desc: "repository with old-style unsplit commit-graph",
-			setup: func(t *testing.T) (*gitalypb.Repository, string) {
-				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
-					SkipCreationViaService: true,
-				})
-				gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
-
-				// Write a non-split commit-graph with bloom filters. We should
-				// always rewrite the commit-graphs when we're not using a split
-				// commit-graph. We make sure to add bloom filters via
-				// `--changed-paths` given that it would otherwise cause us to
-				// rewrite the graph regardless of whether the graph is split or not
-				// if they were missing.
-				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--reachable", "--changed-paths")
-
-				return repoProto, repoPath
-			},
-			expectedWrite: true,
-			expectedCfg: WriteCommitGraphConfig{
-				ReplaceChain: true,
-			},
-			expectedCommitGraph: true,
-		},
-		{
-			desc: "repository with split commit-graph without bitmap",
-			setup: func(t *testing.T) (*gitalypb.Repository, string) {
-				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
-					SkipCreationViaService: true,
-				})
-				gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
-
-				// Generate a split commit-graph, but don't enable computation of
-				// changed paths. This should trigger a rewrite so that we can
-				// recompute all graphs and generate the changed paths.
-				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--reachable", "--split")
-
-				return repoProto, repoPath
-			},
-			expectedWrite: true,
-			expectedCfg: WriteCommitGraphConfig{
-				ReplaceChain: true,
-			},
-			expectedCommitGraph: true,
-		},
-		{
-			desc: "repository with split commit-graph with bitmap without repack",
-			setup: func(t *testing.T) (*gitalypb.Repository, string) {
-				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
-					SkipCreationViaService: true,
-				})
-				gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
-
-				// Write a split commit-graph with bitmaps. This is the state we
-				// want to be in.
-				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--reachable", "--split", "--changed-paths")
-
-				return repoProto, repoPath
-			},
-			// We use the information about whether we repacked objects as an indicator
-			// whether something has changed in the repository. If it didn't, then we
-			// assume no new objects exist and thus we don't rewrite the commit-graph.
-			didRepack:           false,
-			expectedWrite:       false,
-			expectedCommitGraph: true,
-		},
-		{
-			desc: "repository with split commit-graph with bitmap with repack",
-			setup: func(t *testing.T) (*gitalypb.Repository, string) {
-				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
-					SkipCreationViaService: true,
-				})
-				gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
+	t.Run("strategy does not update commit-graph", func(t *testing.T) {
+		repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
+			SkipCreationViaService: true,
+		})
+		repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
-				// Write a split commit-graph with bitmaps. This is the state we
-				// want to be in, so there is no write required if we didn't also
-				// repack objects.
-				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--reachable", "--split", "--changed-paths")
+		gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
 
-				return repoProto, repoPath
-			},
-			// When we have a valid commit-graph, but objects have been repacked, we
-			// assume that there are new objects in the repository. So consequentially,
-			// we should write the commit-graphs.
-			didRepack:           true,
-			expectedWrite:       true,
-			expectedCommitGraph: true,
-		},
-		{
-			desc: "repository with split commit-graph with bitmap with pruned objects",
-			setup: func(t *testing.T) (*gitalypb.Repository, string) {
-				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
-					SkipCreationViaService: true,
-				})
-				gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
+		written, cfg, err := writeCommitGraphIfNeeded(ctx, repo, mockOptimizationStrategy{
+			shouldWriteCommitGraph: false,
+		})
+		require.NoError(t, err)
+		require.False(t, written)
+		require.Equal(t, WriteCommitGraphConfig{}, cfg)
 
-				// Write a split commit-graph with bitmaps. This is the state we
-				// want to be in, so there is no write required if we didn't also
-				// repack objects.
-				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--reachable", "--split", "--changed-paths")
+		require.NoFileExists(t, filepath.Join(repoPath, "objects", "info", "commit-graph"))
+		require.NoDirExists(t, filepath.Join(repoPath, "objects", "info", "commit-graphs"))
+	})
 
-				return repoProto, repoPath
-			},
-			// When we have a valid commit-graph, but objects have been repacked, we
-			// assume that there are new objects in the repository. So consequentially,
-			// we should write the commit-graphs.
-			didPrune:      true,
-			expectedWrite: true,
-			expectedCfg: WriteCommitGraphConfig{
-				ReplaceChain: true,
-			},
-			expectedCommitGraph: true,
-		},
-	} {
-		t.Run(tc.desc, func(t *testing.T) {
-			repoProto, repoPath := tc.setup(t)
-			repo := localrepo.NewTestRepo(t, cfg, repoProto)
+	t.Run("strategy does update commit-graph", func(t *testing.T) {
+		repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
+			SkipCreationViaService: true,
+		})
+		repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
-			didWrite, writeCommitGraphCfg, err := writeCommitGraphIfNeeded(ctx, repo, tc.didRepack, tc.didPrune)
-			require.NoError(t, err)
-			require.Equal(t, tc.expectedWrite, didWrite)
-			require.Equal(t, tc.expectedCfg, writeCommitGraphCfg)
+		gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
 
-			commitGraphPath := filepath.Join(repoPath, "objects", "info", "commit-graphs", "commit-graph-chain")
-			if tc.expectedCommitGraph {
-				require.FileExists(t, commitGraphPath)
-			} else {
-				require.NoFileExists(t, commitGraphPath)
-			}
-			gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "verify")
+		written, cfg, err := writeCommitGraphIfNeeded(ctx, repo, mockOptimizationStrategy{
+			shouldWriteCommitGraph: true,
 		})
-	}
+		require.NoError(t, err)
+		require.True(t, written)
+		require.Equal(t, WriteCommitGraphConfig{}, cfg)
+
+		require.NoFileExists(t, filepath.Join(repoPath, "objects", "info", "commit-graph"))
+		require.DirExists(t, filepath.Join(repoPath, "objects", "info", "commit-graphs"))
+	})
 
 	t.Run("commit-graph with pruned objects", func(t *testing.T) {
 		repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
@@ -774,24 +633,29 @@ func TestWriteCommitGraphIfNeeded(t *testing.T) {
 		require.EqualError(t, verifyCmd.Run(), "exit status 1")
 		require.Equal(t, stderr.String(), fmt.Sprintf("error: Could not read %[1]s\nfailed to parse commit %[1]s from object database for commit-graph\n", unreachableCommitID))
 
-		// Write the commit-graph and pretend that objects have been rewritten, but not
-		// pruned.
-		didWrite, writeCommitGraphCfg, err := writeCommitGraphIfNeeded(ctx, repo, true, false)
+		// Write the commit-graph incrementally.
+		didWrite, writeCommitGraphCfg, err := writeCommitGraphIfNeeded(ctx, repo, mockOptimizationStrategy{
+			shouldWriteCommitGraph: true,
+		})
 		require.NoError(t, err)
 		require.True(t, didWrite)
 		require.Equal(t, WriteCommitGraphConfig{}, writeCommitGraphCfg)
 
-		// When pretending that no objects have been pruned we still observe the same
-		// failure.
+		// We should still observe the failure failure.
 		stderr.Reset()
 		verifyCmd = gittest.NewCommand(t, cfg, "-C", repoPath, "commit-graph", "verify")
 		verifyCmd.Stderr = &stderr
 		require.EqualError(t, verifyCmd.Run(), "exit status 1")
 		require.Equal(t, stderr.String(), fmt.Sprintf("error: Could not read %[1]s\nfailed to parse commit %[1]s from object database for commit-graph\n", unreachableCommitID))
 
-		// Write the commit-graph a second time, but this time we pretend we have just
-		// pruned objects. This should cause the commit-graph to be rewritten.
-		didWrite, writeCommitGraphCfg, err = writeCommitGraphIfNeeded(ctx, repo, false, true)
+		// Write the commit-graph a second time, but this time we ask to rewrite the
+		// commit-graph completely.
+		didWrite, writeCommitGraphCfg, err = writeCommitGraphIfNeeded(ctx, repo, mockOptimizationStrategy{
+			shouldWriteCommitGraph: true,
+			writeCommitGraphCfg: WriteCommitGraphConfig{
+				ReplaceChain: true,
+			},
+		})
 		require.NoError(t, err)
 		require.True(t, didWrite)
 		require.Equal(t, WriteCommitGraphConfig{