operations: Always use Git-based implementation of UserCherryPick

In order to advance our deprecation of libgit2, we have implemented the
UserCherryPick RPC with plain Git commands. This feature flag was rolled
out to production systems two weeks ago without any observed fallout. We
have thus removed the flag once in the past via a0246dcb9 (operations:
Remove CherryPickInPureGit feature flag, 2023-08-08).

Unfortunately, we had to roll back that change because we have observed
failures in QA. As it turns out, this failure was caused by a real, but
unrelated issue: Rails didn't pass a `timestamp` to both UserCherryPick
and UserRevert, which means that the committer time was computed on the
Gitaly host. While this works in standalone Gitaly setups, it results in
a race in Gitaly Cluster where each of the backing Gitaly nodes may
arrive at a different committer time. The consequence is that in many
cases, the transactional vote will fail and thus we abort the change.

This issue exists regardless of whether we use Git2go or Git, and it has
been fixed in Rails. So let's try a second time to remove the feature
flag.

Changelog: removed

3 files changed, 68 insertions, 181 deletions

diff --git a/internal/featureflag/ff_cherry_pick_pure_git.go b/internal/featureflag/ff_cherry_pick_pure_git.go
deleted file mode 100644
index 311e56c05..000000000
--- a/internal/featureflag/ff_cherry_pick_pure_git.go
+++ /dev/null
@@ -1,10 +0,0 @@
-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 d939827ec..beca52822 100644
--- a/internal/gitaly/service/operations/cherry_pick.go
+++ b/internal/gitaly/service/operations/cherry_pick.go
@@ -7,10 +7,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/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
@@ -38,148 +36,89 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 		return nil, structerr.NewInternal("has branches: %w", err)
 	}
 
-	repoPath, err := quarantineRepo.Path()
-	if err != nil {
-		return nil, err
-	}
-
-	var mainline uint
-	if len(req.Commit.ParentIds) > 1 {
-		mainline = 1
-	}
-
 	committerDate := time.Now()
 	if req.Timestamp != nil {
 		committerDate = req.Timestamp.AsTime()
 	}
 
-	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)
+	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)
 		}
-		cherryDate = cherryDate.In(loc.Location())
-
-		// 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,
-							},
-						},
-					},
-				)
+		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())
+
+	// 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, 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(
+			return nil, structerr.NewFailedPrecondition("cherry pick: %w", err).WithDetail(
 				&gitalypb.UserCherryPickError{
-					Error: &gitalypb.UserCherryPickError_ChangesAlreadyApplied{},
+					Error: &gitalypb.UserCherryPickError_CherryPickConflict{
+						CherryPickConflict: &gitalypb.MergeConflictError{
+							ConflictingFiles: conflictingFiles,
+						},
+					},
 				},
 			)
 		}
 
-		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,
-				SigningKey:     s.signingKey,
+		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{},
 			},
 		)
-		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)
-			}
-		}
+	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,
+			SigningKey:     s.signingKey,
+		},
+	)
+	if err != nil {
+		return nil, fmt.Errorf("write commit: %w", err)
 	}
 
 	referenceName := git.NewReferenceNameFromBranchName(string(req.BranchName))
diff --git a/internal/gitaly/service/operations/cherry_pick_test.go b/internal/gitaly/service/operations/cherry_pick_test.go
index 5d445eb7b..f962cbe87 100644
--- a/internal/gitaly/service/operations/cherry_pick_test.go
+++ b/internal/gitaly/service/operations/cherry_pick_test.go
@@ -30,7 +30,6 @@ func TestUserCherryPick(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testUserCherryPick)
 }
