operations: Implement UserCherryPick in pure git

Implement the UserCherryPick RPC using pure git commands, using
git-merge-tree(1) more specific.

This change is still behind a feature flag[1].

[1]: https://gitlab.com/gitlab-org/gitaly/-/issues/5421

Issue: https://gitlab.com/gitlab-org/gitaly/-/issues/4574
Changelog: changed
Milestone: 16.2
Label: maintenance::refactor

3 files changed, 229 insertions, 45 deletions

diff --git a/internal/featureflag/ff_cherry_pick_pure_git.go b/internal/featureflag/ff_cherry_pick_pure_git.go
new file mode 100644
index 000000000..311e56c05
--- /dev/null
+++ b/internal/featureflag/ff_cherry_pick_pure_git.go
@@ -0,0 +1,10 @@
+package featureflag
+
+// CherryPickPureGit will enable the UserCherryPick RPC to use
+// git-merge-tree instead of git2go
+var CherryPickPureGit = NewFeatureFlag(
+	"cherry_pick_pure_git",
+	"v16.2.0",
+	"https://gitlab.com/gitlab-org/gitaly/-/issues/5421",
+	false,
+)
diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go
index da79fc6e5..0c8f8f79f 100644
--- a/internal/gitaly/service/operations/cherry_pick.go
+++ b/internal/gitaly/service/operations/cherry_pick.go
@@ -7,7 +7,9 @@ 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/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
@@ -51,48 +53,132 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 		committerDate = req.Timestamp.AsTime()
 	}
 
