diff options
author | Sami Hiltunen <shiltunen@gitlab.com> | 2021-09-23 14:48:28 +0300 |
---|---|---|
committer | Sami Hiltunen <shiltunen@gitlab.com> | 2021-09-23 14:48:28 +0300 |
commit | 4de5d6363006d15af32c50c1461603e11b0f3640 (patch) | |
tree | d24ab1e323e6745bd69a79988509f8a03d93bc46 /internal | |
parent | 744461d9752f2dd22dd6f8c24b5b55df2d60ca60 (diff) | |
parent | 6e2b5c3e59031d83be93b15a277113111e13849c (diff) |
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
Diffstat (limited to 'internal')
-rw-r--r-- | internal/git/updateref/updateref.go | 14 | ||||
-rw-r--r-- | internal/git/updateref/updateref_test.go | 23 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_updateref_assert_locking.go | 4 |
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) |