operations: Replace git2go merge in squash with git merge

UserSquash only contains one git2go call, a merge. We already have
a helper function merge() that uses Git plumbing commands. Use a feature
flag to gate switching the merge over to the git plumbing commands to
remove all Git2Go usage in this RPC.

4 files changed, 221 insertions, 46 deletions

diff --git a/internal/featureflag/ff_sqush_in_git.go b/internal/featureflag/ff_sqush_in_git.go
new file mode 100644
index 000000000..9a674c4eb
--- /dev/null
+++ b/internal/featureflag/ff_sqush_in_git.go
@@ -0,0 +1,9 @@
+package featureflag
+
+// SquashInGit enables the pure git implementation of UserSquash
+var SquashInGit = NewFeatureFlag(
+	"squash_in_git",
+	"v16.2.0",
+	"https://gitlab.com/gitlab-org/gitaly/-/issues/5424",
+	false,
+)
diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index b63f3c3c4..28b8161e1 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -58,30 +58,36 @@ func (s *Server) merge(
 	authorName string,
 	authorMail string,
 	authorDate time.Time,
+	committerName string,
+	committerMail string,
+	committerDate time.Time,
 	message string,
 	ours string,
 	theirs string,
+	squash bool,
 ) (string, error) {
 	treeOID, err := quarantineRepo.MergeTree(ctx, ours, theirs, localrepo.WithAllowUnrelatedHistories(), localrepo.WithConflictingFileNamesOnly())
 	if err != nil {
 		return "", err
 	}
 
+	parents := []git.ObjectID{git.ObjectID(ours)}
+	if !squash {
+		parents = append(parents, git.ObjectID(theirs))
+	}
+
 	c, err := quarantineRepo.WriteCommit(
 		ctx,
 		localrepo.WriteCommitConfig{
-			TreeID:  treeOID,
-			Message: message,
-			Parents: []git.ObjectID{
-				git.ObjectID(ours),
-				git.ObjectID(theirs),
-			},
+			TreeID:         treeOID,
+			Message:        message,
+			Parents:        parents,
 			AuthorName:     authorName,
 			AuthorEmail:    authorMail,
 			AuthorDate:     authorDate,
-			CommitterName:  authorName,
-			CommitterEmail: authorMail,
-			CommitterDate:  authorDate,
+			CommitterName:  committerName,
+			CommitterEmail: committerMail,
+			CommitterDate:  committerDate,
 		},
 	)
 	if err != nil {
@@ -144,9 +150,14 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc
 		string(firstRequest.User.Name),
 		string(firstRequest.User.Email),
 		authorDate,
+		string(firstRequest.User.Name),
+		string(firstRequest.User.Email),
+		authorDate,
 		string(firstRequest.Message),
 		revision.String(),
-		firstRequest.CommitId)
+		firstRequest.CommitId,
+		false,
+	)
 	if err != nil {
 		var conflictErr *localrepo.MergeTreeConflictError
 		if errors.As(err, &conflictErr) {
@@ -449,9 +460,13 @@ func (s *Server) UserMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 			string(request.User.Name),
 			string(request.User.Email),
 			authorDate,
+			string(request.User.Name),
+			string(request.User.Email),
+			authorDate,
 			string(request.Message),
 			oid.String(),
 			sourceOID.String(),
+			false,
 		)
 	} else {
 		mergeCommitID, err = s.mergeWithGit2Go(
@@ -461,9 +476,14 @@ func (s *Server) UserMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 			string(request.User.Name),
 			string(request.User.Email),
 			authorDate,
+			string(request.User.Name),
+			string(request.User.Email),
+			authorDate,
+
 			string(request.Message),
 			oid.String(),
 			sourceOID.String(),
+			false,
 		)
 	}
 	if err != nil {
@@ -505,18 +525,26 @@ func (s *Server) mergeWithGit2Go(
 	authorName string,
 	authorEmail string,
 	authorDate time.Time,
+	committerName string,
+	committerEmail string,
+	committerDate time.Time,
 	message string,
 	ours string,
 	theirs string,
+	squash bool,
 ) (string, error) {
 	merge, err := s.git2goExecutor.Merge(ctx, repo, git2go.MergeCommand{
-		Repository: repoPath,
-		AuthorName: authorName,
-		AuthorMail: authorEmail,
-		AuthorDate: authorDate,
-		Message:    message,
-		Ours:       ours,
-		Theirs:     theirs,
+		Repository:    repoPath,
+		AuthorName:    authorName,
+		AuthorMail:    authorEmail,
+		AuthorDate:    authorDate,
+		CommitterName: committerName,
+		CommitterMail: committerEmail,
+		CommitterDate: committerDate,
+		Message:       message,
+		Ours:          ours,
+		Theirs:        theirs,
+		Squash:        squash,
 	})
 	if err != nil {
 		return "", err
diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go
index 9c2c23709..2725c9188 100644
--- a/internal/gitaly/service/operations/squash.go
+++ b/internal/gitaly/service/operations/squash.go
@@ -5,7 +5,9 @@ import (
 	"errors"
 	"strings"
 
+	"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/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction"
@@ -133,20 +135,39 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 		message += "\n"
 	}
 
-	// Perform squash merge onto endCommit using git2go merge.
-	merge, err := s.git2goExecutor.Merge(ctx, quarantineRepo, git2go.MergeCommand{
-		Repository:    quarantineRepoPath,
-		AuthorName:    string(req.GetAuthor().GetName()),
-		AuthorMail:    string(req.GetAuthor().GetEmail()),
-		AuthorDate:    commitDate,
-		CommitterName: string(req.GetUser().Name),
-		CommitterMail: string(req.GetUser().Email),
-		CommitterDate: commitDate,
-		Message:       message,
-		Ours:          startCommit.String(),
-		Theirs:        endCommit.String(),
-		Squash:        true,
-	})
+	var commitID string
+	if featureflag.SquashInGit.IsEnabled(ctx) {
+		commitID, err = s.merge(
+			ctx,
+			quarantineRepo,
+			string(req.GetAuthor().GetName()),
+			string(req.GetAuthor().GetEmail()),
+			commitDate,
+			string(req.GetUser().GetName()),
+			string(req.GetUser().GetEmail()),
+			commitDate,
+			message,
+			startCommit.String(),
+			endCommit.String(),
+			true,
+		)
+	} else {
+		commitID, err = s.mergeWithGit2Go(
+			ctx,
+			quarantineRepo,
+			quarantineRepoPath,
+			string(req.GetAuthor().GetName()),
+			string(req.GetAuthor().GetEmail()),
+			commitDate,
+			string(req.GetUser().GetName()),
+			string(req.GetUser().GetEmail()),
+			commitDate,
+			message,
+			startCommit.String(),
+			endCommit.String(),
+			true,
+		)
+	}
 	if err != nil {
 		var conflictErr git2go.ConflictingFilesError
 
@@ -172,9 +193,31 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 				},
 			)
 		}
-	}
 