-	newrev, err := s.git2goExecutor.CherryPick(ctx, quarantineRepo, git2go.CherryPickCommand{
-		Repository:    repoPath,
-		CommitterName: string(req.User.Name),
-		CommitterMail: string(req.User.Email),
-		CommitterDate: committerDate,
-		Message:       string(req.Message),
-		Commit:        req.Commit.Id,
-		Ours:          startRevision.String(),
-		Mainline:      mainline,
-	})
-	if err != nil {
-		var conflictErr git2go.ConflictingFilesError
-		var emptyErr git2go.EmptyError
-
-		switch {
-		case errors.As(err, &conflictErr):
-			conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
-			for _, conflictingFile := range conflictErr.ConflictingFiles {
-				conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
+	var newrev git.ObjectID
+
+	if featureflag.CherryPickPureGit.IsEnabled(ctx) {
+		cherryCommit, err := quarantineRepo.ReadCommit(ctx, git.Revision(req.Commit.Id))
+		if err != nil {
+			if errors.Is(err, localrepo.ErrObjectNotFound) {
+				return nil, structerr.NewNotFound("cherry-pick: commit lookup: commit not found: %q", req.Commit.Id)
 			}
+			return nil, fmt.Errorf("cherry pick: %w", err)
+		}
+		cherryDate := cherryCommit.Author.GetDate().AsTime()
+		loc, err := time.Parse("-0700", string(cherryCommit.Author.GetTimezone()))
+		if err != nil {
+			return nil, fmt.Errorf("get cherry commit location: %w", err)
+		}
+		cherryDate = cherryDate.In(loc.Location())
 
-			return nil, structerr.NewFailedPrecondition("cherry pick: %w", err).WithDetail(
-				&gitalypb.UserCherryPickError{
-					Error: &gitalypb.UserCherryPickError_CherryPickConflict{
-						CherryPickConflict: &gitalypb.MergeConflictError{
-							ConflictingFiles: conflictingFiles,
+		// Cherry-pick is implemented using git-merge-tree(1). We
+		// "merge" in the changes from the commit that is cherry-picked,
+		// compared to it's parent commit (specified as merge base).
+		treeOID, err := quarantineRepo.MergeTree(
+			ctx,
+			startRevision.String(),
+			req.Commit.Id,
+			localrepo.WithMergeBase(git.Revision(req.Commit.Id+"^")),
+			localrepo.WithConflictingFileNamesOnly(),
+		)
+		if err != nil {
+			var conflictErr *localrepo.MergeTreeConflictError
+			if errors.As(err, &conflictErr) {
+				conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFileInfo))
+				for _, conflictingFileInfo := range conflictErr.ConflictingFileInfo {
+					conflictingFiles = append(conflictingFiles, []byte(conflictingFileInfo.FileName))
+				}
+
+				return nil, structerr.NewFailedPrecondition("cherry pick: %w", err).WithDetail(
+					&gitalypb.UserCherryPickError{
+						Error: &gitalypb.UserCherryPickError_CherryPickConflict{
+							CherryPickConflict: &gitalypb.MergeConflictError{
+								ConflictingFiles: conflictingFiles,
+							},
 						},
 					},
-				},
-			)
-		case errors.As(err, &emptyErr):
-			return nil, structerr.NewFailedPrecondition("%w", err).WithDetail(
+				)
+			}
+
+			return nil, fmt.Errorf("cherry-pick command: %w", err)
+		}
+
+		oldTree, err := quarantineRepo.ResolveRevision(
+			ctx,
+			git.Revision(fmt.Sprintf("%s^{tree}", startRevision.String())),
+		)
+		if err != nil {
+			return nil, fmt.Errorf("resolve old tree: %w", err)
+		}
+		if oldTree == treeOID {
+			return nil, structerr.NewFailedPrecondition("cherry-pick: could not apply because the result was empty").WithDetail(
 				&gitalypb.UserCherryPickError{
 					Error: &gitalypb.UserCherryPickError_ChangesAlreadyApplied{},
 				},
 			)
-		case errors.As(err, &git2go.CommitNotFoundError{}):
-			return nil, structerr.NewNotFound("%w", err)
-		case errors.Is(err, git2go.ErrInvalidArgument):
-			return nil, structerr.NewInvalidArgument("%w", err)
-		default:
-			return nil, structerr.NewInternal("cherry-pick command: %w", err)
+		}
+
+		newrev, err = quarantineRepo.WriteCommit(
+			ctx,
+			localrepo.WriteCommitConfig{
+				TreeID:         treeOID,
+				Message:        string(req.Message),
+				Parents:        []git.ObjectID{startRevision},
+				AuthorName:     string(cherryCommit.Author.Name),
+				AuthorEmail:    string(cherryCommit.Author.Email),
+				AuthorDate:     cherryDate,
+				CommitterName:  string(req.User.Name),
+				CommitterEmail: string(req.User.Email),
+				CommitterDate:  committerDate,
+			},
+		)
+		if err != nil {
+			return nil, fmt.Errorf("write commit: %w", err)
+		}
+
+	} else {
+		newrev, err = s.git2goExecutor.CherryPick(ctx, quarantineRepo, git2go.CherryPickCommand{
+			Repository:    repoPath,
+			CommitterName: string(req.User.Name),
+			CommitterMail: string(req.User.Email),
+			CommitterDate: committerDate,
+			Message:       string(req.Message),
+			Commit:        req.Commit.Id,
+			Ours:          startRevision.String(),
+			Mainline:      mainline,
+		})
+		if err != nil {
+			var conflictErr git2go.ConflictingFilesError
+			var emptyErr git2go.EmptyError
+
+			switch {
+			case errors.As(err, &conflictErr):
+				conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
+				for _, conflictingFile := range conflictErr.ConflictingFiles {
+					conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
+				}
+
+				return nil, structerr.NewFailedPrecondition("cherry pick: %w", err).WithDetail(
+					&gitalypb.UserCherryPickError{
+						Error: &gitalypb.UserCherryPickError_CherryPickConflict{
+							CherryPickConflict: &gitalypb.MergeConflictError{
+								ConflictingFiles: conflictingFiles,
+							},
+						},
+					},
+				)
+			case errors.As(err, &emptyErr):
+				return nil, structerr.NewFailedPrecondition("%w", err).WithDetail(
+					&gitalypb.UserCherryPickError{
+						Error: &gitalypb.UserCherryPickError_ChangesAlreadyApplied{},
+					},
+				)
+			case errors.As(err, &git2go.CommitNotFoundError{}):
+				return nil, structerr.NewNotFound("%w", err)
+			case errors.Is(err, git2go.ErrInvalidArgument):
+				return nil, structerr.NewInvalidArgument("%w", err)
+			default:
+				return nil, fmt.Errorf("cherry-pick command: %w", err)
+			}
 		}
 	}
 
diff --git a/internal/gitaly/service/operations/cherry_pick_test.go b/internal/gitaly/service/operations/cherry_pick_test.go
index 267e56b78..5c55a786b 100644
--- a/internal/gitaly/service/operations/cherry_pick_test.go
+++ b/internal/gitaly/service/operations/cherry_pick_test.go
@@ -3,12 +3,14 @@
 package operations
 
 import (
+	"context"
 	"fmt"
 	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"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"
@@ -26,7 +28,16 @@ import (
 func TestUserCherryPick(t *testing.T) {
 	t.Parallel()
 
-	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, testhelper.Context(t))
+	testhelper.NewFeatureSets(
+		featureflag.CherryPickPureGit,
+		featureflag.GPGSigning,
+	).Run(t, testUserCherryPick)
+}
+
+func testUserCherryPick(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
+	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	destinationBranch := "dst-branch"
 
@@ -345,7 +356,15 @@ func TestUserCherryPick(t *testing.T) {
 
 func TestServer_UserCherryPick_successfulGitHooks(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+
+	testhelper.NewFeatureSets(
+		featureflag.CherryPickPureGit,
+		featureflag.GPGSigning,
+	).Run(t, testServerUserCherryPickSuccessfulGitHooks)
+}
+
+func testServerUserCherryPickSuccessfulGitHooks(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
@@ -382,7 +401,15 @@ func TestServer_UserCherryPick_successfulGitHooks(t *testing.T) {
 
 func TestServer_UserCherryPick_stableID(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+
+	testhelper.NewFeatureSets(
+		featureflag.CherryPickPureGit,
+		featureflag.GPGSigning,
+	).Run(t, testServerUserCherryPickStableID)
+}
+
+func testServerUserCherryPickStableID(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
@@ -437,7 +464,15 @@ func TestServer_UserCherryPick_stableID(t *testing.T) {
 
 func TestServer_UserCherryPick_failedValidations(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+
+	testhelper.NewFeatureSets(
+		featureflag.CherryPickPureGit,
+		featureflag.GPGSigning,
+	).Run(t, testServerUserCherryPickFailedValidations)
+}
+
+func testServerUserCherryPickFailedValidations(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
@@ -529,7 +564,16 @@ func TestServer_UserCherryPick_failedValidations(t *testing.T) {
 func TestServer_UserCherryPick_failedWithPreReceiveError(t *testing.T) {
 	t.Parallel()
 
-	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, testhelper.Context(t))
+	testhelper.NewFeatureSets(
+		featureflag.CherryPickPureGit,
+		featureflag.GPGSigning,
+	).Run(t, testServerUserCherryPickFailedWithPreReceiveError)
+}
+
+func testServerUserCherryPickFailedWithPreReceiveError(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -571,7 +615,16 @@ func TestServer_UserCherryPick_failedWithPreReceiveError(t *testing.T) {
 func TestServer_UserCherryPick_failedWithCreateTreeError(t *testing.T) {
 	t.Parallel()
 
-	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, testhelper.Context(t))
+	testhelper.NewFeatureSets(
+		featureflag.CherryPickPureGit,
+		featureflag.GPGSigning,
+	).Run(t, testServerUserCherryPickFailedWithCreateTreeError)
+}
+
+func testServerUserCherryPickFailedWithCreateTreeError(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -602,7 +655,16 @@ func TestServer_UserCherryPick_failedWithCreateTreeError(t *testing.T) {
 func TestServer_UserCherryPick_failedWithCommitError(t *testing.T) {
 	t.Parallel()
 
-	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, testhelper.Context(t))
+	testhelper.NewFeatureSets(
+		featureflag.CherryPickPureGit,
+		featureflag.GPGSigning,
+	).Run(t, testServerUserCherryPickFailedWithCommitError)
+}
+
+func testServerUserCherryPickFailedWithCommitError(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -638,10 +700,19 @@ func TestServer_UserCherryPick_failedWithCommitError(t *testing.T) {
 	assert.Equal(t, []byte("8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab"), targetBranchDivergedErr.TargetBranchDiverged.ParentRevision)
 }
 
-func TestServerUserCherryPickRailedWithConflict(t *testing.T) {
+func TestServer_UserCherryPick_failedWithConflict(t *testing.T) {
+	t.Parallel()
+
+	testhelper.NewFeatureSets(
+		featureflag.CherryPickPureGit,
+		featureflag.GPGSigning,
+	).Run(t, testServerUserCherryPickFailedWithConflict)
+}
+
+func testServerUserCherryPickFailedWithConflict(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, testhelper.Context(t))
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -678,7 +749,15 @@ func TestServerUserCherryPickRailedWithConflict(t *testing.T) {
 
 func TestServer_UserCherryPick_successfulWithGivenCommits(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+
+	testhelper.NewFeatureSets(
+		featureflag.CherryPickPureGit,
+		featureflag.GPGSigning,
+	).Run(t, testServerUserCherryPickSuccessfulWithGivenCommits)
+}
+
+func testServerUserCherryPickSuccessfulWithGivenCommits(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
@@ -734,7 +813,16 @@ func TestServer_UserCherryPick_successfulWithGivenCommits(t *testing.T) {
 func TestServer_UserCherryPick_quarantine(t *testing.T) {
 	t.Parallel()
 
-	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, testhelper.Context(t))
+	testhelper.NewFeatureSets(
+		featureflag.CherryPickPureGit,
+		featureflag.GPGSigning,
+	).Run(t, testServerUserCherryPickQuarantine)
+}
+
+func testServerUserCherryPickQuarantine(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 	// Set up a hook that parses the new object and then aborts the update. Like this, we can