Merge branch 'pks-git-enable-pure-git-rebases' into 'master'

operations: Always use Git implementations of rebases

Closes #5493 and #5524

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

Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

6 files changed, 37 insertions, 186 deletions

diff --git a/internal/featureflag/ff_user_rebase_confirmable_pure_git.go b/internal/featureflag/ff_user_rebase_confirmable_pure_git.go
deleted file mode 100644
index 4ee2df0a8..000000000
--- a/internal/featureflag/ff_user_rebase_confirmable_pure_git.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// UserRebaseConfirmablePureGit will enable the UserRebaseConfirmable RPC to
-// use a pure git implemented rebase instead of git2go.
-var UserRebaseConfirmablePureGit = NewFeatureFlag(
-	"user_rebase_confirmable_pure_git",
-	"v16.3.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/5493",
-	false,
-)
diff --git a/internal/featureflag/ff_user_rebase_to_ref_pure_git.go b/internal/featureflag/ff_user_rebase_to_ref_pure_git.go
deleted file mode 100644
index e81a1f5e0..000000000
--- a/internal/featureflag/ff_user_rebase_to_ref_pure_git.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// UserRebaseToRefPureGit will enable the UserRebaseToRef RPC to
-// use a pure git implemented rebase instead of git2go.
-var UserRebaseToRefPureGit = NewFeatureFlag(
-	"user_rebase_to_ref_pure_git",
-	"v16.4.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/5524",
-	false,
-)
diff --git a/internal/gitaly/service/operations/rebase_confirmable.go b/internal/gitaly/service/operations/rebase_confirmable.go
index ddaf6e944..291ba539e 100644
--- a/internal/gitaly/service/operations/rebase_confirmable.go
+++ b/internal/gitaly/service/operations/rebase_confirmable.go
@@ -6,10 +6,8 @@ import (
 	"strings"
 	"time"
 
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
@@ -61,77 +59,36 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba
 		committer.When = header.Timestamp.AsTime()
 	}
 
