Merge branch 'zj-ff-removal-ff-branch' into 'master'

featureflag: Remove UserFFBranch feature gate

See merge request gitlab-org/gitaly!3180

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,