updateref: Always assert state transitions if Git supports flushing

With 4abf27024 (updateref: Fix indeterminate state when locking refs,
2021-09-13), we have introduced proper verification of state transitions
in git-update-ref(1) such that we notice early on when locking of refs
fails. As a result, we won't try to vote on transactions where we could
have already known that git-update-ref(1) failed to lock refs in the
first place and would thus abort early on.

Remove the feature flag guarding this code. Given that this requires
changes in Git, we still need to verify that the current Git version
supports flushing of status updates.

3 files changed, 14 insertions, 27 deletions

diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go
index dde7b5b76..b00e003b1 100644
--- a/internal/git/updateref/updateref.go
+++ b/internal/git/updateref/updateref.go
@@ -9,7 +9,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/config"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 )
 
 // Updater wraps a `git update-ref --stdin` process, presenting an interface
@@ -72,14 +71,9 @@ func New(ctx context.Context, conf config.Cfg, repo git.RepositoryExecutor, opts
 		return nil, err
 	}
 
-	withStatusFlushing := false
-	if featureflag.UpdaterefVerifyStateChanges.IsEnabled(ctx) {
-		gitVersion, err := git.CurrentVersionForExecutor(ctx, repo)
-		if err != nil {
-			return nil, fmt.Errorf("determining git version: %w", err)
-		}
-
-		withStatusFlushing = gitVersion.FlushesUpdaterefStatus()
+	gitVersion, err := git.CurrentVersionForExecutor(ctx, repo)
+	if err != nil {
+		return nil, fmt.Errorf("determining git version: %w", err)
 	}
 
 	updater := &Updater{
@@ -87,7 +81,7 @@ func New(ctx context.Context, conf config.Cfg, repo git.RepositoryExecutor, opts
 		cmd:                cmd,
 		stderr:             &stderr,
 		stdout:             bufio.NewReader(cmd),
-		withStatusFlushing: withStatusFlushing,
+		withStatusFlushing: gitVersion.FlushesUpdaterefStatus(),
 	}
 
 	// By writing an explicit "start" to the command, we enable
diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go
index aea0de5cb..3668821fe 100644
--- a/internal/git/updateref/updateref_test.go
+++ b/internal/git/updateref/updateref_test.go
@@ -12,7 +12,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/hooks"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
-	"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"
 )
@@ -135,12 +134,11 @@ func TestUpdater_prepareLocksTransaction(t *testing.T) {
 }
 
 func TestUpdater_concurrentLocking(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.UpdaterefVerifyStateChanges,
-	}).Run(t, testUpdaterConcurrentLocking)
-}
+	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testUpdaterConcurrentLocking(t *testing.T, ctx context.Context) {
 	cfg, protoRepo, _ := testcfg.BuildWithRepo(t)
 	repo := localrepo.NewTestRepo(t, cfg, protoRepo)
 
@@ -158,7 +156,7 @@ func testUpdaterConcurrentLocking(t *testing.T, ctx context.Context) {
 
 	// With flushing, we're able to detect concurrent locking at prepare time already instead of
 	// at commit time.
-	if gitSupportsStatusFlushing(t, ctx, cfg) && featureflag.UpdaterefVerifyStateChanges.IsEnabled(ctx) {
+	if gitSupportsStatusFlushing(t, ctx, cfg) {
 		err := secondUpdater.Prepare()
 		require.Error(t, err)
 		require.Contains(t, err.Error(), "state update to \"prepare\" failed: EOF, stderr: \"warning: update refs/heads/master: missing <newvalue>, treating as zero\\nfatal: prepare: cannot lock ref 'refs/heads/master'")
@@ -276,12 +274,11 @@ func TestUpdater_closingStdinAbortsChanges(t *testing.T) {
 }
 
 func TestUpdater_capturesStderr(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.UpdaterefVerifyStateChanges,
-	}).Run(t, testUpdaterCapturesStderr)
-}
+	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testUpdaterCapturesStderr(t *testing.T, ctx context.Context) {
 	cfg, _, updater := setupUpdater(t, ctx)
 
 	ref := "refs/heads/a"
@@ -291,7 +288,7 @@ func testUpdaterCapturesStderr(t *testing.T, ctx context.Context) {
 	require.NoError(t, updater.Update(git.ReferenceName(ref), newValue, oldValue))
 
 	var expectedErr string
-	if gitSupportsStatusFlushing(t, ctx, cfg) && featureflag.UpdaterefVerifyStateChanges.IsEnabled(ctx) {
+	if gitSupportsStatusFlushing(t, ctx, cfg) {
 		expectedErr = fmt.Sprintf("state update to \"commit\" failed: EOF, stderr: \"fatal: commit: cannot update ref '%s': "+
 			"trying to write ref '%s' with nonexistent object %s\\n\"", ref, ref, newValue)
 	} else {
diff --git a/internal/metadata/featureflag/ff_updateref_assert_locking.go b/internal/metadata/featureflag/ff_updateref_assert_locking.go
deleted file mode 100644
index e018fd1e6..000000000
--- a/internal/metadata/featureflag/ff_updateref_assert_locking.go
+++ /dev/null
@@ -1,4 +0,0 @@
-package featureflag
-
-// UpdaterefVerifyStateChanges causes us to assert that transactional state changes finish correctly.
-var UpdaterefVerifyStateChanges = NewFeatureFlag("updateref_verify_state_changes", true)