housekeeping: Introduce configuration for WriteCommitGraph

Make the behaviour of whether we rewrite the commit-graph or not
controllable by callers and adjust callers accordingly. This new
configuration will be used to rewrite the commit-graph in more cases
than only missing bitmaps, as it is done right now.

While at it, improve the error messages to report what we're doing
instead of dictating policy like we do right now, where we ask the
caller to "please remove the commit-graph".

7 files changed, 90 insertions, 39 deletions

diff --git a/internal/git/housekeeping/commit_graph.go b/internal/git/housekeeping/commit_graph.go
index 2728274f4..351c10822 100644
--- a/internal/git/housekeeping/commit_graph.go
+++ b/internal/git/housekeeping/commit_graph.go
@@ -14,38 +14,56 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 )
 
-// WriteCommitGraph updates the commit-graph in the given repository. The commit-graph is updated
-// incrementally, except in the case where it doesn't exist yet or in case it is detected that the
-// commit-graph is missing bloom filters.
-func WriteCommitGraph(ctx context.Context, repo *localrepo.Repo) error {
+// WriteCommitGraphConfig contains configuration that can be passed to WriteCommitGraph to alter its
+// default behaviour.
+type WriteCommitGraphConfig struct {
+	// ReplaceChain causes WriteCommitGraph to rewrite the complete commit-graph chain. This is
+	// a lot more expensive than the default, incremental update of the commit-graph chains but
+	// may be required in certain cases to fix up commit-graphs.
+	ReplaceChain bool
+}
+
+// WriteCommitGraphConfigForRepository returns the optimal default-configuration for the given repo.
+// By default, the configuration will ask for an incremental commit-graph update. If the preexisting
+// commit-graph is missing bloom filters though then the whole commit-graph chain will be rewritten.
+func WriteCommitGraphConfigForRepository(ctx context.Context, repo *localrepo.Repo) (WriteCommitGraphConfig, error) {
 	repoPath, err := repo.Path()
 	if err != nil {
-		return err
+		return WriteCommitGraphConfig{}, err
 	}
 
 	missingBloomFilters := true
 	if _, err := os.Stat(filepath.Join(repoPath, stats.CommitGraphRelPath)); err != nil {
 		if !errors.Is(err, os.ErrNotExist) {
-			return helper.ErrInternal(fmt.Errorf("remove commit graph file: %w", err))
+			return WriteCommitGraphConfig{}, helper.ErrInternal(fmt.Errorf("statting commit-graph: %w", err))
 		}
 
-		// objects/info/commit-graph file doesn't exists
-		// check if commit-graph chain exists and includes Bloom filters
 		if missingBloomFilters, err = stats.IsMissingBloomFilters(repoPath); err != nil {
-			return helper.ErrInternal(fmt.Errorf("should remove commit graph chain: %w", err))
+			return WriteCommitGraphConfig{}, helper.ErrInternal(fmt.Errorf("checking for missing bloom-filters: %w", err))
 		}
 	}
 
+	return WriteCommitGraphConfig{
+		// If the commit-graph doesn't use bloom filters then an incremental update to the
+		// commit-graphs will _not_ write bloom filters for the newly added slice. Because
+		// this is an important optimization for us that speeds up computation of changed
+		// paths we thus rewrite the complete commit-graph chain if bloom filters do not yet
+		// exist.
+		ReplaceChain: missingBloomFilters,
+	}, nil
+}
+
+// WriteCommitGraph updates the commit-graph in the given repository. The commit-graph is updated
+// incrementally, except in the case where it doesn't exist yet or in case it is detected that the
+// commit-graph is missing bloom filters.
+func WriteCommitGraph(ctx context.Context, repo *localrepo.Repo, cfg WriteCommitGraphConfig) error {
 	flags := []git.Option{
 		git.Flag{Name: "--reachable"},
 		git.Flag{Name: "--changed-paths"}, // enables Bloom filters
 		git.ValueFlag{Name: "--size-multiple", Value: "4"},
 	}
 
-	if missingBloomFilters {
-		// if commit graph doesn't use Bloom filters we instruct operation to replace
-		// existent commit graph with the new one
-		// https://git-scm.com/docs/git-commit-graph#Documentation/git-commit-graph.txt-emwriteem
+	if cfg.ReplaceChain {
 		flags = append(flags, git.Flag{Name: "--split=replace"})
 	} else {
 		flags = append(flags, git.Flag{Name: "--split"})
diff --git a/internal/git/housekeeping/optimize_repository.go b/internal/git/housekeeping/optimize_repository.go
index 28f984287..54705c2d0 100644
--- a/internal/git/housekeeping/optimize_repository.go
+++ b/internal/git/housekeeping/optimize_repository.go
@@ -88,7 +88,7 @@ func optimizeRepository(ctx context.Context, m *RepositoryManager, repo *localre
 	timer.ObserveDuration()
 
 	timer = prometheus.NewTimer(m.tasksLatency.WithLabelValues("commit-graph"))
-	if didWriteCommitGraph, err := writeCommitGraphIfNeeded(ctx, repo, didRepack); err != nil {
+	if didWriteCommitGraph, _, err := writeCommitGraphIfNeeded(ctx, repo, didRepack); err != nil {
 		optimizations["written_commit_graph"] = "failure"
 		return fmt.Errorf("could not write commit-graph: %w", err)
 	} else if didWriteCommitGraph {
@@ -354,45 +354,45 @@ func estimateLooseObjectCount(repo *localrepo.Repo, cutoffDate time.Time) (int64
 }
 
 // writeCommitGraphIfNeeded writes the commit-graph if required.
-func writeCommitGraphIfNeeded(ctx context.Context, repo *localrepo.Repo, didRepack bool) (bool, error) {
-	needed, err := needsWriteCommitGraph(ctx, repo, didRepack)
+func writeCommitGraphIfNeeded(ctx context.Context, repo *localrepo.Repo, didRepack bool) (bool, WriteCommitGraphConfig, error) {
+	needed, cfg, err := needsWriteCommitGraph(ctx, repo, didRepack)
 	if err != nil {
-		return false, fmt.Errorf("determining whether repo needs commit-graph update: %w", err)
+		return false, WriteCommitGraphConfig{}, fmt.Errorf("determining whether repo needs commit-graph update: %w", err)
 	}
 	if !needed {
-		return false, nil
+		return false, WriteCommitGraphConfig{}, nil
 	}
 
-	if err := WriteCommitGraph(ctx, repo); err != nil {
-		return true, fmt.Errorf("writing commit-graph: %w", err)
+	if err := WriteCommitGraph(ctx, repo, cfg); err != nil {
+		return true, cfg, fmt.Errorf("writing commit-graph: %w", err)
 	}
 
-	return true, nil
+	return true, cfg, nil
 }
 
 // needsWriteCommitGraph determines whether we need to write the commit-graph.
-func needsWriteCommitGraph(ctx context.Context, repo *localrepo.Repo, didRepack bool) (bool, error) {
+func needsWriteCommitGraph(ctx context.Context, repo *localrepo.Repo, didRepack bool) (bool, WriteCommitGraphConfig, error) {
 	looseRefs, packedRefsSize, err := countLooseAndPackedRefs(ctx, repo)
 	if err != nil {
-		return false, fmt.Errorf("counting refs: %w", err)
+		return false, WriteCommitGraphConfig{}, fmt.Errorf("counting refs: %w", err)
 	}
 
 	// 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, nil
+		return false, WriteCommitGraphConfig{}, 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, nil
+		return true, WriteCommitGraphConfig{}, nil
 	}
 
 	repoPath, err := repo.Path()
 	if err != nil {
-		return false, fmt.Errorf("getting repository path: %w", err)
+		return false, WriteCommitGraphConfig{}, fmt.Errorf("getting repository path: %w", err)
 	}
 
 	// Bloom filters are part of the commit-graph and allow us to efficiently determine which
@@ -401,13 +401,15 @@ func needsWriteCommitGraph(ctx context.Context, repo *localrepo.Repo, didRepack
 	// whole commit-graph to generate them.
 	missingBloomFilters, err := stats.IsMissingBloomFilters(repoPath)
 	if err != nil {
-		return false, fmt.Errorf("checking for bloom filters: %w", err)
+		return false, WriteCommitGraphConfig{}, fmt.Errorf("checking for bloom filters: %w", err)
 	}
 	if missingBloomFilters {
-		return true, nil
+		return true, WriteCommitGraphConfig{
+			ReplaceChain: true,
+		}, nil
 	}
 
-	return false, nil
+	return false, WriteCommitGraphConfig{}, nil
 }
 
 // pruneIfNeeded removes objects from the repository which are either unreachable or which are
diff --git a/internal/git/housekeeping/optimize_repository_test.go b/internal/git/housekeeping/optimize_repository_test.go
index 09c5476c4..1665653a7 100644
--- a/internal/git/housekeeping/optimize_repository_test.go
+++ b/internal/git/housekeeping/optimize_repository_test.go
@@ -978,6 +978,7 @@ func TestWriteCommitGraphIfNeeded(t *testing.T) {
 		setup               func(t *testing.T) (*gitalypb.Repository, string)
 		didRepack           bool
 		expectedWrite       bool
+		expectedCfg         WriteCommitGraphConfig
 		expectedCommitGraph bool
 	}{
 		{
@@ -1005,7 +1006,10 @@ func TestWriteCommitGraphIfNeeded(t *testing.T) {
 				gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(), gittest.WithBranch("main"))
 				return repoProto, repoPath
 			},
-			expectedWrite:       true,
+			expectedWrite: true,
+			expectedCfg: WriteCommitGraphConfig{
+				ReplaceChain: true,
+			},
 			expectedCommitGraph: true,
 		},
 		{
@@ -1024,7 +1028,10 @@ func TestWriteCommitGraphIfNeeded(t *testing.T) {
 
 				return repoProto, repoPath
 			},
-			expectedWrite:       true,
+			expectedWrite: true,
+			expectedCfg: WriteCommitGraphConfig{
+				ReplaceChain: true,
+			},
 			expectedCommitGraph: true,
 		},
 		{
@@ -1040,7 +1047,10 @@ func TestWriteCommitGraphIfNeeded(t *testing.T) {
 
 				return repoProto, repoPath
 			},
-			expectedWrite:       true,
+			expectedWrite: true,
+			expectedCfg: WriteCommitGraphConfig{
+				ReplaceChain: true,
+			},
 			expectedCommitGraph: true,
 		},
 		{
@@ -1087,9 +1097,10 @@ func TestWriteCommitGraphIfNeeded(t *testing.T) {
 			repoProto, repoPath := tc.setup(t)
 			repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
-			didWrite, err := writeCommitGraphIfNeeded(ctx, repo, tc.didRepack)
+			didWrite, writeCommitGraphCfg, err := writeCommitGraphIfNeeded(ctx, repo, tc.didRepack)
 			require.NoError(t, err)
 			require.Equal(t, tc.expectedWrite, didWrite)
+			require.Equal(t, tc.expectedCfg, writeCommitGraphCfg)
 
 			commitGraphPath := filepath.Join(repoPath, "objects", "info", "commit-graphs", "commit-graph-chain")
 			if tc.expectedCommitGraph {
diff --git a/internal/gitaly/service/repository/commit_graph.go b/internal/gitaly/service/repository/commit_graph.go
index 1d5d34a57..1e340b834 100644
--- a/internal/gitaly/service/repository/commit_graph.go
+++ b/internal/gitaly/service/repository/commit_graph.go
@@ -24,7 +24,12 @@ func (s *server) WriteCommitGraph(
 		return nil, helper.ErrInvalidArgumentf("unsupported split strategy: %v", in.GetSplitStrategy())
 	}
 
-	if err := housekeeping.WriteCommitGraph(ctx, repo); err != nil {
+	writeCommitGraphCfg, err := housekeeping.WriteCommitGraphConfigForRepository(ctx, repo)
+	if err != nil {
+		return nil, helper.ErrInternalf("getting commit-graph config: %w", err)
+	}
+
+	if err := housekeeping.WriteCommitGraph(ctx, repo, writeCommitGraphCfg); err != nil {
 		return nil, err
 	}
 
diff --git a/internal/gitaly/service/repository/commit_graph_test.go b/internal/gitaly/service/repository/commit_graph_test.go
index 2fc397270..ef29b9dcf 100644
--- a/internal/gitaly/service/repository/commit_graph_test.go
+++ b/internal/gitaly/service/repository/commit_graph_test.go
@@ -142,12 +142,12 @@ func TestWriteCommitGraph_validationChecks(t *testing.T) {
 		{
 			desc:   "invalid storage",
 			req:    &gitalypb.WriteCommitGraphRequest{Repository: &gitalypb.Repository{StorageName: "invalid"}},
-			expErr: status.Error(codes.InvalidArgument, `GetStorageByName: no such storage: "invalid"`),
+			expErr: status.Error(codes.InvalidArgument, `getting commit-graph config: GetStorageByName: no such storage: "invalid"`),
 		},
 		{
 			desc:   "not existing repository",
 			req:    &gitalypb.WriteCommitGraphRequest{Repository: &gitalypb.Repository{StorageName: repo.StorageName, RelativePath: "invalid"}},
-			expErr: status.Error(codes.NotFound, fmt.Sprintf(`GetRepoPath: not a git repository: "%s/invalid"`, cfg.Storages[0].Path)),
+			expErr: status.Error(codes.NotFound, fmt.Sprintf(`getting commit-graph config: GetRepoPath: not a git repository: "%s/invalid"`, cfg.Storages[0].Path)),
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
diff --git a/internal/gitaly/service/repository/gc.go b/internal/gitaly/service/repository/gc.go
index 8bce85540..6ecf153ac 100644
--- a/internal/gitaly/service/repository/gc.go
+++ b/internal/gitaly/service/repository/gc.go
@@ -51,7 +51,12 @@ func (s *server) GarbageCollect(ctx context.Context, in *gitalypb.GarbageCollect
 		return nil, err
 	}
 
-	if err := housekeeping.WriteCommitGraph(ctx, repo); err != nil {
+	writeCommitGraphCfg, err := housekeeping.WriteCommitGraphConfigForRepository(ctx, repo)
+	if err != nil {
+		return nil, helper.ErrInternalf("getting commit-graph config: %w", err)
+	}
+
+	if err := housekeeping.WriteCommitGraph(ctx, repo, writeCommitGraphCfg); err != nil {
 		return nil, err
 	}
 
diff --git a/internal/gitaly/service/repository/repack.go b/internal/gitaly/service/repository/repack.go
index 752e3c4c3..51164133d 100644
--- a/internal/gitaly/service/repository/repack.go
+++ b/internal/gitaly/service/repository/repack.go
@@ -40,7 +40,12 @@ func (s *server) RepackFull(ctx context.Context, in *gitalypb.RepackFullRequest)
 		return nil, helper.ErrInternalf("repacking objects: %w", err)
 	}
 
-	if err := housekeeping.WriteCommitGraph(ctx, repo); err != nil {
+	writeCommitGraphCfg, err := housekeeping.WriteCommitGraphConfigForRepository(ctx, repo)
+	if err != nil {
+		return nil, helper.ErrInternalf("getting commit-graph config: %w", err)
+	}
+
+	if err := housekeeping.WriteCommitGraph(ctx, repo, writeCommitGraphCfg); err != nil {
 		return nil, helper.ErrInternalf("writing commit-graph: %w", err)
 	}
 
@@ -64,7 +69,12 @@ func (s *server) RepackIncremental(ctx context.Context, in *gitalypb.RepackIncre
 		return nil, helper.ErrInternalf("repacking objects: %w", err)
 	}
 
-	if err := housekeeping.WriteCommitGraph(ctx, repo); err != nil {
+	writeCommitGraphCfg, err := housekeeping.WriteCommitGraphConfigForRepository(ctx, repo)
+	if err != nil {
+		return nil, helper.ErrInternalf("getting commit-graph config: %w", err)
+	}
+
+	if err := housekeeping.WriteCommitGraph(ctx, repo, writeCommitGraphCfg); err != nil {
 		return nil, helper.ErrInternalf("writing commit-graph: %w", err)
 	}