Merge branch 'jc-remove-write-ref-ff' into 'master'

featureflag: Remove TransactionalSymbolicRefUpdates featureflag

See merge request gitlab-org/gitaly!4467

8 files changed, 48 insertions, 132 deletions

diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go
index a7b22fd14..cd577b6a3 100644
--- a/internal/backup/backup_test.go
+++ b/internal/backup/backup_test.go
@@ -1,7 +1,6 @@
 package backup
 
 import (
-	"context"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -13,7 +12,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/setup"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
-	"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/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
@@ -264,12 +262,9 @@ func TestManager_Create_incremental(t *testing.T) {
 }
 
 func TestManager_Restore(t *testing.T) {
-	testhelper.NewFeatureSets(featureflag.TransactionalSymbolicRefUpdates).Run(t, testManagerRestore)
-}
-
-func testManagerRestore(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	testcfg.BuildGitalyHooks(t, cfg)
 
diff --git a/internal/git/localrepo/refs.go b/internal/git/localrepo/refs.go
index 30cc93000..5b0f9e4df 100644
--- a/internal/git/localrepo/refs.go
+++ b/internal/git/localrepo/refs.go
@@ -14,7 +14,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/safe"
 )
 
@@ -168,17 +167,7 @@ func (repo *Repo) UpdateRef(ctx context.Context, reference git.ReferenceName, ne
 // SetDefaultBranch sets the repository's HEAD to point to the given reference.
 // It will not verify the reference actually exists.
 func (repo *Repo) SetDefaultBranch(ctx context.Context, txManager transaction.Manager, reference git.ReferenceName) error {
-	if featureflag.TransactionalSymbolicRefUpdates.IsEnabled(ctx) {
-		return repo.setDefaultBranchWithTransaction(ctx, txManager, reference)
-	}
-
-	if err := repo.ExecAndWait(ctx, git.SubCmd{
-		Name: "symbolic-ref",
-		Args: []string{"HEAD", reference.String()},
-	}, git.WithRefTxHook(repo)); err != nil {
-		return err
-	}
-	return nil
+	return repo.setDefaultBranchWithTransaction(ctx, txManager, reference)
 }
 
 // setDefaultBranchWithTransaction sets the repostory's HEAD to point to the given reference
diff --git a/internal/git/localrepo/refs_test.go b/internal/git/localrepo/refs_test.go
index df5c1c7ec..bd50f1347 100644
--- a/internal/git/localrepo/refs_test.go
+++ b/internal/git/localrepo/refs_test.go
@@ -18,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"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/safe"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
@@ -456,10 +455,7 @@ func TestRepo_UpdateRef(t *testing.T) {
 }
 
 func TestRepo_SetDefaultBranch(t *testing.T) {
-	testhelper.NewFeatureSets(featureflag.TransactionalSymbolicRefUpdates).Run(t, testRepoSetBranchFeatures)
-}
-
-func testRepoSetBranchFeatures(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	_, repo, _ := setupRepo(t)
 
 	txManager := transaction.NewTrackingManager()
@@ -498,19 +494,17 @@ func testRepoSetBranchFeatures(t *testing.T, ctx context.Context) {
 
 			require.Equal(t, tc.expectedRef, newRef)
 
-			if featureflag.TransactionalSymbolicRefUpdates.IsEnabled(ctx) {
-				require.Len(t, txManager.Votes(), 2)
-				h := voting.NewVoteHash()
-				_, err = h.Write([]byte("ref: " + tc.ref.String() + "\n"))
-				require.NoError(t, err)
-				vote, err := h.Vote()
-				require.NoError(t, err)
+			require.Len(t, txManager.Votes(), 2)
+			h := voting.NewVoteHash()
+			_, err = h.Write([]byte("ref: " + tc.ref.String() + "\n"))
+			require.NoError(t, err)
+			vote, err := h.Vote()
+			require.NoError(t, err)
 
-				require.Equal(t, voting.Prepared, txManager.Votes()[0].Phase)
-				require.Equal(t, vote.String(), txManager.Votes()[0].Vote.String())
-				require.Equal(t, voting.Committed, txManager.Votes()[1].Phase)
-				require.Equal(t, vote.String(), txManager.Votes()[1].Vote.String())
-			}
+			require.Equal(t, voting.Prepared, txManager.Votes()[0].Phase)
+			require.Equal(t, vote.String(), txManager.Votes()[0].Vote.String())
+			require.Equal(t, voting.Committed, txManager.Votes()[1].Phase)
+			require.Equal(t, vote.String(), txManager.Votes()[1].Vote.String())
 		})
 	}
 }
