From ad400e7ef734158336f3abb82059d3a7e447def7 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Fri, 11 Aug 2023 10:30:07 +0200 Subject: featureflag: Remove the `CatfileBatchCommand` flag The `CatfileBatchCommand` flag was used to rollout the `--batch-command` option of `git-cat-file(1)`. Since the flag has been enabled for a while now, we can delete the flag and default to the enabled state. We cannot remove the old code because we are still dependent on the minimum git version supporting `--batch-command`, which is scheduled to land in git 2.42.0. --- internal/featureflag/ff_catfile_batch_command.go | 9 --------- internal/git/catfile/cache.go | 5 ++--- internal/git/catfile/cache_test.go | 15 ++++----------- internal/git/catfile/tag_test.go | 7 +------ internal/testhelper/testhelper.go | 3 --- 5 files changed, 7 insertions(+), 32 deletions(-) delete mode 100644 internal/featureflag/ff_catfile_batch_command.go diff --git a/internal/featureflag/ff_catfile_batch_command.go b/internal/featureflag/ff_catfile_batch_command.go deleted file mode 100644 index 0702a592d..000000000 --- a/internal/featureflag/ff_catfile_batch_command.go +++ /dev/null @@ -1,9 +0,0 @@ -package featureflag - -// CatfileBatchCommand enables the `--batch-command` mode for git-cat-file(1). -var CatfileBatchCommand = NewFeatureFlag( - "catfile_batch_command", - "v16.2.0", - "https://gitlab.com/gitlab-org/gitaly/-/issues/4573", - false, -) diff --git a/internal/git/catfile/cache.go b/internal/git/catfile/cache.go index d44b0dcdf..494493785 100644 --- a/internal/git/catfile/cache.go +++ b/internal/git/catfile/cache.go @@ -8,7 +8,6 @@ import ( "time" "github.com/prometheus/client_golang/prometheus" - "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" @@ -186,7 +185,7 @@ func (c *ProcessCache) ObjectReader(ctx context.Context, repo git.RepositoryExec return nil, nil, fmt.Errorf("git version: %w", err) } - if featureflag.CatfileBatchCommand.IsEnabled(ctx) && version.CatfileSupportsNulTerminatedOutput() { + if version.CatfileSupportsNulTerminatedOutput() { cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectReaders, func(ctx context.Context) (cacheable, error) { return newObjectReader(ctx, repo, c.catfileLookupCounter) }, "catfile.ObjectReader") @@ -218,7 +217,7 @@ func (c *ProcessCache) ObjectInfoReader(ctx context.Context, repo git.Repository return nil, nil, fmt.Errorf("git version: %w", err) } - if featureflag.CatfileBatchCommand.IsEnabled(ctx) && version.CatfileSupportsNulTerminatedOutput() { + if version.CatfileSupportsNulTerminatedOutput() { cached, cancel, err = c.getOrCreateProcess(ctx, repo, &c.objectReaders, func(ctx context.Context) (cacheable, error) { return newObjectReader(ctx, repo, c.catfileLookupCounter) }, "catfile.ObjectReader") diff --git a/internal/git/catfile/cache_test.go b/internal/git/catfile/cache_test.go index 26c0a439f..d4e371797 100644 --- a/internal/git/catfile/cache_test.go +++ b/internal/git/catfile/cache_test.go @@ -9,7 +9,6 @@ import ( "time" "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" @@ -206,10 +205,7 @@ func TestCache_autoExpiry(t *testing.T) { func TestCache_ObjectReader(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.CatfileBatchCommand).Run(t, testCacheObjectReader) -} - -func testCacheObjectReader(t *testing.T, ctx context.Context) { + ctx := testhelper.Context(t) cfg := testcfg.Build(t) repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ @@ -254,7 +250,7 @@ func testCacheObjectReader(t *testing.T, ctx context.Context) { cancel() var allKeys []key - if featureflag.CatfileBatchCommand.IsEnabled(ctx) && version.CatfileSupportsNulTerminatedOutput() { + if version.CatfileSupportsNulTerminatedOutput() { allKeys = keys(t, &cache.objectReaders) } else { allKeys = keys(t, &cache.objectContentReaders) @@ -320,10 +316,7 @@ func testCacheObjectReader(t *testing.T, ctx context.Context) { func TestCache_ObjectInfoReader(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.CatfileBatchCommand).Run(t, testCacheObjectInfoReader) -} - -func testCacheObjectInfoReader(t *testing.T, ctx context.Context) { + ctx := testhelper.Context(t) cfg := testcfg.Build(t) repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ @@ -367,7 +360,7 @@ func testCacheObjectInfoReader(t *testing.T, ctx context.Context) { cancel() var allKeys []key - if featureflag.CatfileBatchCommand.IsEnabled(ctx) && version.CatfileSupportsNulTerminatedOutput() { + if version.CatfileSupportsNulTerminatedOutput() { allKeys = keys(t, &cache.objectReaders) } else { allKeys = keys(t, &cache.objectInfoReaders) diff --git a/internal/git/catfile/tag_test.go b/internal/git/catfile/tag_test.go index 2e8c03660..8c2e5b015 100644 --- a/internal/git/catfile/tag_test.go +++ b/internal/git/catfile/tag_test.go @@ -1,13 +1,11 @@ package catfile import ( - "context" "fmt" "strings" "testing" "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v16/internal/helper" @@ -18,10 +16,7 @@ import ( func TestGetTag(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.CatfileBatchCommand).Run(t, testGetTag) -} - -func testGetTag(t *testing.T, ctx context.Context) { + ctx := testhelper.Context(t) cfg, objectReader, _, repoPath := setupObjectReader(t, ctx) commitID := gittest.WriteCommit(t, cfg, repoPath) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 17b511bc4..4cf5555ff 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -202,9 +202,6 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context { // deep in the call stack, so almost every test function would have to inject it into its // context. The values of these flags should be randomized to increase the test coverage. ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RunCommandsInCGroup, true) - // CatfileBatchCommand affects many tests since most of them rely on catfile for content/info - // information about objects. - ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.CatfileBatchCommand, rand.Int()%2 == 0) // Randomly enable mailmap ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.MailmapOptions, rand.Int()%2 == 0) // LowerBigFileThreshold is checked on every spawn of Git commands and is thus infeasible to be checked for -- cgit v1.2.3