@@ -38,8 +37,6 @@ func TestUserCherryPick(t *testing.T) {
 func testUserCherryPick(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	ctx, cfg, client := setupOperationsService(t, ctx)
 
 	destinationBranch := "dst-branch"
@@ -363,7 +360,6 @@ func TestServer_UserCherryPick_successfulGitHooks(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testServerUserCherryPickSuccessfulGitHooks)
 }
@@ -371,8 +367,6 @@ func TestServer_UserCherryPick_successfulGitHooks(t *testing.T) {
 func testServerUserCherryPickSuccessfulGitHooks(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	ctx, cfg, client := setupOperationsService(t, ctx)
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -422,7 +416,6 @@ func TestServer_UserCherryPick_mergeCommit(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testServerUserCherryPickMergeCommit)
 }
@@ -441,8 +434,6 @@ func testServerUserCherryPickMergeCommit(t *testing.T, ctx context.Context) {
 		testcfg.BuildGitalyGPG(t, cfg)
 	}
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -518,7 +509,7 @@ func testServerUserCherryPickMergeCommit(t *testing.T, ctx context.Context) {
 			{Mode: "100644", Path: "z", Content: "zucchini"},
 		})
 
-	if featureflag.GPGSigning.IsEnabled(ctx) && featureflag.CherryPickPureGit.IsEnabled(ctx) {
+	if featureflag.GPGSigning.IsEnabled(ctx) {
 		data, err := repo.ReadObject(ctx, git.ObjectID(response.BranchUpdate.CommitId))
 		require.NoError(t, err)
 
@@ -535,7 +526,6 @@ func TestServer_UserCherryPick_stableID(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testServerUserCherryPickStableID)
 }
@@ -543,8 +533,6 @@ func TestServer_UserCherryPick_stableID(t *testing.T) {
 func testServerUserCherryPickStableID(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	ctx, cfg, client := setupOperationsService(t, ctx)
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -626,7 +614,6 @@ func TestServer_UserCherryPick_failedValidations(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testServerUserCherryPickFailedValidations)
 }
@@ -634,8 +621,6 @@ func TestServer_UserCherryPick_failedValidations(t *testing.T) {
 func testServerUserCherryPickFailedValidations(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	ctx, cfg, client := setupOperationsService(t, ctx)
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -739,7 +724,6 @@ func TestServer_UserCherryPick_failedWithPreReceiveError(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testServerUserCherryPickFailedWithPreReceiveError)
 }
@@ -747,8 +731,6 @@ func TestServer_UserCherryPick_failedWithPreReceiveError(t *testing.T) {
 func testServerUserCherryPickFailedWithPreReceiveError(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	ctx, cfg, client := setupOperationsService(t, ctx)
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -804,7 +786,6 @@ func TestServer_UserCherryPick_failedWithCreateTreeError(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testServerUserCherryPickFailedWithCreateTreeError)
 }
@@ -812,8 +793,6 @@ func TestServer_UserCherryPick_failedWithCreateTreeError(t *testing.T) {
 func testServerUserCherryPickFailedWithCreateTreeError(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	ctx, cfg, client := setupOperationsService(t, ctx)
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -858,7 +837,6 @@ func TestServer_UserCherryPick_failedWithCommitError(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testServerUserCherryPickFailedWithCommitError)
 }
@@ -866,8 +844,6 @@ func TestServer_UserCherryPick_failedWithCommitError(t *testing.T) {
 func testServerUserCherryPickFailedWithCommitError(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	ctx, cfg, client := setupOperationsService(t, ctx)
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -921,7 +897,6 @@ func TestServer_UserCherryPick_failedWithConflict(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testServerUserCherryPickFailedWithConflict)
 }
@@ -929,8 +904,6 @@ func TestServer_UserCherryPick_failedWithConflict(t *testing.T) {
 func testServerUserCherryPickFailedWithConflict(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	ctx, cfg, client := setupOperationsService(t, ctx)
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -988,7 +961,6 @@ func TestServer_UserCherryPick_successfulWithGivenCommits(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testServerUserCherryPickSuccessfulWithGivenCommits)
 }
@@ -996,8 +968,6 @@ func TestServer_UserCherryPick_successfulWithGivenCommits(t *testing.T) {
 func testServerUserCherryPickSuccessfulWithGivenCommits(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	ctx, cfg, client := setupOperationsService(t, ctx)
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -1066,7 +1036,6 @@ func TestServer_UserCherryPick_quarantine(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testServerUserCherryPickQuarantine)
 }
@@ -1074,8 +1043,6 @@ func TestServer_UserCherryPick_quarantine(t *testing.T) {
 func testServerUserCherryPickQuarantine(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	ctx, cfg, client := setupOperationsService(t, ctx)
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -1135,7 +1102,6 @@ func TestServer_UserCherryPick_reverse(t *testing.T) {
 	t.Parallel()
 
 	testhelper.NewFeatureSets(
-		featureflag.CherryPickPureGit,
 		featureflag.GPGSigning,
 	).Run(t, testServerUserCherryPickReverse)
 }
@@ -1143,8 +1109,6 @@ func TestServer_UserCherryPick_reverse(t *testing.T) {
 func testServerUserCherryPickReverse(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
-	skipSHA256WithGit2goCherryPick(t, ctx)
-
 	ctx, cfg, client := setupOperationsService(t, ctx)
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -1212,9 +1176,3 @@ func testServerUserCherryPickReverse(t *testing.T, ctx context.Context) {
 		)
 	}
 }
-
-func skipSHA256WithGit2goCherryPick(t *testing.T, ctx context.Context) {
-	if gittest.DefaultObjectHash.Format == git.ObjectHashSHA256.Format && featureflag.CherryPickPureGit.IsDisabled(ctx) {
-		t.Skip("SHA256 repositories are only supported when using the pure Git implementation")
-	}
-}