From 5580f0065297ac864d1a8c285beebc5e6deaa9a6 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Wed, 16 Oct 2024 21:13:28 +0200 Subject: [PATCH 1/4] featureflag: Remove featureflag Git v2.47 The new Git version of v2.47 was added behind a featureflag. Now that we have rolled out the feature flag, we can remove the flag. --- internal/featureflag/ff_git_v247.go | 9 --------- internal/git/gitcmd/execution_environment.go | 3 --- internal/testhelper/testhelper.go | 3 --- 3 files changed, 15 deletions(-) delete mode 100644 internal/featureflag/ff_git_v247.go diff --git a/internal/featureflag/ff_git_v247.go b/internal/featureflag/ff_git_v247.go deleted file mode 100644 index d8d9760669..0000000000 --- a/internal/featureflag/ff_git_v247.go +++ /dev/null @@ -1,9 +0,0 @@ -package featureflag - -// GitV247 enables the use Git v2.47. -var GitV247 = NewFeatureFlag( - "git_v247", - "v17.5.0", - "https://gitlab.com/gitlab-org/gitaly/-/issues/6419", - false, -) diff --git a/internal/git/gitcmd/execution_environment.go b/internal/git/gitcmd/execution_environment.go index ef3521487c..742be12605 100644 --- a/internal/git/gitcmd/execution_environment.go +++ b/internal/git/gitcmd/execution_environment.go @@ -23,9 +23,6 @@ var ( BundledGitConstructors = []BundledGitEnvironmentConstructor{ { Suffix: "-v2.47", - FeatureFlags: []featureflag.FeatureFlag{ - featureflag.GitV247, - }, }, { Suffix: "-v2.46", diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index e86c4767ae..62b998955e 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -301,9 +301,6 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context { ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RemoveCatfileCacheSessionID, rnd.Int()%2 == 0) - // Randomly enable either Git version 2.46 or 2.47. - ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.GitV247, rnd.Int()%2 == 0) - for _, opt := range opts { ctx = opt(ctx) } -- GitLab From 589bdfdfffb02b6df52158088ea647355dc71d14 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Wed, 16 Oct 2024 22:30:39 +0200 Subject: [PATCH 2/4] git: Make Git v2.47 the default Now that we have two versions of Git, let's make the newer version the default. This allows us to remove the older version in a follow up commit. Also remove the requirement in the reftable CI job, since we already meet the required version. This bumps the reftable tests to run with Git v2.47, which included changes to allow reftables to compact tables, even if some tables were locked. This requries changes in our tests to reflect the same. --- .gitlab-ci.yml | 3 +- Makefile | 2 +- ...transaction_manager_default_branch_test.go | 42 ++++++------------- .../transaction_manager_housekeeping_test.go | 10 ----- .../transaction_manager_refs_test.go | 19 +-------- .../transaction_manager_repo_test.go | 14 ++----- .../partition/transaction_manager_test.go | 28 ++++--------- 7 files changed, 27 insertions(+), 91 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 4d991cfa0c..6eeac6534f 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -14,7 +14,7 @@ default: .versions: debian: "bookworm" git_default: "default" - git_minimum: "v2.46.0" + git_minimum: "v2.47.0" go_default: "1.22" go_supported: ["1.22", "1.23"] macos: "macos-14-xcode-15" @@ -302,7 +302,6 @@ test:reftable: variables: <<: *test_variables GITALY_TEST_ENABLE_SYMREF_UPDATE: "true" - GIT_VERSION: "v2.46.0" TEST_TARGET: test-with-reftable test:nightly: diff --git a/Makefile b/Makefile index 1aacac7a18..c4284c1ff7 100644 --- a/Makefile +++ b/Makefile @@ -148,7 +148,7 @@ OVERRIDE_GIT_VERSION ?= ${GIT_VERSION} ifeq (${GIT_VERSION:default=},) # GIT_VERSION should be overridden to the default version of bundled Git. This is only # necessary until https://gitlab.com/gitlab-org/gitaly/-/issues/6195 is complete. - override GIT_VERSION := ${GIT_VERSION_2_46} + override GIT_VERSION := ${GIT_VERSION_2_47} # When GIT_VERSION is not explicitly set, we default to bundled Git. export WITH_BUNDLED_GIT = YesPlease else diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_default_branch_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_default_branch_test.go index 3f85e34d03..5bfd1c5fc9 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_default_branch_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_default_branch_test.go @@ -247,16 +247,6 @@ func generateDefaultBranchTests(t *testing.T, setup testTransactionSetup) []tran }, { MinIndex: 2, - MaxIndex: 2, - References: []git.Reference{ - { - Name: "refs/heads/main", - Target: setup.Commits.First.OID.String(), - }, - }, - }, - { - MinIndex: 3, MaxIndex: 3, References: []git.Reference{ { @@ -264,6 +254,10 @@ func generateDefaultBranchTests(t *testing.T, setup testTransactionSetup) []tran Target: "refs/heads/non-existent", IsSymbolic: true, }, + { + Name: "refs/heads/main", + Target: setup.Commits.First.OID.String(), + }, }, }, }, @@ -348,16 +342,6 @@ func generateDefaultBranchTests(t *testing.T, setup testTransactionSetup) []tran }, { MinIndex: 3, - MaxIndex: 3, - References: []git.Reference{ - { - Name: "refs/heads/branch2", - Target: setup.ObjectHash.ZeroOID.String(), - }, - }, - }, - { - MinIndex: 4, MaxIndex: 4, References: []git.Reference{ { @@ -365,6 +349,10 @@ func generateDefaultBranchTests(t *testing.T, setup testTransactionSetup) []tran Target: "refs/heads/branch2", IsSymbolic: true, }, + { + Name: "refs/heads/branch2", + Target: setup.ObjectHash.ZeroOID.String(), + }, }, }, }, @@ -450,16 +438,6 @@ func generateDefaultBranchTests(t *testing.T, setup testTransactionSetup) []tran }, { MinIndex: 3, - MaxIndex: 3, - References: []git.Reference{ - { - Name: "refs/heads/main", - Target: setup.Commits.Second.OID.String(), - }, - }, - }, - { - MinIndex: 4, MaxIndex: 4, References: []git.Reference{ { @@ -467,6 +445,10 @@ func generateDefaultBranchTests(t *testing.T, setup testTransactionSetup) []tran Target: "refs/heads/branch2", IsSymbolic: true, }, + { + Name: "refs/heads/main", + Target: setup.Commits.Second.OID.String(), + }, }, }, }, diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping_test.go index 635d7919a3..f0e36bd571 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping_test.go @@ -4650,16 +4650,6 @@ func generateHousekeepingRepackingConcurrentTests(t *testing.T, ctx context.Cont }, { MinIndex: 3, - MaxIndex: 3, - References: []git.Reference{ - { - Name: "refs/heads/branch-2", - Target: setup.Commits.Diverging.OID.String(), - }, - }, - }, - { - MinIndex: 4, MaxIndex: 4, References: []git.Reference{ { diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go index f3362806e5..d46394eed7 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go @@ -481,19 +481,12 @@ func generateModifyReferencesTests(t *testing.T, setup testTransactionSetup) []t }, { MinIndex: 3, - MaxIndex: 3, + MaxIndex: 4, References: []git.Reference{ { Name: "refs/heads/parent", Target: setup.ObjectHash.ZeroOID.String(), }, - }, - }, - - { - MinIndex: 4, - MaxIndex: 4, - References: []git.Reference{ { Name: "refs/heads/parent/child", Target: setup.Commits.First.OID.String(), @@ -2084,16 +2077,6 @@ func generateModifyReferencesTests(t *testing.T, setup testTransactionSetup) []t }, { MinIndex: 2, - MaxIndex: 2, - References: []git.Reference{ - { - Name: "refs/heads/main", - Target: setup.Commits.First.OID.String(), - }, - }, - }, - { - MinIndex: 3, MaxIndex: 3, References: []git.Reference{ { diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_repo_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_repo_test.go index 0c20353811..785204ee33 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_repo_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_repo_test.go @@ -1155,16 +1155,6 @@ func generateDeleteRepositoryTests(t *testing.T, setup testTransactionSetup) []t }, { MinIndex: 2, - MaxIndex: 2, - References: []git.Reference{ - { - Name: "refs/heads/main", - Target: setup.Commits.First.OID.String(), - }, - }, - }, - { - MinIndex: 3, MaxIndex: 3, References: []git.Reference{ { @@ -1172,6 +1162,10 @@ func generateDeleteRepositoryTests(t *testing.T, setup testTransactionSetup) []t Target: "refs/heads/new-head", IsSymbolic: true, }, + { + Name: "refs/heads/main", + Target: setup.Commits.First.OID.String(), + }, }, }, }, diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go index d51dc21416..1e4672b346 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go @@ -1608,16 +1608,6 @@ func generateCommonTests(t *testing.T, ctx context.Context, setup testTransactio }, { MinIndex: 2, - MaxIndex: 2, - References: []git.Reference{ - { - Name: "refs/heads/main", - Target: setup.Commits.First.OID.String(), - }, - }, - }, - { - MinIndex: 3, MaxIndex: 3, References: []git.Reference{ { @@ -1625,6 +1615,10 @@ func generateCommonTests(t *testing.T, ctx context.Context, setup testTransactio Target: "refs/heads/new-head", IsSymbolic: true, }, + { + Name: "refs/heads/main", + Target: setup.Commits.First.OID.String(), + }, }, }, }, @@ -1684,16 +1678,6 @@ func generateCommonTests(t *testing.T, ctx context.Context, setup testTransactio }, { MinIndex: 2, - MaxIndex: 2, - References: []git.Reference{ - { - Name: "refs/heads/main", - Target: setup.Commits.First.OID.String(), - }, - }, - }, - { - MinIndex: 3, MaxIndex: 3, References: []git.Reference{ { @@ -1701,6 +1685,10 @@ func generateCommonTests(t *testing.T, ctx context.Context, setup testTransactio Target: "refs/heads/new-head", IsSymbolic: true, }, + { + Name: "refs/heads/main", + Target: setup.Commits.First.OID.String(), + }, }, }, }, -- GitLab From 0deefccd43e2bcde822b251e641b04d1519a9498 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Wed, 16 Oct 2024 22:33:10 +0200 Subject: [PATCH 3/4] git: Make Git v2.47 as the minimum version In the previous commit, we made Git v2.47 as the minimum version of Git. Now let's completely remove the older version and make this the default version. --- Makefile | 14 +++----------- README.md | 2 +- internal/git/gitcmd/execution_environment.go | 3 --- internal/git/version.go | 4 ++-- internal/git/version_test.go | 6 +++--- 5 files changed, 9 insertions(+), 20 deletions(-) diff --git a/Makefile b/Makefile index c4284c1ff7..a55274bebe 100644 --- a/Makefile +++ b/Makefile @@ -136,7 +136,6 @@ GIT_VERSION ?= # GIT_VERSION_x_xx defines versions for each instance of bundled Git we ship. When a new # major version is added, be sure to update GIT_PACKED_EXECUTABLES, the *-bundled-git targets, # and add new targets under the "# These targets build specific releases of Git." section. -GIT_VERSION_2_46 ?= v2.46.2 GIT_VERSION_2_47 ?= v2.47.0 # # OVERRIDE_GIT_VERSION allows you to specify a custom semver value to be reported by the @@ -216,8 +215,7 @@ BUILD_GEM_OPTIONS ?= BUILD_GEM_NAME ?= gitaly # Git binaries that are eventually embedded into the Gitaly binary. -GIT_PACKED_EXECUTABLES = $(addprefix ${BUILD_DIR}/bin/gitaly-, $(addsuffix -v2.46, ${GIT_EXECUTABLES})) \ - $(addprefix ${BUILD_DIR}/bin/gitaly-, $(addsuffix -v2.47, ${GIT_EXECUTABLES})) +GIT_PACKED_EXECUTABLES = $(addprefix ${BUILD_DIR}/bin/gitaly-, $(addsuffix -v2.47, ${GIT_EXECUTABLES})) # All executables provided by Gitaly. GITALY_EXECUTABLES = $(addprefix ${BUILD_DIR}/bin/,$(notdir $(shell find ${SOURCE_DIR}/cmd -mindepth 1 -maxdepth 1 -type d -print))) @@ -306,15 +304,13 @@ install: build .PHONY: build-bundled-git ## Build bundled Git binaries. -build-bundled-git: build-bundled-git-v2.46 build-bundled-git-v2.47 -build-bundled-git-v2.46: $(patsubst %,${BUILD_DIR}/bin/gitaly-%-v2.46,${GIT_EXECUTABLES}) +build-bundled-git: build-bundled-git-v2.47 build-bundled-git-v2.47: $(patsubst %,${BUILD_DIR}/bin/gitaly-%-v2.47,${GIT_EXECUTABLES}) .PHONY: install-bundled-git ## Install bundled Git binaries. The target directory can be modified by ## setting PREFIX and DESTDIR. -install-bundled-git: install-bundled-git-v2.46 install-bundled-git-v2.47 -install-bundled-git-v2.46: $(patsubst %,${INSTALL_DEST_DIR}/gitaly-%-v2.46,${GIT_EXECUTABLES}) +install-bundled-git: install-bundled-git-v2.47 install-bundled-git-v2.47: $(patsubst %,${INSTALL_DEST_DIR}/gitaly-%-v2.47,${GIT_EXECUTABLES}) ifdef WITH_BUNDLED_GIT @@ -574,10 +570,6 @@ ${DEPENDENCY_DIR}/git-distribution/git: ${DEPENDENCY_DIR}/git-distribution/Makef ${Q}touch $@ # These targets build specific releases of Git. -${BUILD_DIR}/bin/gitaly-%-v2.46: override GIT_VERSION = ${GIT_VERSION_2_46} -${BUILD_DIR}/bin/gitaly-%-v2.46: ${DEPENDENCY_DIR}/git-v2.46/% | ${BUILD_DIR}/bin - ${Q}install $< $@ - ${BUILD_DIR}/bin/gitaly-%-v2.47: override GIT_VERSION = ${GIT_VERSION_2_47} ${BUILD_DIR}/bin/gitaly-%-v2.47: ${DEPENDENCY_DIR}/git-v2.47/% | ${BUILD_DIR}/bin ${Q}install $< $@ diff --git a/README.md b/README.md index 8b1b1f50a7..2665e70689 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ Most users won't install Gitaly on its own. It is already included in [your GitL Gitaly requires Go 1.22. Run `make` to compile the executables required by Gitaly. -Gitaly uses `git`. Versions `2.46.0` and newer are supported. +Gitaly uses `git`. Versions `2.47.0` and newer are supported. ## Configuration diff --git a/internal/git/gitcmd/execution_environment.go b/internal/git/gitcmd/execution_environment.go index 742be12605..75a2e7b4d0 100644 --- a/internal/git/gitcmd/execution_environment.go +++ b/internal/git/gitcmd/execution_environment.go @@ -24,9 +24,6 @@ var ( { Suffix: "-v2.47", }, - { - Suffix: "-v2.46", - }, } // defaultExecutionEnvironmentConstructors is the list of Git environments supported by the diff --git a/internal/git/version.go b/internal/git/version.go index 55a6f81669..2fe5323b4a 100644 --- a/internal/git/version.go +++ b/internal/git/version.go @@ -14,9 +14,9 @@ import ( // - https://docs.gitlab.com/ee/install/installation.html#software-requirements // - https://docs.gitlab.com/ee/update/ (see e.g. https://docs.gitlab.com/ee/update/#1440) var minimumVersion = Version{ - versionString: "2.46.0", + versionString: "2.47.0", major: 2, - minor: 46, + minor: 47, patch: 0, rc: false, diff --git a/internal/git/version_test.go b/internal/git/version_test.go index 173de8b601..1ad60e760f 100644 --- a/internal/git/version_test.go +++ b/internal/git/version_test.go @@ -197,9 +197,9 @@ func TestVersion_IsSupported(t *testing.T) { {"2.24.0", false}, {"2.43.0", false}, {"2.44.0", false}, - {"2.45.0-rc0", false}, - {"2.45.0", false}, - {"2.46.0", true}, + {"2.46.0-rc0", false}, + {"2.46.0", false}, + {"2.47.0", true}, {"3.0.0", true}, {"3.0.0.gl5", true}, } { -- GitLab From 40b593c2b5f4ec389ee627a22888af7df302e8ed Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Thu, 17 Oct 2024 13:43:03 +0200 Subject: [PATCH 4/4] update-ref: Make using 'symref-update' default behavior We added a featureflag to rollout using 'symref-update' in 'git-update-ref(1)'. The rollout is now done, so we can remove the flag and cleanup all code. This involves cleaning the old flow where we manually obtained a lock on the 'HEAD' file and instead now relying completely on the reference transaction hook. --- .gitlab-ci.yml | 1 - internal/featureflag/ff_symref_updates.go | 16 ------ internal/git/gittest/version.go | 10 ---- internal/git/localrepo/refs.go | 50 +------------------ internal/git/localrepo/refs_external_test.go | 41 +++++---------- .../service/repository/replicate_test.go | 5 +- .../service/repository/write_ref_test.go | 22 +++----- .../storagemgr/partition/testhelper_test.go | 9 ++-- .../partition/transaction_manager_test.go | 10 +--- internal/testhelper/testhelper.go | 3 -- 10 files changed, 27 insertions(+), 140 deletions(-) delete mode 100644 internal/featureflag/ff_symref_updates.go diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 6eeac6534f..1a188c746d 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -301,7 +301,6 @@ test:reftable: <<: *test_definition variables: <<: *test_variables - GITALY_TEST_ENABLE_SYMREF_UPDATE: "true" TEST_TARGET: test-with-reftable test:nightly: diff --git a/internal/featureflag/ff_symref_updates.go b/internal/featureflag/ff_symref_updates.go deleted file mode 100644 index 89eeadd2dc..0000000000 --- a/internal/featureflag/ff_symref_updates.go +++ /dev/null @@ -1,16 +0,0 @@ -package featureflag - -// SymrefUpdate is used to handle the rollout of using symref-update -// subcommands. This is required because the new command uses a different -// voting hash and to stay backward compatible we need to ensure that -// all nodes are on the same version before enabling this. -// -// Note that the command itself will be released in Git 2.46.0, and hence -// can only be enabled once that Git version is set to be the minimum (or -// if the patches are backported). -var SymrefUpdate = NewFeatureFlag( - "symref_update", - "v17.2.0", - "https://gitlab.com/gitlab-org/gitaly/-/issues/6153", - false, -) diff --git a/internal/git/gittest/version.go b/internal/git/gittest/version.go index fc956b4810..d0bb99e89a 100644 --- a/internal/git/gittest/version.go +++ b/internal/git/gittest/version.go @@ -5,7 +5,6 @@ import ( "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/gitcmd" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" @@ -40,12 +39,3 @@ func SkipIfGitVersion(tb testing.TB, ctx context.Context, cfg config.Cfg, expect tb.Skipf("Unsupported Git version %q, expected %q: %q", actual, expected, reason) } } - -// IfSymrefUpdateSupported returns the appropriate value based on if symref updates are enabled. -func IfSymrefUpdateSupported[T any](tb testing.TB, ctx context.Context, cfg config.Cfg, yes, no T) T { - if featureflag.SymrefUpdate.IsEnabled(ctx) { - return yes - } - - return no -} diff --git a/internal/git/localrepo/refs.go b/internal/git/localrepo/refs.go index 244d8656bc..39e224d9cb 100644 --- a/internal/git/localrepo/refs.go +++ b/internal/git/localrepo/refs.go @@ -7,17 +7,13 @@ import ( "errors" "fmt" "io" - "path/filepath" "strings" "gitlab.com/gitlab-org/gitaly/v16/internal/command" - "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" "gitlab.com/gitlab-org/gitaly/v16/internal/git/updateref" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" - "gitlab.com/gitlab-org/gitaly/v16/internal/safe" ) // ErrMismatchingState is similar to updateref.MismatchingStateError. It's declared separately to avoid @@ -162,14 +158,7 @@ func (repo *Repo) SetDefaultBranch(ctx context.Context, txManager transaction.Ma return fmt.Errorf("%q is a malformed refname", reference) } - // To ensure we stay backward compatible, we should only use the new mechanism - // once all nodes have been updated to contain the code for symref updates. - // This is the reason we use a feature flag to propagate this change. - if featureflag.SymrefUpdate.IsEnabled(ctx) { - return repo.setDefaultBranchWithUpdateRef(ctx, reference, version) - } - - return repo.setDefaultBranchManually(ctx, txManager, reference) + return repo.setDefaultBranchWithUpdateRef(ctx, reference, version) } // setDefaultBranchWithUpdateRef uses 'symref-update' command to update HEAD. @@ -204,43 +193,6 @@ func (repo *Repo) setDefaultBranchWithUpdateRef( return nil } -// setDefaultBranchWithTransaction sets the repository's HEAD to point to the given reference -// using a safe locking file writer and commits the transaction if one exists in the context -func (repo *Repo) setDefaultBranchManually(ctx context.Context, txManager transaction.Manager, reference git.ReferenceName) error { - newHeadContent := []byte(fmt.Sprintf("ref: %s\n", reference.String())) - - repoPath, err := repo.Path(ctx) - if err != nil { - return fmt.Errorf("getting repository path: %w", err) - } - - headPath := filepath.Join(repoPath, "HEAD") - - lockingFileWriter, err := safe.NewLockingFileWriter(headPath) - if err != nil { - return fmt.Errorf("getting file writer: %w", err) - } - defer func() { - if err := lockingFileWriter.Close(); err != nil { - repo.logger.WithError(err).ErrorContext(ctx, "closing locked HEAD failed") - } - }() - - if _, err := lockingFileWriter.Write(newHeadContent); err != nil { - return fmt.Errorf("writing temporary HEAD: %w", err) - } - - if err := transaction.CommitLockedFile(ctx, txManager, lockingFileWriter); err != nil { - return fmt.Errorf("committing temporary HEAD: %w", err) - } - - if tx := storage.ExtractTransaction(ctx); tx != nil { - tx.MarkDefaultBranchUpdated() - } - - return nil -} - type getRemoteReferenceConfig struct { patterns []string config []gitcmd.ConfigPair diff --git a/internal/git/localrepo/refs_external_test.go b/internal/git/localrepo/refs_external_test.go index 01698c2339..2bfee94903 100644 --- a/internal/git/localrepo/refs_external_test.go +++ b/internal/git/localrepo/refs_external_test.go @@ -5,14 +5,11 @@ import ( "context" "errors" "fmt" - "os" - "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/command" - "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" @@ -22,9 +19,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service/hook" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/mode" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction" - "gitlab.com/gitlab-org/gitaly/v16/internal/safe" "gitlab.com/gitlab-org/gitaly/v16/internal/structerr" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg" @@ -123,10 +118,7 @@ func TestRepo_SetDefaultBranch(t *testing.T) { require.Len(t, txManager.Votes(), 2) h := voting.NewVoteHash() - _, err = fmt.Fprint(h, gittest.IfSymrefUpdateSupported(t, ctx, cfg, - fmt.Sprintf("%s ref:%s %s\n", gittest.DefaultObjectHash.ZeroOID, tc.ref.String(), "HEAD"), - fmt.Sprintf("ref: %s\n", tc.ref.String()), - )) + _, err = fmt.Fprintf(h, "%s ref:%s %s\n", gittest.DefaultObjectHash.ZeroOID, tc.ref.String(), "HEAD") require.NoError(t, err) vote, err := h.Vote() @@ -162,7 +154,7 @@ func TestRepo_SetDefaultBranch_errors(t *testing.T) { t.Parallel() cfg := testcfg.Build(t) - repoPath, repo := setupRepoWithHooksServer(t, ctx, cfg) + _, repo := setupRepoWithHooksServer(t, ctx, cfg) ref, err := repo.HeadReference(ctx) require.NoError(t, err) @@ -173,20 +165,16 @@ func TestRepo_SetDefaultBranch_errors(t *testing.T) { updater, err := updateref.New(ctx, repo) require.NoError(t, err) - if featureflag.SymrefUpdate.IsEnabled(ctx) { - require.NoError(t, updater.Start()) - require.NoError(t, updater.UpdateSymbolicReference(version, "HEAD", "refs/heads/temp")) - require.NoError(t, updater.Prepare()) - t.Cleanup(func() { require.NoError(t, updater.Close()) }) - } else { - require.NoError(t, os.WriteFile(filepath.Join(repoPath, "HEAD.lock"), []byte(""), mode.File)) - } + require.NoError(t, updater.Start()) + require.NoError(t, updater.UpdateSymbolicReference(version, "HEAD", "refs/heads/temp")) + require.NoError(t, updater.Prepare()) + t.Cleanup(func() { require.NoError(t, updater.Close()) }) err = repo.SetDefaultBranch(ctx, &transaction.MockManager{}, "refs/heads/branch") - require.ErrorIs(t, err, gittest.IfSymrefUpdateSupported(t, ctx, cfg, gittest.FilesOrReftables[error]( + require.ErrorIs(t, err, gittest.FilesOrReftables[error]( updateref.AlreadyLockedError{ReferenceName: "HEAD"}, updateref.AlreadyLockedError{}, - ), safe.ErrFileAlreadyLocked)) + )) refAfter, err := repo.HeadReference(ctx) require.NoError(t, err) @@ -258,19 +246,14 @@ func TestRepo_SetDefaultBranch_errors(t *testing.T) { }, } - repoPath, repo := setupRepoWithHooksServer(t, ctx, cfg, testserver.WithTransactionManager(failingTxManager)) + _, repo := setupRepoWithHooksServer(t, ctx, cfg, testserver.WithTransactionManager(failingTxManager)) err = repo.SetDefaultBranch(ctx, failingTxManager, "refs/heads/branch") require.Error(t, err) - if featureflag.SymrefUpdate.IsEnabled(ctx) { - var sErr structerr.Error - require.ErrorAs(t, err, &sErr) - require.Equal(t, "error executing git hook\nfatal: ref updates aborted by hook\n", sErr.Metadata()["stderr"]) - } else { - require.Equal(t, "committing temporary HEAD: voting on locked file: preimage vote: injected error", err.Error()) - require.NoFileExists(t, filepath.Join(repoPath, "HEAD.lock")) - } + var sErr structerr.Error + require.ErrorAs(t, err, &sErr) + require.Equal(t, "error executing git hook\nfatal: ref updates aborted by hook\n", sErr.Metadata()["stderr"]) }) } diff --git a/internal/gitaly/service/repository/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go index 6566b2fa76..ead5daa3a5 100644 --- a/internal/gitaly/service/repository/replicate_test.go +++ b/internal/gitaly/service/repository/replicate_test.go @@ -649,9 +649,8 @@ func TestFetchInternalRemote_successful(t *testing.T) { }, ) - // When using 'symref-updates' we use the reference-transaction - // hook for the default branch updates. - require.Equal(t, gittest.IfSymrefUpdateSupported(t, ctx, remoteCfg, 4, 2), referenceTransactionHookCalled) + // We use the reference-transaction hook for the default branch updates. + require.Equal(t, 4, referenceTransactionHookCalled) } func TestFetchInternalRemote_failure(t *testing.T) { diff --git a/internal/gitaly/service/repository/write_ref_test.go b/internal/gitaly/service/repository/write_ref_test.go index 8331e96f96..08c5f41768 100644 --- a/internal/gitaly/service/repository/write_ref_test.go +++ b/internal/gitaly/service/repository/write_ref_test.go @@ -266,20 +266,14 @@ func TestWriteRef(t *testing.T) { git.NewReference(git.DefaultRef, defaultCommit), git.NewReference("refs/heads/new-default", newCommit), }, - expectedVotes: gittest.IfSymrefUpdateSupported(t, ctx, cfg, - []transaction.PhasedVote{ - {Phase: voting.Prepared, Vote: voting.VoteFromData([]byte( - fmt.Sprintf("%s ref:refs/heads/new-default HEAD\n", gittest.DefaultObjectHash.ZeroOID), - ))}, - {Phase: voting.Committed, Vote: voting.VoteFromData([]byte( - fmt.Sprintf("%s ref:refs/heads/new-default HEAD\n", gittest.DefaultObjectHash.ZeroOID), - ))}, - }, - []transaction.PhasedVote{ - {Phase: voting.Prepared, Vote: voting.VoteFromData([]byte("ref: refs/heads/new-default\n"))}, - {Phase: voting.Committed, Vote: voting.VoteFromData([]byte("ref: refs/heads/new-default\n"))}, - }, - ), + expectedVotes: []transaction.PhasedVote{ + {Phase: voting.Prepared, Vote: voting.VoteFromData([]byte( + fmt.Sprintf("%s ref:refs/heads/new-default HEAD\n", gittest.DefaultObjectHash.ZeroOID), + ))}, + {Phase: voting.Committed, Vote: voting.VoteFromData([]byte( + fmt.Sprintf("%s ref:refs/heads/new-default HEAD\n", gittest.DefaultObjectHash.ZeroOID), + ))}, + }, } }, }, diff --git a/internal/gitaly/storage/storagemgr/partition/testhelper_test.go b/internal/gitaly/storage/storagemgr/partition/testhelper_test.go index f30e2e62ab..8762216dad 100644 --- a/internal/gitaly/storage/storagemgr/partition/testhelper_test.go +++ b/internal/gitaly/storage/storagemgr/partition/testhelper_test.go @@ -18,7 +18,6 @@ import ( io_prometheus_client "github.com/prometheus/client_model/go" "github.com/stretchr/testify/assert" "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/catfile" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" @@ -1239,11 +1238,9 @@ func runTransactionTest(t *testing.T, ctx context.Context, tc transactionTestCas if step.DefaultBranchUpdate != nil { transaction.MarkDefaultBranchUpdated() - if featureflag.SymrefUpdate.IsEnabled(ctx) { - transaction.UpdateReferences(map[git.ReferenceName]git.ReferenceUpdate{ - "HEAD": {NewTarget: step.DefaultBranchUpdate.Reference}, - }) - } + transaction.UpdateReferences(map[git.ReferenceName]git.ReferenceUpdate{ + "HEAD": {NewTarget: step.DefaultBranchUpdate.Reference}, + }) require.NoError(t, rewrittenRepo.SetDefaultBranch(storage.ContextWithTransaction(ctx, transaction), nil, step.DefaultBranchUpdate.Reference)) } diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go index 1e4672b346..3bcf86be91 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go @@ -20,7 +20,6 @@ import ( "github.com/stretchr/testify/assert" "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/catfile" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" @@ -326,15 +325,8 @@ func setupTest(t *testing.T, ctx context.Context, testPartitionID storage.Partit func TestTransactionManager(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.SymrefUpdate).Run(t, testTransactionManager) -} - -func testTransactionManager(t *testing.T, ctx context.Context) { - t.Parallel() - if featureflag.SymrefUpdate.IsDisabled(ctx) { - testhelper.SkipWithReftable(t, "reftable tests can only be run with symref-updates feature") - } + ctx := testhelper.Context(t) // testPartitionID is the partition ID used in the tests for the TransactionManager. const testPartitionID storage.PartitionID = 1 diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 62b998955e..15e2cce72a 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -292,9 +292,6 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context { ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.MailmapOptions, rnd.Int()%2 == 0) // Disable LogGitTraces ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.LogGitTraces, false) - // Enable SymrefUpdates - symrefUpdateEnabled, _ := env.GetBool("GITALY_TEST_ENABLE_SYMREF_UPDATE", false) - ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.SymrefUpdate, symrefUpdateEnabled) // Enable reftable backend, if env variable set newRepoReftableEnabled := env.GetString("GIT_DEFAULT_REF_FORMAT", "files") ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.NewRepoReftableBackend, newRepoReftableEnabled == "reftable") -- GitLab