featureflag: Remove UserFFBranch feature gate

Through b3a047a56 (Merge branch 'go_user_ff_branch_by_default' into 'master', 2021-01-27)
the UserFFBranch flag defaults to true. Which means that the feature
gate is up for removal if the changes are stable, which they are.

Thus this change removes the feature gate, and makes sure the tests
succeed. In the next release the Ruby implementation can be removed.

4 files changed, 109 insertions, 140 deletions

diff --git a/changelogs/unreleased/zj-ff-removal-ff-branch.yml b/changelogs/unreleased/zj-ff-removal-ff-branch.yml
new file mode 100644
index 000000000..8fd68558d
--- /dev/null
+++ b/changelogs/unreleased/zj-ff-removal-ff-branch.yml
@@ -0,0 +1,5 @@
+---
+title: 'featureflag: Remove UserFFBranch feature gate'
+merge_request: 3180
+author:
+type: changed
diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index 48b2dc727..74edf8229 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -13,10 +13,8 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/internal/git2go"
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
 	"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/proto/go/gitalypb"
 )
 
@@ -154,10 +152,6 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ
 		return nil, helper.ErrInvalidArgument(err)
 	}
 
-	if featureflag.IsDisabled(ctx, featureflag.GoUserFFBranch) {
-		return s.userFFBranchRuby(ctx, in)
-	}
-
 	branch := fmt.Sprintf("refs/heads/%s", in.Branch)
 	revision, err := localrepo.New(s.gitCmdFactory, in.Repository, s.cfg).ResolveRevision(ctx, git.Revision(branch))
 	if err != nil {
@@ -198,20 +192,6 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ
 	}, nil
 }
 
