operations: Remove feature flag for transactional voting in UserSquash

With af4ea3258 (operations: Fix missing votes on squashed commits,
2022-03-18) we have introduced the use of quarantine directories in the
`UserSquash()` RPC to enable transactional voting. This change in
behaviour has been rolled out to production, and the change was enabled
by default in e8413304d (operations: Always use structured errors for
UserSquash, 2022-03-21).

We haven't seen any issues with this change. Remove the feature flag to
always enable it.

Changelog: removed

3 files changed, 54 insertions, 133 deletions

diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go
index 1129d2658..9b3adaba9 100644
--- a/internal/gitaly/service/operations/squash.go
+++ b/internal/gitaly/service/operations/squash.go
@@ -7,13 +7,10 @@ import (
 	"fmt"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/git/quarantine"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
@@ -82,32 +79,21 @@ func validateUserSquashRequest(req *gitalypb.UserSquashRequest) error {
 }
 
 func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest) (string, error) {
-	repo := s.localrepo(req.GetRepository())
-	repoPath, err := repo.Path()
-	if err != nil {
-		return "", helper.ErrInternalf("cannot resolve repo path: %w", err)
-	}
-
 	// All new objects are staged into a quarantine directory first so that we can do
 	// transactional voting before we commit data to disk.
-	var quarantineDir *quarantine.Dir
-	if featureflag.UserSquashQuarantinedVoting.IsEnabled(ctx) {
-		var quarantineRepo *localrepo.Repo
-		quarantineDir, quarantineRepo, err = s.quarantinedRepo(ctx, req.GetRepository())
-		if err != nil {
-			return "", helper.ErrInternalf("creating quarantine: %w", err)
-		}
+	quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository())
+	if err != nil {
+		return "", helper.ErrInternalf("creating quarantine: %w", err)
+	}
 
-		repo = quarantineRepo
-		repoPath, err = quarantineRepo.Path()
-		if err != nil {
-			return "", helper.ErrInternalf("getting quarantine path: %w", err)
-		}
+	quarantineRepoPath, err := quarantineRepo.Path()
+	if err != nil {
+		return "", helper.ErrInternalf("getting quarantine path: %w", err)
 	}
 
 	// We need to retrieve the start commit such that we can create the new commit with
 	// all parents of the start commit.
-	startCommit, err := repo.ResolveRevision(ctx, git.Revision(req.GetStartSha()+"^{commit}"))
+	startCommit, err := quarantineRepo.ResolveRevision(ctx, git.Revision(req.GetStartSha()+"^{commit}"))
 	if err != nil {
 		detailedErr, err := helper.ErrWithDetails(
 			helper.ErrInvalidArgumentf("resolving start revision: %w", err),
@@ -127,7 +113,7 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 	}
 
 	// And we need to take the tree of the end commit. This tree already is the result
-	endCommit, err := repo.ResolveRevision(ctx, git.Revision(req.GetEndSha()+"^{commit}"))
+	endCommit, err := quarantineRepo.ResolveRevision(ctx, git.Revision(req.GetEndSha()+"^{commit}"))
 	if err != nil {
 		detailedErr, err := helper.ErrWithDetails(
 			helper.ErrInvalidArgumentf("resolving end revision: %w", err),
@@ -153,8 +139,8 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 
 	// We're now rebasing the end commit on top of the start commit. The resulting tree
 	// is then going to be the tree of the squashed commit.
-	rebasedCommitID, err := s.git2goExecutor.Rebase(ctx, repo, git2go.RebaseCommand{
-		Repository: repoPath,
+	rebasedCommitID, err := s.git2goExecutor.Rebase(ctx, quarantineRepo, git2go.RebaseCommand{
+		Repository: quarantineRepoPath,
 		Committer: git2go.NewSignature(
 			string(req.GetUser().Name), string(req.GetUser().Email), commitDate,
 		),
@@ -191,7 +177,7 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 		return "", helper.ErrInternalf("rebasing commits: %w", err)
 	}
 
-	treeID, err := repo.ResolveRevision(ctx, rebasedCommitID.Revision()+"^{tree}")
+	treeID, err := quarantineRepo.ResolveRevision(ctx, rebasedCommitID.Revision()+"^{tree}")
 	if err != nil {
 		return "", fmt.Errorf("cannot resolve rebased tree: %w", err)
 	}
@@ -217,7 +203,7 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 	}
 
 	var stdout, stderr bytes.Buffer
-	if err := repo.ExecAndWait(ctx, git.SubCmd{
+	if err := quarantineRepo.ExecAndWait(ctx, git.SubCmd{
 		Name:  "commit-tree",
 		Flags: flags,
 		Args: []string{
@@ -234,28 +220,26 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 	// objects, vote on them and migrate them into the main directory if quorum was reached so
 	// that we don't pollute the object directory with objects we don't want to have in the
 	// first place.
-	if featureflag.UserSquashQuarantinedVoting.IsEnabled(ctx) {
-		if err := transaction.VoteOnContext(
-			ctx,
-			s.txManager,
-			voting.VoteFromData([]byte(commitID)),
-			voting.Prepared,
-		); err != nil {
-			return "", helper.ErrAbortedf("preparatory vote on squashed commit: %w", err)
-		}
-
-		if err := quarantineDir.Migrate(); err != nil {
-			return "", helper.ErrInternalf("migrating quarantine directory: %w", err)
-		}
-
-		if err := transaction.VoteOnContext(
-			ctx,
-			s.txManager,
-			voting.VoteFromData([]byte(commitID)),
-			voting.Committed,
-		); err != nil {
-			return "", helper.ErrAbortedf("committing vote on squashed commit: %w", err)
-		}
+	if err := transaction.VoteOnContext(
+		ctx,
+		s.txManager,
+		voting.VoteFromData([]byte(commitID)),
+		voting.Prepared,
+	); err != nil {
+		return "", helper.ErrAbortedf("preparatory vote on squashed commit: %w", err)
+	}
+
+	if err := quarantineDir.Migrate(); err != nil {
+		return "", helper.ErrInternalf("migrating quarantine directory: %w", err)
+	}
+
+	if err := transaction.VoteOnContext(
+		ctx,
+		s.txManager,
+		voting.VoteFromData([]byte(commitID)),
+		voting.Committed,
+	); err != nil {
+		return "", helper.ErrAbortedf("committing vote on squashed commit: %w", err)
 	}
 
 	return commitID, nil
diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go
index f412fa335..57cb362fe 100644
--- a/internal/gitaly/service/operations/squash_test.go
+++ b/internal/gitaly/service/operations/squash_test.go
@@ -17,7 +17,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
@@ -41,11 +40,8 @@ var (
 
 func TestUserSquash_successful(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashSuccessful)
-}
 
-func testUserSquashSuccessful(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	for _, tc := range []struct {
 		desc             string
@@ -101,11 +97,9 @@ func testUserSquashSuccessful(t *testing.T, ctx context.Context) {
 
 func TestUserSquash_transactional(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashTransactional)
-}
 
-func testUserSquashTransactional(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
+
 	txManager := transaction.MockManager{}
 
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx,
@@ -196,46 +190,29 @@ func testUserSquashTransactional(t *testing.T, ctx context.Context) {
 				Timestamp:     &timestamppb.Timestamp{Seconds: 1234512345},
 			})
 
-			if featureflag.UserSquashQuarantinedVoting.IsEnabled(ctx) {
-				if tc.expectedErr == nil {
-					require.NoError(t, err)
-					require.Equal(t, squashedCommitID, response.SquashSha)
-				} else {
-					testhelper.RequireGrpcError(t, tc.expectedErr, err)
-				}
-
-				require.Equal(t, tc.expectedVotes, votes)
-			} else {
-				// There is no transactional voting when the feature flag is
-				// disabled, so we'd never return an error from it either.
+			if tc.expectedErr == nil {
 				require.NoError(t, err)
-				require.Empty(t, votes)
+				require.Equal(t, squashedCommitID, response.SquashSha)
+			} else {
+				testhelper.RequireGrpcError(t, tc.expectedErr, err)
 			}
+			require.Equal(t, tc.expectedVotes, votes)
 
 			exists, err := repo.HasRevision(ctx, git.Revision(squashedCommitID+"^{commit}"))
 			require.NoError(t, err)
 
-			// In case the feature flag is enabled we use a quarantine directory to
-			// stage the new objects. So if we fail to reach quorum in the preparatory
-			// vote we expect the object to not have been migrated to the repository. On
-			// the other hand, if the feature flag is disabled, the object would always
-			// exist no matter whether the RPC is successful or not.
-			if featureflag.UserSquashQuarantinedVoting.IsEnabled(ctx) {
-				require.Equal(t, tc.expectedExists, exists)
-			} else {
-				require.True(t, exists)
-			}
+			// We use a quarantine directory to stage the new objects. So if we fail to
+			// reach quorum in the preparatory vote we expect the object to not have
+			// been migrated to the repository.
+			require.Equal(t, tc.expectedExists, exists)
 		})
 	}
 }
 
 func TestUserSquash_stableID(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashStableID)
-}
 
-func testUserSquashStableID(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 
@@ -294,11 +271,8 @@ func ensureSplitIndexExists(t *testing.T, cfg config.Cfg, repoDir string) bool {
 
 func TestUserSquash_threeWayMerge(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashThreeWayMerge)
-}
 
-func testUserSquashThreeWayMerge(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 
@@ -333,12 +307,8 @@ func testUserSquashThreeWayMerge(t *testing.T, ctx context.Context) {
 
 func TestUserSquash_splitIndex(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashSplitIndex)
-}
-
-func testUserSquashSplitIndex(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	require.False(t, ensureSplitIndexExists(t, cfg, repoPath))
@@ -360,12 +330,9 @@ func testUserSquashSplitIndex(t *testing.T, ctx context.Context) {
 }
 
 func TestUserSquash_renames(t *testing.T) {
-	testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashRenames)
-}
-
-func testUserSquashRenames(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	gittest.AddWorktree(t, cfg, repoPath, "worktree")
@@ -424,12 +391,8 @@ func testUserSquashRenames(t *testing.T, ctx context.Context) {
 
 func TestUserSquash_missingFileOnTargetBranch(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashMissingFileOnTargetBranch)
-}
-
-func testUserSquashMissingFileOnTargetBranch(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	conflictingStartSha := "bbd36ad238d14e1c03ece0f3358f545092dc9ca3"
@@ -451,12 +414,8 @@ func testUserSquashMissingFileOnTargetBranch(t *testing.T, ctx context.Context)
 
 func TestUserSquash_emptyCommit(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashEmptyCommit)
-}
-
-func testUserSquashEmptyCommit(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -653,14 +612,9 @@ func TestUserSquash_validation(t *testing.T) {
 }
 
 func TestUserSquash_conflicts(t *testing.T) {
-	testhelper.NewFeatureSets(
-		featureflag.UserSquashQuarantinedVoting,
-	).Run(t, testUserSquashConflicts)
-}
-
-func testUserSquashConflicts(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries(
@@ -704,12 +658,8 @@ func testUserSquashConflicts(t *testing.T, ctx context.Context) {
 
 func TestUserSquash_ancestry(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashAncestry)
-}
-
-func testUserSquashAncestry(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	// We create an empty parent commit and two commits which both branch off from it. As a
@@ -742,14 +692,8 @@ func testUserSquashAncestry(t *testing.T, ctx context.Context) {
 
 func TestUserSquash_gitError(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(
-		featureflag.UserSquashQuarantinedVoting,
-	).Run(t, testUserSquashGitError)
-}
-
-func testUserSquashGitError(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	testCases := []struct {
diff --git a/internal/metadata/featureflag/ff_user_squash_quarantined_voting.go b/internal/metadata/featureflag/ff_user_squash_quarantined_voting.go
deleted file mode 100644
index baf41ebf0..000000000
--- a/internal/metadata/featureflag/ff_user_squash_quarantined_voting.go
+++ /dev/null
@@ -1,7 +0,0 @@
-package featureflag
-
-// UserSquashQuarantinedVoting enables the use of a quarantine directory to stage all objects
-// created by UserSquash into a temporary directory. This quarantine directory will only be migrated
-// into the final repository when the RPC is successful, including a new transactional vote on the
-// object ID of the resulting squashed commit.
-var UserSquashQuarantinedVoting = NewFeatureFlag("user_squash_quarantined_voting", true)