-	commitID := merge.CommitID
+		var mergeConflictErr *localrepo.MergeTreeConflictError
+		if errors.As(err, &mergeConflictErr) {
+			conflictingFiles := make([][]byte, 0, len(mergeConflictErr.ConflictingFileInfo))
+			for _, conflictingFile := range mergeConflictErr.ConflictingFileInfo {
+				conflictingFiles = append(conflictingFiles, []byte(conflictingFile.FileName))
+			}
+
+			return "", structerr.NewFailedPrecondition("squashing commits: %w", err).WithDetail(
+				&gitalypb.UserSquashError{
+					// Note: this is actually a merge conflict, but we've kept
+					// the old "rebase" name for compatibility reasons.
+					Error: &gitalypb.UserSquashError_RebaseConflict{
+						RebaseConflict: &gitalypb.MergeConflictError{
+							ConflictingFiles: conflictingFiles,
+							ConflictingCommitIds: []string{
+								startCommit.String(),
+								endCommit.String(),
+							},
+						},
+					},
+				},
+			)
+		}
+	}
 
 	// The RPC is badly designed in that it never updates any references, but only creates the
 	// objects and writes them to disk. We still use a quarantine directory to stage the new
diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go
index d401f8868..ff5c06b8a 100644
--- a/internal/gitaly/service/operations/squash_test.go
+++ b/internal/gitaly/service/operations/squash_test.go
@@ -10,6 +10,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
@@ -44,7 +45,14 @@ var (
 func TestUserSquash_successful(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashSuccessful)
+}
+
+func testUserSquashSuccessful(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	for _, tc := range []struct {
 		desc             string
@@ -96,10 +104,17 @@ func TestUserSquash_successful(t *testing.T) {
 	}
 }
 
-func TestUserSquash_transactional(t *testing.T) {
+func TestUserSquash_transaction(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashTransactional)
+}
+
+func testUserSquashTransactional(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	txManager := transaction.MockManager{}
 
@@ -213,7 +228,15 @@ func TestUserSquash_transactional(t *testing.T) {
 func TestUserSquash_stableID(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashStableID)
+}
+
+func testUserSquashStableID(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -270,7 +293,15 @@ func ensureSplitIndexExists(t *testing.T, cfg config.Cfg, repoDir string) bool {
 func TestUserSquash_threeWayMerge(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashThreeWayMerge)
+}
+
+func testUserSquashThreeWayMerge(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -303,7 +334,15 @@ func TestUserSquash_threeWayMerge(t *testing.T) {
 func TestUserSquash_splitIndex(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashSplitIndex)
+}
+
+func testUserSquashSplitIndex(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	require.False(t, ensureSplitIndexExists(t, cfg, repoPath))
@@ -325,7 +364,15 @@ func TestUserSquash_splitIndex(t *testing.T) {
 func TestUserSquash_renames(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashRenames)
+}
+
+func testUserSquashRenames(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
@@ -395,7 +442,15 @@ func TestUserSquash_renames(t *testing.T) {
 func TestUserSquash_missingFileOnTargetBranch(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashMissingFileOnTargetBranch)
+}
+
+func testUserSquashMissingFileOnTargetBranch(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	conflictingStartSha := "bbd36ad238d14e1c03ece0f3358f545092dc9ca3"
@@ -416,7 +471,15 @@ func TestUserSquash_missingFileOnTargetBranch(t *testing.T) {
 func TestUserSquash_emptyCommit(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashEmptyCommit)
+}
+
+func testUserSquashEmptyCommit(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -614,7 +677,15 @@ func TestUserSquash_validation(t *testing.T) {
 func TestUserSquash_conflicts(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashConflicts)
+}
+
+func testUserSquashConflicts(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries(
@@ -662,7 +733,15 @@ func TestUserSquash_conflicts(t *testing.T) {
 func TestUserSquash_ancestry(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashAncestry)
+}
+
+func testUserSquashAncestry(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	// We create an empty parent commit and two commits which both branch off from it. As a
@@ -696,7 +775,15 @@ func TestUserSquash_ancestry(t *testing.T) {
 func TestUserSquash_gitError(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashGitError)
+}
+
+func testUserSquashGitError(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	testCases := []struct {
@@ -797,7 +884,15 @@ func TestUserSquash_gitError(t *testing.T) {
 func TestUserSquash_squashingMerge(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(
+		featureflag.SquashInGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserSquashSquashingMerge)
+}
+
+func testUserSquashSquashingMerge(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("base"),