@@ -535,10 +529,7 @@ func (b *blockingManager) Stop(_ context.Context, _ txinfo.Transaction) error {
 }
 
 func TestRepo_SetDefaultBranch_errors(t *testing.T) {
-	ctx := featureflag.ContextWithFeatureFlag(
-		testhelper.Context(t),
-		featureflag.TransactionalSymbolicRefUpdates,
-		true)
+	ctx := testhelper.Context(t)
 
 	t.Run("malformed refname", func(t *testing.T) {
 		_, repo, _ := setupRepo(t)
diff --git a/internal/gitaly/service/repository/create_repository_from_bundle_test.go b/internal/gitaly/service/repository/create_repository_from_bundle_test.go
index 033c161e3..f0bfba7e3 100644
--- a/internal/gitaly/service/repository/create_repository_from_bundle_test.go
+++ b/internal/gitaly/service/repository/create_repository_from_bundle_test.go
@@ -2,7 +2,6 @@ package repository
 
 import (
 	"bytes"
-	"context"
 	"io"
 	"os"
 	"path/filepath"
@@ -16,7 +15,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"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/praefect/praefectutil"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/tempdir"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
@@ -31,11 +29,8 @@ import (
 
 func TestCreateRepositoryFromBundle_successful(t *testing.T) {
 	t.Parallel()
+	ctx := testhelper.Context(t)
 
-	testhelper.NewFeatureSets(featureflag.TransactionalSymbolicRefUpdates).Run(t, testCreateRepositoryFromBundleSuccessful)
-}
-
-func testCreateRepositoryFromBundleSuccessful(t *testing.T, ctx context.Context) {
 	cfg, repo, repoPath, client := setupRepositoryService(ctx, t)
 
 	locator := config.NewLocator(cfg)
@@ -105,10 +100,7 @@ func testCreateRepositoryFromBundleSuccessful(t *testing.T, ctx context.Context)
 func TestCreateRepositoryFromBundle_transactional(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.TransactionalSymbolicRefUpdates).Run(t, testCreateRepositoryFromBundleTransactional)
-}
-
-func testCreateRepositoryFromBundleTransactional(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	txManager := transaction.NewTrackingManager()
 
 	cfg, repoProto, repoPath, client := setupRepositoryService(ctx, t, testserver.WithTransactionManager(txManager))
@@ -175,10 +167,7 @@ func testCreateRepositoryFromBundleTransactional(t *testing.T, ctx context.Conte
 func TestCreateRepositoryFromBundle_invalidBundle(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.TransactionalSymbolicRefUpdates).Run(t, testCreateRepositoryFromBundleInvalidBundle)
-}
-
-func testCreateRepositoryFromBundleInvalidBundle(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
 	stream, err := client.CreateRepositoryFromBundle(ctx)
@@ -215,10 +204,7 @@ func testCreateRepositoryFromBundleInvalidBundle(t *testing.T, ctx context.Conte
 func TestCreateRepositoryFromBundle_invalidArgument(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.TransactionalSymbolicRefUpdates).Run(t, testCreateRepositoryFromBundleInvalidArgument)
-}
-
-func testCreateRepositoryFromBundleInvalidArgument(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	_, client := setupRepositoryServiceWithoutRepo(t)
 
 	stream, err := client.CreateRepositoryFromBundle(ctx)
@@ -233,10 +219,7 @@ func testCreateRepositoryFromBundleInvalidArgument(t *testing.T, ctx context.Con
 func TestCreateRepositoryFromBundle_existingRepository(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.TransactionalSymbolicRefUpdates).Run(t, testCreateRepositoryFromBundleExistingRepository)
-}
-
-func testCreateRepositoryFromBundleExistingRepository(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
 	// The above test creates the second repository on the server. As this test can run with Praefect in front of it,
diff --git a/internal/gitaly/service/repository/fetch_bundle_test.go b/internal/gitaly/service/repository/fetch_bundle_test.go
index ed84aee27..f11327fce 100644
--- a/internal/gitaly/service/repository/fetch_bundle_test.go
+++ b/internal/gitaly/service/repository/fetch_bundle_test.go
@@ -14,7 +14,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/hook"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"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/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
@@ -28,10 +27,7 @@ import (
 func TestServer_FetchBundle_success(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.TransactionalSymbolicRefUpdates).Run(t, testServerFetchBundlesuccess)
-}
-
-func testServerFetchBundlesuccess(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, _, repoPath, client := setupRepositoryService(ctx, t)
 
 	tmp := testhelper.TempDir(t)
@@ -76,10 +72,7 @@ func testServerFetchBundlesuccess(t *testing.T, ctx context.Context) {
 func TestServer_FetchBundle_transaction(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.TransactionalSymbolicRefUpdates).Run(t, testServerFetchBundleTransaction)
-}
-
-func testServerFetchBundleTransaction(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, repoProto, repoPath := testcfg.BuildWithRepo(t)
 	gitCmdFactory := gittest.NewCommandFactory(t, cfg)
 	testcfg.BuildGitalyHooks(t, cfg)
diff --git a/internal/gitaly/service/repository/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go
index 07d011d18..6ac836783 100644
--- a/internal/gitaly/service/repository/replicate_test.go
+++ b/internal/gitaly/service/repository/replicate_test.go
@@ -28,7 +28,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"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/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
@@ -41,12 +40,8 @@ import (
 
 func TestReplicateRepository(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(
-		featureflag.TransactionalSymbolicRefUpdates,
-	).Run(t, testReplicateRepository)
-}
 
-func testReplicateRepository(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfgBuilder := testcfg.NewGitalyCfgBuilder(testcfg.WithStorages("default", "replica"))
 	cfg := cfgBuilder.Build(t)
 
@@ -118,12 +113,8 @@ func testReplicateRepository(t *testing.T, ctx context.Context) {
 
 func TestReplicateRepositoryTransactional(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(
-		featureflag.TransactionalSymbolicRefUpdates,
-	).Run(t, testReplicateRepositoryTransactional)
-}
 
-func testReplicateRepositoryTransactional(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfgBuilder := testcfg.NewGitalyCfgBuilder(testcfg.WithStorages("default", "replica"))
 	cfg := cfgBuilder.Build(t)
 
@@ -303,12 +294,9 @@ func TestReplicateRepositoryInvalidArguments(t *testing.T) {
 
 func TestReplicateRepository_BadRepository(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(
-		featureflag.TransactionalSymbolicRefUpdates,
-	).Run(t, testReplicateRepositoryBadRepository)
-}
 
-func testReplicateRepositoryBadRepository(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
+
 	for _, tc := range []struct {
 		desc          string
 		invalidSource bool
@@ -390,12 +378,8 @@ func testReplicateRepositoryBadRepository(t *testing.T, ctx context.Context) {
 
 func TestReplicateRepository_FailedFetchInternalRemote(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(
-		featureflag.TransactionalSymbolicRefUpdates,
-	).Run(t, testReplicateRepositoryFailedFetchInternalRemote)
-}
 
-func testReplicateRepositoryFailedFetchInternalRemote(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t, testcfg.WithStorages("default", "replica"))
 	testcfg.BuildGitalyHooks(t, cfg)
 	testcfg.BuildGitalySSH(t, cfg)
@@ -475,12 +459,8 @@ func listenGitalySSHCalls(t *testing.T, conf config.Cfg) func() gitalySSHParams
 
 func TestFetchInternalRemote_successful(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(
-		featureflag.TransactionalSymbolicRefUpdates,
-	).Run(t, testFetchInternalRemoteSuccessful)
-}
 
-func testFetchInternalRemoteSuccessful(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	remoteCfg, remoteRepo, remoteRepoPath := testcfg.BuildWithRepo(t)
 	testcfg.BuildGitalyHooks(t, remoteCfg)
 	gittest.WriteCommit(t, remoteCfg, remoteRepoPath, gittest.WithBranch("master"))
@@ -566,12 +546,8 @@ func testFetchInternalRemoteSuccessful(t *testing.T, ctx context.Context) {
 
 func TestFetchInternalRemote_failure(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(
-		featureflag.TransactionalSymbolicRefUpdates,
-	).Run(t, testFetchInternalRemoteFailure)
-}
 
-func testFetchInternalRemoteFailure(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, repoProto, _ := testcfg.BuildWithRepo(t)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 	ctx = testhelper.MergeIncomingMetadata(ctx, testcfg.GitalyServersMetadataFromCfg(t, cfg))
diff --git a/internal/gitaly/service/repository/write_ref_test.go b/internal/gitaly/service/repository/write_ref_test.go
index fdd6cf5e6..5e3b7f92a 100644
--- a/internal/gitaly/service/repository/write_ref_test.go
+++ b/internal/gitaly/service/repository/write_ref_test.go
@@ -2,7 +2,6 @@ package repository
 
 import (
 	"bytes"
-	"context"
 	"path/filepath"
 	"testing"
 
@@ -10,7 +9,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"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"
@@ -57,37 +55,33 @@ func TestWriteRefSuccessful(t *testing.T) {
 		},
 	}
 
-	testhelper.NewFeatureSets(featureflag.TransactionalSymbolicRefUpdates).Run(t, func(t *testing.T, ctx context.Context) {
-		ctx, err := txinfo.InjectTransaction(ctx, 1, "node", true)
-		require.NoError(t, err)
-		ctx = metadata.IncomingToOutgoing(ctx)
+	ctx, err := txinfo.InjectTransaction(testhelper.Context(t), 1, "node", true)
+	require.NoError(t, err)
+	ctx = metadata.IncomingToOutgoing(ctx)
 
-		for _, tc := range testCases {
-			t.Run(tc.desc, func(t *testing.T) {
-				txManager.Reset()
-				_, err = client.WriteRef(ctx, tc.req)
-				require.NoError(t, err)
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			txManager.Reset()
+			_, err = client.WriteRef(ctx, tc.req)
+			require.NoError(t, err)
 
-				if featureflag.TransactionalSymbolicRefUpdates.IsEnabled(ctx) {
-					require.Len(t, txManager.Votes(), tc.expectedVotes)
-				}
+			require.Len(t, txManager.Votes(), tc.expectedVotes)
 
-				if bytes.Equal(tc.req.Ref, []byte("HEAD")) {
-					content := testhelper.MustReadFile(t, filepath.Join(repoPath, "HEAD"))
+			if bytes.Equal(tc.req.Ref, []byte("HEAD")) {
+				content := testhelper.MustReadFile(t, filepath.Join(repoPath, "HEAD"))
 
-					refRevision := bytes.Join([][]byte{[]byte("ref: "), tc.req.Revision, []byte("\n")}, nil)
+				refRevision := bytes.Join([][]byte{[]byte("ref: "), tc.req.Revision, []byte("\n")}, nil)
 
-					require.EqualValues(t, refRevision, content)
-					return
-				}
-				rev := gittest.Exec(t, cfg, "--git-dir", repoPath, "log", "--pretty=%H", "-1", string(tc.req.Ref))
+				require.EqualValues(t, refRevision, content)
+				return
+			}
+			rev := gittest.Exec(t, cfg, "--git-dir", repoPath, "log", "--pretty=%H", "-1", string(tc.req.Ref))
 
-				rev = bytes.Replace(rev, []byte("\n"), nil, 1)
+			rev = bytes.Replace(rev, []byte("\n"), nil, 1)
 
-				require.Equal(t, string(tc.req.Revision), string(rev))
-			})
-		}
-	})
+			require.Equal(t, string(tc.req.Revision), string(rev))
+		})
+	}
 }
 
 func TestWriteRefValidationError(t *testing.T) {
diff --git a/internal/metadata/featureflag/ff_write_ref_manual_voting.go b/internal/metadata/featureflag/ff_write_ref_manual_voting.go
deleted file mode 100644
index d0ec0caed..000000000
--- a/internal/metadata/featureflag/ff_write_ref_manual_voting.go
+++ /dev/null
@@ -1,5 +0,0 @@
-package featureflag
-
-// TransactionalSymbolicRefUpdates allows the WriteRef RPC to use an implementation to update HEAD that does
-// its own transaction voting.
-var TransactionalSymbolicRefUpdates = NewFeatureFlag("transactional_symbolic_ref_updates", true)