-	var newrev git.ObjectID
-	if featureflag.UserRebaseConfirmablePureGit.IsEnabled(ctx) {
-		newrev, err = quarantineRepo.Rebase(
-			ctx,
-			startRevision.String(),
-			oldrev.String(),
-			localrepo.RebaseWithCommitter(committer),
-		)
-		if err != nil {
-			var conflictErr *localrepo.RebaseConflictError
-			if errors.As(err, &conflictErr) {
-				conflictingFilesFromErr := conflictErr.ConflictError.ConflictedFiles()
-				conflictingFiles := make([][]byte, 0, len(conflictingFilesFromErr))
-				for _, conflictingFile := range conflictingFilesFromErr {
-					conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
-				}
-				return structerr.NewFailedPrecondition("rebasing commits: %w", conflictErr).WithDetail(
-					&gitalypb.UserRebaseConfirmableError{
-						Error: &gitalypb.UserRebaseConfirmableError_RebaseConflict{
-							RebaseConflict: &gitalypb.MergeConflictError{
-								ConflictingFiles: conflictingFiles,
-								ConflictingCommitIds: []string{
-									startRevision.String(),
-									oldrev.String(),
-								},
-							},
-						},
-					},
-				)
+	newrev, err := quarantineRepo.Rebase(
+		ctx,
+		startRevision.String(),
+		oldrev.String(),
+		localrepo.RebaseWithCommitter(committer),
+	)
+	if err != nil {
+		var conflictErr *localrepo.RebaseConflictError
+		if errors.As(err, &conflictErr) {
+			conflictingFilesFromErr := conflictErr.ConflictError.ConflictedFiles()
+			conflictingFiles := make([][]byte, 0, len(conflictingFilesFromErr))
+			for _, conflictingFile := range conflictingFilesFromErr {
+				conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
 			}
-
-			return structerr.NewInternal("rebasing commits: %w", err)
-		}
-	} else {
-		repoPath, err := quarantineRepo.Path()
-		if err != nil {
-			return err
-		}
-
-		newrev, err = s.git2goExecutor.Rebase(ctx, quarantineRepo, git2go.RebaseCommand{
-			Repository:       repoPath,
-			Committer:        committer,
-			CommitID:         oldrev,
-			UpstreamCommitID: startRevision,
-			SkipEmptyCommits: true,
-		})
-		if err != nil {
-			var conflictErr git2go.ConflictingFilesError
-			if errors.As(err, &conflictErr) {
-				conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
-				for _, conflictingFile := range conflictErr.ConflictingFiles {
-					conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
-				}
-
-				return structerr.NewFailedPrecondition("rebasing commits: %w", err).WithDetail(
-					&gitalypb.UserRebaseConfirmableError{
-						Error: &gitalypb.UserRebaseConfirmableError_RebaseConflict{
-							RebaseConflict: &gitalypb.MergeConflictError{
-								ConflictingFiles: conflictingFiles,
-								ConflictingCommitIds: []string{
-									startRevision.String(),
-									oldrev.String(),
-								},
+			return structerr.NewFailedPrecondition("rebasing commits: %w", conflictErr).WithDetail(
+				&gitalypb.UserRebaseConfirmableError{
+					Error: &gitalypb.UserRebaseConfirmableError_RebaseConflict{
+						RebaseConflict: &gitalypb.MergeConflictError{
+							ConflictingFiles: conflictingFiles,
+							ConflictingCommitIds: []string{
+								startRevision.String(),
+								oldrev.String(),
 							},
 						},
 					},
-				)
-			}
-
-			return structerr.NewInternal("rebasing commits: %w", err)
+				},
+			)
 		}
+
+		return structerr.NewInternal("rebasing commits: %w", err)
 	}
 
 	if err := stream.Send(&gitalypb.UserRebaseConfirmableResponse{
@@ -174,8 +131,6 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba
 					},
 				},
 			)
-		case errors.Is(err, git2go.ErrInvalidArgument):
-			return fmt.Errorf("update ref: %w", err)
 		}
 
 		return structerr.NewInternal("updating ref with hooks: %w", err)
diff --git a/internal/gitaly/service/operations/rebase_confirmable_test.go b/internal/gitaly/service/operations/rebase_confirmable_test.go
index 7a7831d4c..378cf0f36 100644
--- a/internal/gitaly/service/operations/rebase_confirmable_test.go
+++ b/internal/gitaly/service/operations/rebase_confirmable_test.go
@@ -30,13 +30,10 @@ func TestUserRebaseConfirmable_successful(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmableSuccessful)
 }
 
 func testUserRebaseConfirmableSuccessful(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -94,13 +91,10 @@ func TestUserRebaseConfirmable_skipEmptyCommits(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmableSkipEmptyCommits)
 }
 
 func testUserRebaseConfirmableSkipEmptyCommits(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -210,13 +204,10 @@ func TestUserRebaseConfirmable_transaction(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmableTransaction)
 }
 
 func testUserRebaseConfirmableTransaction(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	txManager := transaction.NewTrackingManager()
@@ -304,13 +295,10 @@ func TestUserRebaseConfirmable_stableCommitIDs(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmableStableCommitIDs)
 }
 
 func testUserRebaseConfirmableStableCommitIDs(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -386,13 +374,10 @@ func TestUserRebaseConfirmable_inputValidation(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmableInputValidation)
 }
 
 func testUserRebaseConfirmableInputValidation(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -454,13 +439,10 @@ func TestUserRebaseConfirmable_abortViaClose(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmableAbortViaClose)
 }
 
 func testUserRebaseConfirmableAbortViaClose(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -539,13 +521,10 @@ func TestUserRebaseConfirmable_abortViaApply(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmableAbortViaApply)
 }
 
 func testUserRebaseConfirmableAbortViaApply(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -585,13 +564,10 @@ func TestUserRebaseConfirmable_preReceiveError(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmablePreReceiveError)
 }
 
 func testUserRebaseConfirmablePreReceiveError(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -652,13 +628,10 @@ func TestUserRebaseConfirmable_mergeConflict(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmableMergeConflict)
 }
 
 func testUserRebaseConfirmableMergeConflict(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -703,13 +676,10 @@ func TestUserRebaseConfirmable_deletedFileInLocalRepo(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmableDeletedFileInLocalRepo)
 }
 
 func testUserRebaseConfirmableDeletedFileInLocalRepo(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -783,13 +753,10 @@ func TestUserRebaseConfirmable_deletedFileInRemoteRepo(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmableDeletedFileInRemoteRepo)
 }
 
 func testUserRebaseConfirmableDeletedFileInRemoteRepo(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -857,13 +824,10 @@ func TestUserRebaseConfirmable_failedWithCode(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseConfirmablePureGit,
 	).Run(t, testUserRebaseConfirmableFailedWithCode)
 }
 
 func testUserRebaseConfirmableFailedWithCode(t *testing.T, ctx context.Context) {
-	skipSHA256WithGit2goRebase(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -996,9 +960,3 @@ func setupRebasableRepositories(tb testing.TB, ctx context.Context, cfg config.C
 		commonCommit: localCommonCommit,
 	}
 }
-
-func skipSHA256WithGit2goRebase(t *testing.T, ctx context.Context) {
-	if gittest.DefaultObjectHash.Format == git.ObjectHashSHA256.Format && featureflag.UserRebaseConfirmablePureGit.IsDisabled(ctx) {
-		t.Skip("SHA256 repositories are only supported when using the pure Git implementation")
-	}
-}
diff --git a/internal/gitaly/service/operations/rebase_to_ref.go b/internal/gitaly/service/operations/rebase_to_ref.go
index 82b931890..3e601b9cf 100644
--- a/internal/gitaly/service/operations/rebase_to_ref.go
+++ b/internal/gitaly/service/operations/rebase_to_ref.go
@@ -6,10 +6,8 @@ import (
 	"fmt"
 	"time"
 
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
 )
@@ -27,11 +25,6 @@ func (s *Server) UserRebaseToRef(ctx context.Context, request *gitalypb.UserReba
 		return nil, structerr.NewInternal("creating repo quarantine: %w", err)
 	}
 
-	repoPath, err := quarantineRepo.Path()
-	if err != nil {
-		return nil, err
-	}
-
 	oid, err := quarantineRepo.ResolveRevision(ctx, git.Revision(request.FirstParentRef))
 	if err != nil {
 		return nil, structerr.NewInvalidArgument("invalid FirstParentRef")
@@ -86,40 +79,20 @@ func (s *Server) UserRebaseToRef(ctx context.Context, request *gitalypb.UserReba
 		committer.When = request.Timestamp.AsTime()
 	}
 
-	var rebasedOID git.ObjectID
-	if featureflag.UserRebaseToRefPureGit.IsEnabled(ctx) {
-		rebasedOID, err = quarantineRepo.Rebase(
-			ctx,
-			oid.String(),
-			sourceOID.String(),
-			localrepo.RebaseWithCommitter(committer),
-		)
-		if err != nil {
-			var conflictErr *localrepo.RebaseConflictError
-			if errors.As(err, &conflictErr) {
-				return nil, structerr.NewFailedPrecondition("failed to rebase %s on %s while preparing %s due to conflict",
-					sourceOID, oid, string(request.TargetRef))
-			}
-
-			return nil, structerr.NewInternal("rebasing commits: %w", err)
+	rebasedOID, err := quarantineRepo.Rebase(
+		ctx,
+		oid.String(),
+		sourceOID.String(),
+		localrepo.RebaseWithCommitter(committer),
+	)
+	if err != nil {
+		var conflictErr *localrepo.RebaseConflictError
+		if errors.As(err, &conflictErr) {
+			return nil, structerr.NewFailedPrecondition("failed to rebase %s on %s while preparing %s due to conflict",
+				sourceOID, oid, string(request.TargetRef))
 		}
-	} else {
-		rebasedOID, err = s.git2goExecutor.Rebase(ctx, quarantineRepo, git2go.RebaseCommand{
-			Repository:       repoPath,
-			Committer:        committer,
-			CommitID:         sourceOID,
-			UpstreamCommitID: oid,
-			SkipEmptyCommits: true,
-		})
-		if err != nil {
-			var conflictErr git2go.ConflictingFilesError
-			if errors.As(err, &conflictErr) {
-				return nil, structerr.NewFailedPrecondition("failed to rebase %s on %s while preparing %s due to conflict",
-					sourceOID, oid, string(request.TargetRef))
-			}
 
-			return nil, structerr.NewInternal("rebasing commits: %w", err)
-		}
+		return nil, structerr.NewInternal("rebasing commits: %w", err)
 	}
 
 	if err := quarantineDir.Migrate(); err != nil {
diff --git a/internal/gitaly/service/operations/rebase_to_ref_test.go b/internal/gitaly/service/operations/rebase_to_ref_test.go
index ed6caa0d0..61ed7acc2 100644
--- a/internal/gitaly/service/operations/rebase_to_ref_test.go
+++ b/internal/gitaly/service/operations/rebase_to_ref_test.go
@@ -22,13 +22,10 @@ func TestUserRebaseToRef_successful(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseToRefPureGit,
 	).Run(t, testUserRebaseToRefSuccessful)
 }
 
 func testUserRebaseToRefSuccessful(t *testing.T, ctx context.Context) {
-	skipSHA256WithUserRebaseToRefGit2go(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -88,13 +85,10 @@ func TestUserRebaseToRef_failure(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseToRefPureGit,
 	).Run(t, testUserRebaseToRefFailure)
 }
 
 func testUserRebaseToRefFailure(t *testing.T, ctx context.Context) {
-	skipSHA256WithUserRebaseToRefGit2go(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -216,13 +210,10 @@ func TestUserRebaseToRef_conflict(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
-		featureflag.UserRebaseToRefPureGit,
 	).Run(t, testUserRebaseToRefConflict)
 }
 
 func testUserRebaseToRefConflict(t *testing.T, ctx context.Context) {
-	skipSHA256WithUserRebaseToRefGit2go(t, ctx)
-
 	t.Parallel()
 
 	ctx, cfg, client := setupOperationsService(t, ctx)
@@ -265,9 +256,3 @@ func testUserRebaseToRefConflict(t *testing.T, ctx context.Context) {
 	currentTargetRefOID := gittest.ResolveRevision(t, cfg, repoPath, targetRef)
 	require.Equal(t, targetRefOID, currentTargetRefOID, "target ref should not change when the rebase fails due to GitError")
 }
-
-func skipSHA256WithUserRebaseToRefGit2go(t *testing.T, ctx context.Context) {
-	if gittest.DefaultObjectHash.Format == git.ObjectHashSHA256.Format && featureflag.UserRebaseToRefPureGit.IsDisabled(ctx) {
-		t.Skip("SHA256 repositories are only supported when using the pure Git implementation")
-	}
-}