Merge branch 'pks-object-pool-enable-revlist-based-connectivity-check' into 'master'

objectpool: Enable rev-list based connectivity checks

Closes #4489

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5764

Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>

3 files changed, 16 insertions, 69 deletions

diff --git a/internal/gitaly/service/objectpool/alternates.go b/internal/gitaly/service/objectpool/alternates.go
index 89486b990..db8e4f578 100644
--- a/internal/gitaly/service/objectpool/alternates.go
+++ b/internal/gitaly/service/objectpool/alternates.go
@@ -10,13 +10,11 @@ import (
 	"time"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
 )
@@ -204,35 +202,17 @@ func (s *server) removeAlternatesIfOk(ctx context.Context, repo *localrepo.Repo,
 		}
 	}()
 
-	var err error
-	var cmd *command.Command
-
-	if featureflag.RevlistForConnectivity.IsEnabled(ctx) {
-		// The choice here of git rev-list is for performance reasons.
-		// git fsck --connectivity-only performed badly for large
-		// repositories. The reasons are detailed in https://lore.kernel.org/git/9304B938-4A59-456B-B091-DBBCAA1823B2@gmail.com/
-		cmd, err = repo.Exec(ctx, git.Command{
-			Name: "rev-list",
-			Flags: []git.Option{
-				git.Flag{Name: "--objects"},
-				git.Flag{Name: "--all"},
-				git.Flag{Name: "--quiet"},
-			},
-		})
-	} else {
-		cmd, err = repo.Exec(ctx, git.Command{
-			Name:  "fsck",
-			Flags: []git.Option{git.Flag{Name: "--connectivity-only"}},
-		}, git.WithConfig(git.ConfigPair{
-			// Starting with Git's f30e4d854b (fsck: verify commit graph when implicitly
-			// enabled, 2021-10-15), git-fsck(1) will check the commit graph for consistency
-			// even if `core.commitGraph` is not enabled explicitly. We do not want to verify
-			// whether the commit graph is consistent though, but only care about connectivity,
-			// so we now explicitly disable usage of the commit graph.
-			Key: "core.commitGraph", Value: "false",
-		}))
-	}
-
+	// The choice here of git rev-list is for performance reasons.
+	// git fsck --connectivity-only performed badly for large
+	// repositories. The reasons are detailed in https://lore.kernel.org/git/9304B938-4A59-456B-B091-DBBCAA1823B2@gmail.com/
+	cmd, err := repo.Exec(ctx, git.Command{
+		Name: "rev-list",
+		Flags: []git.Option{
+			git.Flag{Name: "--objects"},
+			git.Flag{Name: "--all"},
+			git.Flag{Name: "--quiet"},
+		},
+	})
 	if err != nil {
 		return err
 	}
diff --git a/internal/gitaly/service/objectpool/alternates_test.go b/internal/gitaly/service/objectpool/alternates_test.go
index 21fc62db7..bb8351d93 100644
--- a/internal/gitaly/service/objectpool/alternates_test.go
+++ b/internal/gitaly/service/objectpool/alternates_test.go
@@ -1,7 +1,6 @@
 package objectpool
 
 import (
-	"context"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -11,7 +10,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -20,15 +18,8 @@ import (
 
 func TestDisconnectGitAlternates(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(
-		featureflag.RevlistForConnectivity,
-	).Run(
-		t,
-		testDisconnectGitAlternates,
-	)
-}
 
-func testDisconnectGitAlternates(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, repoProto, repoPath, _, client := setup(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -67,13 +58,8 @@ func testDisconnectGitAlternates(t *testing.T, ctx context.Context) {
 
 func TestDisconnectGitAlternatesNoAlternates(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.RevlistForConnectivity).Run(
-		t,
-		testDisconnectGitAlternatesNoAlternates,
-	)
-}
 
-func testDisconnectGitAlternatesNoAlternates(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, repoProto, repoPath, _, client := setup(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -89,13 +75,8 @@ func testDisconnectGitAlternatesNoAlternates(t *testing.T, ctx context.Context)
 
 func TestDisconnectGitAlternatesUnexpectedAlternates(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.RevlistForConnectivity).Run(
-		t,
-		testDisconnectGitAlternatesUnexpectedAlternates,
-	)
-}
 
-func testDisconnectGitAlternatesUnexpectedAlternates(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, _, _, _, client := setup(t, ctx)
 
 	testCases := []struct {
@@ -127,13 +108,9 @@ func testDisconnectGitAlternatesUnexpectedAlternates(t *testing.T, ctx context.C
 
 func TestRemoveAlternatesIfOk(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.RevlistForConnectivity).Run(
-		t,
-		testRemoveAlternatesIfOk,
-	)
-}
 
-func testRemoveAlternatesIfOk(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
+
 	t.Run("pack files are missing", func(t *testing.T) {
 		cfg, repoProto, repoPath, _, _ := setup(t, ctx)
 		srv := server{gitCmdFactory: gittest.NewCommandFactory(t, cfg)}
diff --git a/internal/metadata/featureflag/ff_revlist_for_connectivity.go b/internal/metadata/featureflag/ff_revlist_for_connectivity.go
deleted file mode 100644
index 09c667fb4..000000000
--- a/internal/metadata/featureflag/ff_revlist_for_connectivity.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// RevlistForConnectivity causes the connectivity check when removing alternates
-// files to use git-rev-list instead of git-fsck
-var RevlistForConnectivity = NewFeatureFlag(
-	"revlist_for_connectivity",
-	"v15.5.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4489",
-	false,
-)