-func (s *Server) userFFBranchRuby(ctx context.Context, in *gitalypb.UserFFBranchRequest) (*gitalypb.UserFFBranchResponse, error) {
-	client, err := s.ruby.OperationServiceClient(ctx)
-	if err != nil {
-		return nil, err
-	}
-
-	clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, in.GetRepository())
-	if err != nil {
-		return nil, err
-	}
-
-	return client.UserFFBranch(clientCtx, in)
-}
-
 func validateUserMergeToRefRequest(in *gitalypb.UserMergeToRefRequest) error {
 	if len(in.FirstParentRef) == 0 && len(in.Branch) == 0 {
 		return fmt.Errorf("empty first parent ref and branch name")
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 2b0d9c1a1..342665ded 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -446,42 +446,41 @@ func TestFailedMergeDueToHooks(t *testing.T) {
 }
 
 func TestSuccessfulUserFFBranchRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoUserFFBranch,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		serverSocketPath, stop := runOperationServiceServer(t)
-		defer stop()
-
-		client, conn := newOperationClient(t, serverSocketPath)
-		defer conn.Close()
-
-		testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
-		defer cleanupFn()
-
-		commitID := "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
-		branchName := "test-ff-target-branch"
-		request := &gitalypb.UserFFBranchRequest{
-			Repository: testRepo,
-			CommitId:   commitID,
-			Branch:     []byte(branchName),
-			User:       testhelper.TestUser,
-		}
-		expectedResponse := &gitalypb.UserFFBranchResponse{
-			BranchUpdate: &gitalypb.OperationBranchUpdate{
-				RepoCreated:   false,
-				BranchCreated: false,
-				CommitId:      commitID,
-			},
-		}
+	serverSocketPath, stop := runOperationServiceServer(t)
+	defer stop()
 
-		testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
+	client, conn := newOperationClient(t, serverSocketPath)
+	defer conn.Close()
 
-		resp, err := client.UserFFBranch(ctx, request)
-		require.NoError(t, err)
-		testhelper.ProtoEqual(t, expectedResponse, resp)
-		newBranchHead := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", branchName)
-		require.Equal(t, commitID, text.ChompBytes(newBranchHead), "branch head not updated")
-	})
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	commitID := "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
+	branchName := "test-ff-target-branch"
+	request := &gitalypb.UserFFBranchRequest{
+		Repository: testRepo,
+		CommitId:   commitID,
+		Branch:     []byte(branchName),
+		User:       testhelper.TestUser,
+	}
+	expectedResponse := &gitalypb.UserFFBranchResponse{
+		BranchUpdate: &gitalypb.OperationBranchUpdate{
+			RepoCreated:   false,
+			BranchCreated: false,
+			CommitId:      commitID,
+		},
+	}
+
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
+
+	resp, err := client.UserFFBranch(ctx, request)
+	require.NoError(t, err)
+	testhelper.ProtoEqual(t, expectedResponse, resp)
+	newBranchHead := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", branchName)
+	require.Equal(t, commitID, text.ChompBytes(newBranchHead), "branch head not updated")
 }
 
 func TestFailedUserFFBranchRequest(t *testing.T) {
@@ -561,22 +560,21 @@ func TestFailedUserFFBranchRequest(t *testing.T) {
 		},
 	}
 
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoUserFFBranch,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		for _, testCase := range testCases {
-			t.Run(testCase.desc, func(t *testing.T) {
-				request := &gitalypb.UserFFBranchRequest{
-					Repository: testCase.repo,
-					User:       testCase.user,
-					Branch:     testCase.branch,
-					CommitId:   testCase.commitID,
-				}
-				_, err := client.UserFFBranch(ctx, request)
-				testhelper.RequireGrpcError(t, err, testCase.code)
-			})
-		}
-	})
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	for _, testCase := range testCases {
+		t.Run(testCase.desc, func(t *testing.T) {
+			request := &gitalypb.UserFFBranchRequest{
+				Repository: testCase.repo,
+				User:       testCase.user,
+				Branch:     testCase.branch,
+				CommitId:   testCase.commitID,
+			}
+			_, err := client.UserFFBranch(ctx, request)
+			testhelper.RequireGrpcError(t, err, testCase.code)
+		})
+	}
 }
 
 func TestFailedUserFFBranchDueToHooks(t *testing.T) {
@@ -602,81 +600,70 @@ func TestFailedUserFFBranchDueToHooks(t *testing.T) {
 
 	hookContent := []byte("#!/bin/sh\necho 'failure'\nexit 1")
 
-	featureSets := testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoUserFFBranch,
-	})
-
-	for _, featureSet := range featureSets {
-		t.Run(featureSet.Desc(), func(t *testing.T) {
-			for _, hookName := range gitlabPreHooks {
-				t.Run(hookName, func(t *testing.T) {
-					remove := testhelper.WriteCustomHook(t, testRepoPath, hookName, hookContent)
-					defer remove()
-
-					ctx, cancel := testhelper.Context()
-					defer cancel()
+	for _, hookName := range gitlabPreHooks {
+		t.Run(hookName, func(t *testing.T) {
+			remove := testhelper.WriteCustomHook(t, testRepoPath, hookName, hookContent)
+			defer remove()
 
-					ctx = featureSet.Disable(ctx)
+			ctx, cancel := testhelper.Context()
+			defer cancel()
 
-					resp, err := client.UserFFBranch(ctx, request)
-					require.Nil(t, err)
-					require.Contains(t, resp.PreReceiveError, "failure")
-				})
-			}
+			resp, err := client.UserFFBranch(ctx, request)
+			require.Nil(t, err)
+			require.Contains(t, resp.PreReceiveError, "failure")
 		})
 	}
 }
 
 func TestUserFFBranch_ambiguousReference(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoUserFFBranch,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		serverSocketPath, stop := runOperationServiceServer(t)
-		defer stop()
-
-		client, conn := newOperationClient(t, serverSocketPath)
-		defer func() { require.NoError(t, conn.Close()) }()
-
-		testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
-		defer cleanupFn()
-
-		branchName := "test-ff-target-branch"
-
-		// We're creating both a branch and a tag with the same name.
-		// If `git rev-parse` is called on the branch name directly
-		// without using the fully qualified reference, then it would
-		// return the OID of the tag instead of the branch.
-		//
-		// In the past, this used to cause us to use the tag's OID as
-		// old revision when calling git-update-ref. As a result, the
-		// update would've failed as the branch's current revision
-		// didn't match the specified old revision.
-		testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath,
-			"branch", branchName,
-			"6d394385cf567f80a8fd85055db1ab4c5295806f")
-		testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f~")
-
-		commitID := "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
-		request := &gitalypb.UserFFBranchRequest{
-			Repository: testRepo,
-			CommitId:   commitID,
-			Branch:     []byte(branchName),
-			User:       testhelper.TestUser,
-		}
-		expectedResponse := &gitalypb.UserFFBranchResponse{
-			BranchUpdate: &gitalypb.OperationBranchUpdate{
-				RepoCreated:   false,
-				BranchCreated: false,
-				CommitId:      commitID,
-			},
-		}
+	serverSocketPath, stop := runOperationServiceServer(t)
+	defer stop()
 
-		resp, err := client.UserFFBranch(ctx, request)
-		require.NoError(t, err)
-		testhelper.ProtoEqual(t, expectedResponse, resp)
-		newBranchHead := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", "refs/heads/"+branchName)
-		require.Equal(t, commitID, text.ChompBytes(newBranchHead), "branch head not updated")
-	})
+	client, conn := newOperationClient(t, serverSocketPath)
+	defer func() { require.NoError(t, conn.Close()) }()
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	branchName := "test-ff-target-branch"
+
+	// We're creating both a branch and a tag with the same name.
+	// If `git rev-parse` is called on the branch name directly
+	// without using the fully qualified reference, then it would
+	// return the OID of the tag instead of the branch.
+	//
+	// In the past, this used to cause us to use the tag's OID as
+	// old revision when calling git-update-ref. As a result, the
+	// update would've failed as the branch's current revision
+	// didn't match the specified old revision.
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath,
+		"branch", branchName,
+		"6d394385cf567f80a8fd85055db1ab4c5295806f")
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f~")
+
+	commitID := "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
+	request := &gitalypb.UserFFBranchRequest{
+		Repository: testRepo,
+		CommitId:   commitID,
+		Branch:     []byte(branchName),
+		User:       testhelper.TestUser,
+	}
+	expectedResponse := &gitalypb.UserFFBranchResponse{
+		BranchUpdate: &gitalypb.OperationBranchUpdate{
+			RepoCreated:   false,
+			BranchCreated: false,
+			CommitId:      commitID,
+		},
+	}
+
+	resp, err := client.UserFFBranch(ctx, request)
+	require.NoError(t, err)
+	testhelper.ProtoEqual(t, expectedResponse, resp)
+	newBranchHead := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", "refs/heads/"+branchName)
+	require.Equal(t, commitID, text.ChompBytes(newBranchHead), "branch head not updated")
 }
 
 func TestSuccessfulUserMergeToRefRequest(t *testing.T) {
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 096d0ddf3..ccf392078 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -15,8 +15,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}
-	// GoUserFFBranch enables the Go implementation of UserFFBranch
-	GoUserFFBranch = FeatureFlag{Name: "go_user_ff_branch", OnByDefault: true}
 	// GoUserCherryPick enables the Go implementation of UserCherryPick
 	GoUserCherryPick = FeatureFlag{Name: "go_user_cherry_pick", OnByDefault: false}
 	// GoUserUpdateBranch enables the Go implementation of UserUpdateBranch
@@ -104,7 +102,6 @@ var All = []FeatureFlag{
 	DistributedReads,
 	LogCommandStats,
 	ReferenceTransactions,
-	GoUserFFBranch,
 	GoUserCherryPick,
 	GoUserUpdateBranch,
 	GoUserCommitFiles,