Merge branch 'pks-go-user-merge-branch-remove-ff' into 'master'

featureflags: Remove GoUserMergeBranch feature flag

See merge request gitlab-org/gitaly!3049

4 files changed, 23 insertions, 93 deletions

diff --git a/changelogs/unreleased/pks-go-user-merge-branch-remove-ff.yml b/changelogs/unreleased/pks-go-user-merge-branch-remove-ff.yml
new file mode 100644
index 000000000..a09f069d5
--- /dev/null
+++ b/changelogs/unreleased/pks-go-user-merge-branch-remove-ff.yml
@@ -0,0 +1,5 @@
+---
+title: 'featureflags: Remove GoUserMergeBranch feature flag'
+merge_request: 3049
+author:
+type: performance
diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index 318707b54..21fefd5a7 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -19,58 +19,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
-func (s *Server) UserMergeBranch(bidi gitalypb.OperationService_UserMergeBranchServer) error {
-	ctx := bidi.Context()
-
-	if featureflag.IsEnabled(ctx, featureflag.GoUserMergeBranch) {
-		return s.userMergeBranch(bidi)
-	}
-
-	firstRequest, err := bidi.Recv()
-	if err != nil {
-		return err
-	}
-
-	client, err := s.ruby.OperationServiceClient(ctx)
-	if err != nil {
-		return err
-	}
-
-	clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, firstRequest.GetRepository())
-	if err != nil {
-		return err
-	}
-
-	rubyBidi, err := client.UserMergeBranch(clientCtx)
-	if err != nil {
-		return err
-	}
-
-	if err := rubyBidi.Send(firstRequest); err != nil {
-		return err
-	}
-
-	return rubyserver.ProxyBidi(
-		func() error {
-			request, err := bidi.Recv()
-			if err != nil {
-				return err
-			}
-
-			return rubyBidi.Send(request)
-		},
-		rubyBidi,
-		func() error {
-			response, err := rubyBidi.Recv()
-			if err != nil {
-				return err
-			}
-
-			return bidi.Send(response)
-		},
-	)
-}
-
 func validateMergeBranchRequest(request *gitalypb.UserMergeBranchRequest) error {
 	if request.User == nil {
 		return fmt.Errorf("empty user")
@@ -91,7 +39,7 @@ func validateMergeBranchRequest(request *gitalypb.UserMergeBranchRequest) error
 	return nil
 }
 
-func (s *Server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranchServer) error {
+func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranchServer) error {
 	ctx := stream.Context()
 
 	firstRequest, err := stream.Recv()
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 219163200..91ce33c9b 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -16,7 +16,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	gitlog "gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
-	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -40,10 +39,6 @@ func testWithFeature(t *testing.T, feature featureflag.FeatureFlag, testcase fun
 }
 
 func TestSuccessfulMerge(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserMergeBranch, testSuccessfulMerge)
-}
-
-func testSuccessfulMerge(t *testing.T, ctx context.Context) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -55,6 +50,9 @@ func testSuccessfulMerge(t *testing.T, ctx context.Context) {
 	client, conn := newOperationClient(t, serverSocketPath)
 	defer conn.Close()
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	mergeBidi, err := client.UserMergeBranch(ctx)
 	require.NoError(t, err)
 
@@ -131,10 +129,6 @@ func testSuccessfulMerge(t *testing.T, ctx context.Context) {
 }
 
 func TestSuccessfulMerge_stableMergeIDs(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserMergeBranch, testUserMergeBranchStableMergeIDs)
-}
-
-func testUserMergeBranchStableMergeIDs(t *testing.T, ctx context.Context) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -144,6 +138,9 @@ func testUserMergeBranchStableMergeIDs(t *testing.T, ctx context.Context) {
 	client, conn := newOperationClient(t, serverSocketPath)
 	defer conn.Close()
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	mergeBidi, err := client.UserMergeBranch(ctx)
 	require.NoError(t, err)
 
@@ -206,10 +203,6 @@ func testUserMergeBranchStableMergeIDs(t *testing.T, ctx context.Context) {
 }
 
 func TestAbortedMerge(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserMergeBranch, testAbortedMerge)
-}
-
-func testAbortedMerge(t *testing.T, ctx context.Context) {
 	locator := config.NewLocator(config.Config)
 
 	serverSocketPath, stop := runOperationServiceServer(t)
@@ -231,6 +224,9 @@ func testAbortedMerge(t *testing.T, ctx context.Context) {
 		Message:    []byte("foobar"),
 	}
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testCases := []struct {
 		req       *gitalypb.UserMergeBranchRequest
 		closeSend bool
@@ -277,10 +273,6 @@ func testAbortedMerge(t *testing.T, ctx context.Context) {
 }
 
 func TestFailedMergeConcurrentUpdate(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserMergeBranch, testFailedMergeConcurrentUpdate)
-}
-
-func testFailedMergeConcurrentUpdate(t *testing.T, ctx context.Context) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -292,6 +284,9 @@ func testFailedMergeConcurrentUpdate(t *testing.T, ctx context.Context) {
 	client, conn := newOperationClient(t, serverSocketPath)
 	defer conn.Close()
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	mergeBidi, err := client.UserMergeBranch(ctx)
 	require.NoError(t, err)
 
@@ -327,10 +322,6 @@ func testFailedMergeConcurrentUpdate(t *testing.T, ctx context.Context) {
 }
 
 func TestUserMergeBranch_ambiguousReference(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserMergeBranch, testUserMergeBranchAmbiguousReference)
-}
-
-func testUserMergeBranchAmbiguousReference(t *testing.T, ctx context.Context) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -340,6 +331,9 @@ func testUserMergeBranchAmbiguousReference(t *testing.T, ctx context.Context) {
 	client, conn := newOperationClient(t, serverSocketPath)
 	defer conn.Close()
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	merge, err := client.UserMergeBranch(ctx)
 	require.NoError(t, err)
 
@@ -392,28 +386,14 @@ func testUserMergeBranchAmbiguousReference(t *testing.T, ctx context.Context) {
 	commit, err := gitlog.GetCommit(ctx, locator, testRepo, git.Revision("refs/heads/"+mergeBranchName))
 	require.NoError(t, err, "look up git commit after call has finished")
 
-	tag, err := gitlog.GetCommit(ctx, locator, testRepo, git.Revision("refs/tags/"+mergeBranchName))
-	require.NoError(t, err, "look up git tag after call has finished")
-
 	require.Equal(t, gitalypb.OperationBranchUpdate{CommitId: commit.Id}, *(response.BranchUpdate))
 	require.Equal(t, mergeCommitMessage, string(commit.Body))
 	require.Equal(t, testhelper.TestUser.Name, commit.Author.Name)
 	require.Equal(t, testhelper.TestUser.Email, commit.Author.Email)
-
-	if featureflag.IsEnabled(helper.OutgoingToIncoming(ctx), featureflag.GoUserMergeBranch) {
-		require.Equal(t, []string{mergeBranchHeadBefore, commitToMerge}, commit.ParentIds)
-	} else {
-		// The Ruby implementation performs a mismerge with the
-		// ambiguous tag instead of with the branch name.
-		require.Equal(t, []string{tag.Id, commitToMerge}, commit.ParentIds)
-	}
+	require.Equal(t, []string{mergeBranchHeadBefore, commitToMerge}, commit.ParentIds)
 }
 
 func TestFailedMergeDueToHooks(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserMergeBranch, testFailedMergeDueToHooks)
-}
-
-func testFailedMergeDueToHooks(t *testing.T, ctx context.Context) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -432,7 +412,7 @@ func testFailedMergeDueToHooks(t *testing.T, ctx context.Context) {
 			remove := testhelper.WriteCustomHook(t, testRepoPath, hookName, hookContent)
 			defer remove()
 
-			ctx, cancel := context.WithCancel(ctx)
+			ctx, cancel := testhelper.Context()
 			defer cancel()
 
 			mergeBidi, err := client.UserMergeBranch(ctx)
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 6b895c234..3ac1cfa7d 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -17,8 +17,6 @@ var (
 	ReferenceTransactions = FeatureFlag{Name: "reference_transactions", OnByDefault: true}
 	// LogCommandStats will log additional rusage stats for commands
 	LogCommandStats = FeatureFlag{Name: "log_command_stats", OnByDefault: false}
-	// GoUserMergeBranch enables the Go implementation of UserMergeBranch
-	GoUserMergeBranch = FeatureFlag{Name: "go_user_merge_branch", OnByDefault: true}
 	// GoUserFFBranch enables the Go implementation of UserFFBranch
 	GoUserFFBranch = FeatureFlag{Name: "go_user_ff_branch", OnByDefault: false}
 	// GoUserCreateBranch enables the Go implementation of UserCreateBranch
@@ -107,7 +105,6 @@ var All = []FeatureFlag{
 	DistributedReads,
 	LogCommandStats,
 	ReferenceTransactions,
-	GoUserMergeBranch,
 	GoUserFFBranch,
 	GoUserCreateBranch,
 	GoUserUpdateBranch,