Merge branch 'pks-updateref-always-assert-locking' into 'master'

updateref: Always assert state transitions if Git supports flushing

Closes #3781

See merge request gitlab-org/gitaly!3904

3 files changed, 14 insertions, 27 deletions

diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go
index 630c3ebb3..b5b672875 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)