From ff35b310859f8775a080be9182b99fd961e1814d Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Wed, 5 Jun 2024 16:22:24 +0200 Subject: [PATCH 1/9] makefile: Introduce a new OVERRIDE_GIT_VERSION variable Gitaly uses bundled Git and we usually lag behind the latest Git versions by a few months due to the nature of how we rollout newer Git versions using featureflags and while staying backwards compatible. Added to this there is a delay between patches being developed on Git and these patches being merged through seen -> next -> master and being tagged with a new version. Sometimes we'd want to start developing on top of these patches in Gitaly. Since they don't have a version set, Gitaly can't parse their versioning. Let's introduce OVERRIDE_GIT_VERSION, which can override the Git version, while still respecting the GIT_VERSION variable which is refers to the reference we build Git against. --- .gitlab-ci.yml | 2 +- Makefile | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d3972d124e..0e87357fd8 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -306,7 +306,7 @@ test:nightly: <<: *test_definition variables: <<: *test_variables - SKIP_OVERRIDING_GIT_VERSION: "YesPlease" + OVERRIDE_GIT_VERSION: "" parallel: matrix: - GIT_VERSION: [ "master", "next" ] diff --git a/Makefile b/Makefile index c25fc5b005..8f58d6c343 100644 --- a/Makefile +++ b/Makefile @@ -132,10 +132,11 @@ GIT_VERSION_2_44 ?= v2.44.1.gl1 ## The Git version used for bundled Git v2.45. GIT_VERSION_2_45 ?= v2.45.1 -## Skip overriding the Git version and instead use the Git version as specified -## in the Git sources. This is required when building Git from a version that -## cannot be parsed by Gitaly. -SKIP_OVERRIDING_GIT_VERSION ?= +## Override the Git version with a custom version if specified. If set to empty +## we do not add the version file and instead use the Git version as specified i +## the Git sources. This is useful to build upon Git features which aren't yet +## part of a release or a version that cannot be parsed by Gitaly (custom refs). +OVERRIDE_GIT_VERSION ?= ${GIT_VERSION} # The default version is used in case the caller does not set the variable or # if it is either set to the empty string or "default". @@ -622,13 +623,15 @@ ${DEPENDENCY_DIR}/git-%/Makefile: ${DEPENDENCY_DIR}/git-%.version ${Q}${GIT} -C "${@D}" fetch --depth 1 ${GIT_QUIET} origin ${GIT_VERSION} ${Q}${GIT} -C "${@D}" reset --hard ${Q}${GIT} -C "${@D}" checkout ${GIT_QUIET} --detach FETCH_HEAD -ifdef SKIP_OVERRIDING_GIT_VERSION +ifeq ($(OVERRIDE_GIT_VERSION),) ${Q}rm -f "${@D}"/version + echo "one" else @ # We're writing the version into the "version" file in Git's own source @ # directory. If it exists, Git's Makefile will pick it up and use it as @ # the version instead of auto-detecting via git-describe(1). - ${Q}echo ${GIT_VERSION} >"${@D}"/version + ${Q}echo ${OVERRIDE_GIT_VERSION} >"${@D}"/version + echo "two" endif ${Q}touch $@ -- GitLab From a31fcf30b35d0b35870001d9961bb8b09739169e Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 4 Jun 2024 18:21:59 +0200 Subject: [PATCH 2/9] ci: Change requirement for running reftable tests The reftable backend was merged in Git 2.45, and our tests have been targeting and running on said version. However the update symref commands are yet to be released in a new version so let's target the branch containing symref-updates and set the version to a high number for now. --- .gitlab-ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 0e87357fd8..1ce0ce9ee5 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -299,7 +299,8 @@ test:reftable: allow_failure: true variables: <<: *test_variables - GIT_VERSION: "v2.45.0" + OVERRIDE_GIT_VERSION: "v99.99.99" + GIT_VERSION: "261-support-transactional-symbolic-reference-updates" TEST_TARGET: test-with-reftable test:nightly: -- GitLab From 9c7e5c90eba9712e5ac1e8a70a86c83ccce0c2c8 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Wed, 5 Jun 2024 17:15:56 +0200 Subject: [PATCH 3/9] updateref: Add support for 'symref-update' Add support for the upcoming 'symref-update' subcommand, which allows us to update symrefs. Since the feature is yet to be released in a new Git version, we block it behind a high Git version. --- internal/git/updateref/updateref.go | 14 +++++++++++ internal/git/updateref/updateref_test.go | 30 ++++++++++++++++++++++++ internal/git/version.go | 8 +++++++ 3 files changed, 52 insertions(+) diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go index 08b90d8ea2..024d3c1780 100644 --- a/internal/git/updateref/updateref.go +++ b/internal/git/updateref/updateref.go @@ -394,6 +394,20 @@ func (u *Updater) Update(reference git.ReferenceName, newOID, oldOID git.ObjectI return u.write("update %s\x00%s\x00%s\x00", reference.String(), newOID, oldOID) } +// SymrefUpdate is used to do a symbolic reference update. We can potentially provide the oldTarget +// or the oldOID. +func (u *Updater) SymrefUpdate(version git.Version, reference, newTarget git.ReferenceName) error { + if !version.SupportSymrefUpdates() { + return fmt.Errorf("incompatible version for symref-updates") + } + + if err := u.expectState(stateStarted); err != nil { + return err + } + + return u.write("symref-update %s\x00%s\x00\x00\x00", reference.String(), newTarget) +} + // Create commands the reference to be created with the given object ID. The ref must not exist. // // A reference transaction must be started before calling Create. diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go index 730bd70c22..c8e1949e6a 100644 --- a/internal/git/updateref/updateref_test.go +++ b/internal/git/updateref/updateref_test.go @@ -15,6 +15,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git" "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/helper/text" "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" @@ -947,3 +948,32 @@ func TestUpdater_multipleUpdateError(t *testing.T) { require.NoError(t, updater.Create("refs/heads/test_a", commitID)) require.Equal(t, MultipleUpdatesError{ReferenceName: "refs/heads/test_a"}, updater.Commit()) } + +func TestUpdater_symrefs(t *testing.T) { + t.Parallel() + + ctx := testhelper.Context(t) + + cfg, executor, repoPath, updater := setupUpdater(t, ctx) + defer testhelper.MustClose(t, updater) + + actual, err := executor.GitVersion(ctx) + require.NoError(t, err) + + gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master")) + + require.NoError(t, updater.Start()) + + if !actual.SupportSymrefUpdates() { + err := updater.SymrefUpdate(actual, "refs/heads/symref", "refs/heads/master") + require.EqualError(t, err, "incompatible version for symref-updates") + require.NoError(t, updater.Commit()) + } else { + require.NoError(t, updater.SymrefUpdate(actual, "refs/heads/symref", "refs/heads/master")) + require.NoError(t, updater.Commit()) + + // Verify that the reference was created as expected. + target := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "symbolic-ref", "refs/heads/symref")) + require.Equal(t, target, "refs/heads/master") + } +} diff --git a/internal/git/version.go b/internal/git/version.go index b04540a5fe..b06ce8edff 100644 --- a/internal/git/version.go +++ b/internal/git/version.go @@ -64,6 +64,14 @@ func parseVersionOutput(versionOutput []byte) (Version, error) { return version, nil } +// SupportSymrefUpdates validates if the version supports 'symref-update' subcommands +// in 'git-update-ref'. This feature has not been tagged yet, so we use a high +// version number to build for it. This should be changed as the feature gets merged +// and released. +func (v Version) SupportSymrefUpdates() bool { + return v.GreaterOrEqual(NewVersion(99, 99, 99, 0)) +} + // String returns the string representation of the version. func (v Version) String() string { return v.versionString -- GitLab From 1f4f42f8cb0441afa1db855b7737c2c0d819da55 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Thu, 6 Jun 2024 15:46:39 +0200 Subject: [PATCH 4/9] gittest: Add function to skip test for a Git version In the upcoming commit, we'll introduce a new way to update default branches using 'symref-udpate' subcommand. This will be part of an upcoming Git release and will rely on reference-transaction hook to run. So existing tests will fail to run with this functionality, so we need to skip them when running for this experimental Git version. Add a function to allow us to do so. Also introduce `IfSymrefUpdateSupported` which will help us pick the write output for tests if the version supports symref-update. --- internal/git/gittest/version.go | 31 +++++++++++++++++++++++++++++++ internal/git/version.go | 20 ++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/internal/git/gittest/version.go b/internal/git/gittest/version.go index 0094f6e7fc..dd1ae48910 100644 --- a/internal/git/gittest/version.go +++ b/internal/git/gittest/version.go @@ -24,3 +24,34 @@ func SkipIfGitVersionLessThan(tb testing.TB, ctx context.Context, cfg config.Cfg tb.Skipf("Unsupported Git version %q, expected minimum %q: %q", actual, expected, reason) } } + +// SkipIfGitVersion skips the test if the Git version is the same. +func SkipIfGitVersion(tb testing.TB, ctx context.Context, cfg config.Cfg, expected git.Version, reason string) { + cmdFactory, clean, err := git.NewExecCommandFactory(cfg, testhelper.SharedLogger(tb)) + require.NoError(tb, err) + defer clean() + + actual, err := cmdFactory.GitVersion(ctx) + require.NoError(tb, err) + + if actual.Equal(expected) { + tb.Skipf("Unsupported Git version %q, expected %q: %q", actual, expected, reason) + } +} + +// IfSymrefUpdateSupported returns the appropriate value based on if symref updates are +// supported or not in the current git version. +func IfSymrefUpdateSupported[T any](tb testing.TB, ctx context.Context, cfg config.Cfg, yes, no T) T { + cmdFactory, clean, err := git.NewExecCommandFactory(cfg, testhelper.SharedLogger(tb)) + require.NoError(tb, err) + defer clean() + + actual, err := cmdFactory.GitVersion(ctx) + require.NoError(tb, err) + + if actual.SupportSymrefUpdates() { + return yes + } + + return no +} diff --git a/internal/git/version.go b/internal/git/version.go index b06ce8edff..fddb0036b1 100644 --- a/internal/git/version.go +++ b/internal/git/version.go @@ -117,6 +117,26 @@ func (v Version) LessThan(other Version) bool { } } +// Equal determines whether the version is the same as another version. +func (v Version) Equal(other Version) bool { + switch { + case v.major != other.major: + return false + case v.minor != other.minor: + return false + case v.patch != other.patch: + return false + case v.rc != other.rc: + return false + case v.gl != other.gl: + return false + + default: + // this should only be reachable when versions are equal + return true + } +} + // GreaterOrEqual determines whether the version is newer than or equal to another version. func (v Version) GreaterOrEqual(other Version) bool { return !v.LessThan(other) -- GitLab From 9c901492651aca0c2895eb82e4a664642217f5eb Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Thu, 6 Jun 2024 15:49:01 +0200 Subject: [PATCH 5/9] localrepo: Allow updating default branch via git-update-ref(1) To update the default branch of repository, we simply modify the HEAD file of the repository. To do this in Praefect, we create a HEAD.lock file and vote on the content of the file. This works because we use the filesystem backend and would break with the upcoming reftable backend. To solve this issue, we introduced 'symref-update' command to Git. This feature is yet to be merged and tagged in an official Git release, but we can already start adding the required code and tests in our CI. In a previous commit, we introduced the `updateref.SymrefUpdate` to update symrefs. Using this, we introduce `setDefaultBranchWithUpdateRef` in the `localrepo` package. Now when the Git version is set to the experimental version (v99.99.99), we will use this function over the existing function which has been renamed to `setDefaultBranchManually`. While earlier we manually voted on the contents of the HEAD.lock file for Praefect. Now we simply rely on the existing architecture of the reference-transaction hook to capture this detail for us. This means existing tests will not work for the newly introduced code since they don't instantiate the reference transaction hook, so we introduce new tests for the same. The tests would cause an import loop so we add them to 'localrepo_test' package to avoid the looping. Eventually this flow would be made default and we can clear up all this messy glue code. --- internal/git/localrepo/refs.go | 44 ++- internal/git/localrepo/refs_external_test.go | 259 ++++++++++++++++++ internal/git/localrepo/refs_test.go | 22 +- .../service/repository/replicate_test.go | 4 +- 4 files changed, 317 insertions(+), 12 deletions(-) create mode 100644 internal/git/localrepo/refs_external_test.go diff --git a/internal/git/localrepo/refs.go b/internal/git/localrepo/refs.go index cce0092017..b386e141b7 100644 --- a/internal/git/localrepo/refs.go +++ b/internal/git/localrepo/refs.go @@ -151,12 +151,52 @@ func (repo *Repo) UpdateRef(ctx context.Context, reference git.ReferenceName, ne // SetDefaultBranch sets the repository's HEAD to point to the given reference. // It will not verify the reference actually exists. func (repo *Repo) SetDefaultBranch(ctx context.Context, txManager transaction.Manager, reference git.ReferenceName) error { - return repo.setDefaultBranchWithTransaction(ctx, txManager, reference) + version, err := repo.GitVersion(ctx) + if err != nil { + return fmt.Errorf("detecting Git version: %w", err) + } + + if version.SupportSymrefUpdates() { + return repo.setDefaultBranchWithUpdateRef(ctx, txManager, reference, version) + } + + return repo.setDefaultBranchManually(ctx, txManager, reference) +} + +// setDefaultBranchWithUpdateRef uses 'symref-update' command to update HEAD. +func (repo *Repo) setDefaultBranchWithUpdateRef( + ctx context.Context, + txManager transaction.Manager, + reference git.ReferenceName, + version git.Version, +) error { + updater, err := updateref.New(ctx, repo, updateref.WithNoDeref()) + if err != nil { + return fmt.Errorf("creating updateref: %w", err) + } + + if err := updater.Start(); err != nil { + return fmt.Errorf("start: %w", err) + } + + if err := updater.SymrefUpdate(version, "HEAD", reference); err != nil { + return fmt.Errorf("update: %w", err) + } + + if err := updater.Commit(); err != nil { + return fmt.Errorf("commit: %w", err) + } + + if err := updater.Close(); err != nil { + return fmt.Errorf("close: %w", err) + } + + 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) setDefaultBranchWithTransaction(ctx context.Context, txManager transaction.Manager, reference git.ReferenceName) error { +func (repo *Repo) setDefaultBranchManually(ctx context.Context, txManager transaction.Manager, reference git.ReferenceName) error { if err := git.ValidateReference(reference.String()); err != nil { return fmt.Errorf("%q is a malformed refname", reference) } diff --git a/internal/git/localrepo/refs_external_test.go b/internal/git/localrepo/refs_external_test.go new file mode 100644 index 0000000000..7ebe9b792b --- /dev/null +++ b/internal/git/localrepo/refs_external_test.go @@ -0,0 +1,259 @@ +package localrepo_test + +import ( + "bytes" + "context" + "errors" + "fmt" + "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/git" + "gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile" + "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" + "gitlab.com/gitlab-org/gitaly/v16/internal/git/updateref" + "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/transaction" + "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" + "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testserver" + "gitlab.com/gitlab-org/gitaly/v16/internal/transaction/txinfo" + "gitlab.com/gitlab-org/gitaly/v16/internal/transaction/voting" + "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" + "google.golang.org/grpc" + "google.golang.org/grpc/peer" +) + +func setupRepoWithHooksServer(t *testing.T, ctx context.Context, cfg config.Cfg, opts ...testserver.GitalyServerOpt) (string, *localrepo.Repo) { + testcfg.BuildGitalyHooks(t, cfg) + + cfg.SocketPath = testserver.RunGitalyServer(t, cfg, func(srv *grpc.Server, deps *service.Dependencies) { + gitalypb.RegisterHookServiceServer(srv, hook.NewServer(deps)) + }, opts...) + + repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ + SkipCreationViaService: true, + }) + gitCmdFactory := gittest.NewCommandFactory(t, cfg) + catfileCache := catfile.NewCache(cfg) + t.Cleanup(catfileCache.Stop) + repo := localrepo.New(testhelper.NewLogger(t), config.NewLocator(cfg), gitCmdFactory, catfileCache, repoProto) + + return repoPath, repo +} + +func TestRepo_SetDefaultBranch(t *testing.T) { + t.Parallel() + + testCases := []struct { + desc string + ref git.ReferenceName + expectedRef git.ReferenceName + }{ + { + desc: "update the branch ref", + ref: "refs/heads/feature", + expectedRef: "refs/heads/feature", + }, + { + desc: "unknown ref", + ref: "refs/heads/non_existent_ref", + expectedRef: "refs/heads/non_existent_ref", + }, + } + for _, tc := range testCases { + tc := tc + + t.Run(tc.desc, func(t *testing.T) { + t.Parallel() + + ctx := testhelper.Context(t) + cfg := testcfg.Build(t) + + gittest.SkipIfGitVersionLessThan(t, ctx, cfg, git.NewVersion(99, 99, 99, 0), "This requires symref-update in Git") + + txManager := transaction.NewTrackingManager() + txManager.Reset() + ctx, err := txinfo.InjectTransaction( + peer.NewContext(ctx, &peer.Peer{}), + 1, + "node", + true, + ) + require.NoError(t, err) + + repoPath, repo := setupRepoWithHooksServer(t, ctx, cfg, testserver.WithTransactionManager(txManager)) + + gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master")) + gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("feature")) + + require.NoError(t, repo.SetDefaultBranch(ctx, txManager, tc.ref)) + + newRef, err := repo.HeadReference(ctx) + require.NoError(t, err) + + require.Equal(t, tc.expectedRef, newRef) + + require.Len(t, txManager.Votes(), 2) + h := voting.NewVoteHash() + _, err = h.Write([]byte(fmt.Sprintf("%s ref:%s %s\n", gittest.DefaultObjectHash.ZeroOID, tc.ref.String(), "HEAD"))) + require.NoError(t, err) + vote, err := h.Vote() + require.NoError(t, err) + + require.Equal(t, voting.Prepared, txManager.Votes()[0].Phase) + require.Equal(t, vote.String(), txManager.Votes()[0].Vote.String()) + require.Equal(t, voting.Committed, txManager.Votes()[1].Phase) + require.Equal(t, vote.String(), txManager.Votes()[1].Vote.String()) + }) + } +} + +func TestRepo_SetDefaultBranch_errors(t *testing.T) { + t.Parallel() + + ctx := testhelper.Context(t) + + t.Run("malformed refname", func(t *testing.T) { + t.Parallel() + + cfg := testcfg.Build(t) + gittest.SkipIfGitVersionLessThan(t, ctx, cfg, git.NewVersion(99, 99, 99, 0), "This requires symref-update in Git") + + _, repo := setupRepoWithHooksServer(t, ctx, cfg) + + invalidRefname := "./.lock" + + err := repo.SetDefaultBranch(ctx, &transaction.MockManager{}, git.ReferenceName(invalidRefname)) + require.ErrorIs(t, err, updateref.InvalidReferenceFormatError{ReferenceName: invalidRefname}) + }) + + t.Run("HEAD is locked by another process", func(t *testing.T) { + t.Parallel() + + cfg := testcfg.Build(t) + gittest.SkipIfGitVersionLessThan(t, ctx, cfg, git.NewVersion(99, 99, 99, 0), "This requires symref-update in Git") + + _, repo := setupRepoWithHooksServer(t, ctx, cfg) + + ref, err := repo.HeadReference(ctx) + require.NoError(t, err) + + version, err := repo.GitVersion(ctx) + require.NoError(t, err) + + updater, err := updateref.New(ctx, repo) + require.NoError(t, err) + + require.NoError(t, updater.Start()) + require.NoError(t, updater.SymrefUpdate(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.FilesOrReftables( + updateref.AlreadyLockedError{ReferenceName: "HEAD"}, + updateref.AlreadyLockedError{}, + )) + + refAfter, err := repo.HeadReference(ctx) + require.NoError(t, err) + require.Equal(t, ref, refAfter) + }) + + t.Run("HEAD is locked by SetDefaultBranch", func(t *testing.T) { + t.Parallel() + + testhelper.SkipWithReftable(t, "reftable doesn't add HEAD.lock") + cfg := testcfg.Build(t) + gittest.SkipIfGitVersionLessThan(t, ctx, cfg, git.NewVersion(99, 99, 99, 0), "This requires symref-update in Git") + + ctx, err := txinfo.InjectTransaction( + peer.NewContext(ctx, &peer.Peer{}), + 1, + "node", + true, + ) + require.NoError(t, err) + + ch := make(chan struct{}) + doneCh := make(chan struct{}) + + _, repo := setupRepoWithHooksServer(t, ctx, cfg, testserver.WithTransactionManager(&blockingManager{ch})) + + go func() { + _ = repo.SetDefaultBranch(ctx, nil, "refs/heads/branch") + doneCh <- struct{}{} + }() + <-ch + + var stderr bytes.Buffer + err = repo.ExecAndWait(ctx, git.Command{ + Name: "symbolic-ref", + Args: []string{"HEAD", "refs/heads/otherbranch"}, + }, git.WithRefTxHook(repo), git.WithStderr(&stderr)) + + code, ok := command.ExitStatus(err) + require.True(t, ok) + assert.Equal(t, 1, code) + assert.Regexp(t, "Unable to create .+\\/HEAD\\.lock': File exists.", stderr.String()) + ch <- struct{}{} + <-doneCh + }) + + t.Run("failing vote unlocks symref", func(t *testing.T) { + t.Parallel() + + cfg := testcfg.Build(t) + gittest.SkipIfGitVersionLessThan(t, ctx, cfg, git.NewVersion(99, 99, 99, 0), "This requires symref-update in Git") + + ctx, err := txinfo.InjectTransaction( + peer.NewContext(ctx, &peer.Peer{}), + 1, + "node", + true, + ) + require.NoError(t, err) + + failingTxManager := &transaction.MockManager{ + VoteFn: func(context.Context, txinfo.Transaction, voting.Vote, voting.Phase) error { + return errors.New("injected error") + }, + } + + _, repo := setupRepoWithHooksServer(t, ctx, cfg, testserver.WithTransactionManager(failingTxManager)) + + err = repo.SetDefaultBranch(ctx, nil, "refs/heads/branch") + require.Error(t, err) + + var sErr structerr.Error + require.ErrorAs(t, err, &sErr) + require.Contains(t, sErr.Metadata(), "stderr") + require.Equal(t, "error executing git hook\nfatal: ref updates aborted by hook\n", sErr.Metadata()["stderr"]) + }) +} + +type blockingManager struct { + ch chan struct{} +} + +func (b *blockingManager) Vote(_ context.Context, _ txinfo.Transaction, _ voting.Vote, phase voting.Phase) error { + // the purpose of this is to block SetDefaultBranch from completing, so just choose to block on + // a Prepared vote. + if phase == voting.Prepared { + b.ch <- struct{}{} + <-b.ch + } + + return nil +} + +func (b *blockingManager) Stop(_ context.Context, _ txinfo.Transaction) error { + return nil +} diff --git a/internal/git/localrepo/refs_test.go b/internal/git/localrepo/refs_test.go index 9cac85424c..d191cb1099 100644 --- a/internal/git/localrepo/refs_test.go +++ b/internal/git/localrepo/refs_test.go @@ -591,8 +591,6 @@ func TestRepo_UpdateRef(t *testing.T) { } func TestRepo_SetDefaultBranch(t *testing.T) { - testhelper.SkipWithReftable(t, "localrepo.SetDefaultBranch modifies HEAD through the filesystem directly") - t.Parallel() testCases := []struct { @@ -620,6 +618,8 @@ func TestRepo_SetDefaultBranch(t *testing.T) { ctx := testhelper.Context(t) cfg, repo, repoPath := setupRepo(t) + gittest.SkipIfGitVersion(t, ctx, cfg, git.NewVersion(99, 99, 99, 0), "These tests don't work with symref-update") + gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master")) gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("feature")) @@ -656,12 +656,12 @@ func TestRepo_SetDefaultBranch(t *testing.T) { } func TestRepo_HeadReference(t *testing.T) { - testhelper.SkipWithReftable(t, "localrepo.SetDefaultBranch modifies HEAD through the filesystem directly") - t.Parallel() ctx := testhelper.Context(t) - _, repo, _ := setupRepo(t) + cfg, repo, _ := setupRepo(t) + + gittest.SkipIfGitVersion(t, ctx, cfg, git.NewVersion(99, 99, 99, 0), "These tests don't work with symref-update") referenceName, err := repo.HeadReference(ctx) require.NoError(t, err) @@ -702,7 +702,8 @@ func TestRepo_SetDefaultBranch_errors(t *testing.T) { t.Run("malformed refname", func(t *testing.T) { t.Parallel() - _, repo, _ := setupRepo(t) + cfg, repo, _ := setupRepo(t) + gittest.SkipIfGitVersion(t, ctx, cfg, git.NewVersion(99, 99, 99, 0), "These tests don't work with symref-update") err := repo.SetDefaultBranch(ctx, &transaction.MockManager{}, "./.lock") require.EqualError(t, err, `"./.lock" is a malformed refname`) @@ -711,7 +712,8 @@ func TestRepo_SetDefaultBranch_errors(t *testing.T) { t.Run("HEAD is locked by another process", func(t *testing.T) { t.Parallel() - _, repo, _ := setupRepo(t) + cfg, repo, _ := setupRepo(t) + gittest.SkipIfGitVersion(t, ctx, cfg, git.NewVersion(99, 99, 99, 0), "These tests don't work with symref-update") ref, err := repo.HeadReference(ctx) require.NoError(t, err) @@ -743,7 +745,8 @@ func TestRepo_SetDefaultBranch_errors(t *testing.T) { require.NoError(t, err) - _, repo, _ := setupRepo(t) + cfg, repo, _ := setupRepo(t) + gittest.SkipIfGitVersion(t, ctx, cfg, git.NewVersion(99, 99, 99, 0), "These tests don't work with symref-update") ch := make(chan struct{}) doneCh := make(chan struct{}) @@ -778,7 +781,8 @@ func TestRepo_SetDefaultBranch_errors(t *testing.T) { ) require.NoError(t, err) - _, repo, repoPath := setupRepo(t) + cfg, repo, repoPath := setupRepo(t) + gittest.SkipIfGitVersion(t, ctx, cfg, git.NewVersion(99, 99, 99, 0), "These tests don't work with symref-update") failingTxManager := &transaction.MockManager{ VoteFn: func(context.Context, txinfo.Transaction, voting.Vote, voting.Phase) error { diff --git a/internal/gitaly/service/repository/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go index 8c777eb552..2baa61479b 100644 --- a/internal/gitaly/service/repository/replicate_test.go +++ b/internal/gitaly/service/repository/replicate_test.go @@ -650,7 +650,9 @@ func TestFetchInternalRemote_successful(t *testing.T) { }, ) - require.Equal(t, 2, referenceTransactionHookCalled) + // 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) } func TestFetchInternalRemote_failure(t *testing.T) { -- GitLab From f16d6628125d62557981705bfcdfd7b5b50f13a7 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Sun, 28 Apr 2024 21:00:58 +0200 Subject: [PATCH 6/9] repository: Remove `testhelper.SkipWithReftable()` With the fixes around making reftable work with default branch, remove all the `testhelper.SkipWithReftable()` in the repository package. --- .../gitaly/service/repository/create_repository_test.go | 7 ++----- internal/gitaly/service/repository/fetch_bundle_test.go | 2 -- internal/gitaly/service/repository/write_ref_test.go | 2 -- 3 files changed, 2 insertions(+), 9 deletions(-) diff --git a/internal/gitaly/service/repository/create_repository_test.go b/internal/gitaly/service/repository/create_repository_test.go index 14999b5c55..7eb65b17b2 100644 --- a/internal/gitaly/service/repository/create_repository_test.go +++ b/internal/gitaly/service/repository/create_repository_test.go @@ -3,7 +3,6 @@ package repository import ( "fmt" "os" - "path" "path/filepath" "testing" @@ -44,8 +43,6 @@ func TestCreateRepository_missingAuth(t *testing.T) { } func TestCreateRepository_successful(t *testing.T) { - testhelper.SkipWithReftable(t, "reads HEAD from the filesystem and only verifies refs/ folder permissions") - t.Parallel() ctx := testhelper.Context(t) @@ -77,8 +74,8 @@ func TestCreateRepository_successful(t *testing.T) { require.NoError(t, unix.Access(dir, unix.X_OK)) } - symRef := testhelper.MustReadFile(t, path.Join(repoDir, "HEAD")) - require.Equal(t, symRef, []byte(fmt.Sprintf("ref: %s\n", git.DefaultRef))) + symRef := string(gittest.Exec(t, cfg, "-C", repoDir, "symbolic-ref", "HEAD")) + require.Equal(t, symRef, fmt.Sprintf("%s\n", git.DefaultRef)) } func TestCreateRepository_withDefaultBranch(t *testing.T) { diff --git a/internal/gitaly/service/repository/fetch_bundle_test.go b/internal/gitaly/service/repository/fetch_bundle_test.go index ce6b5b216f..0ee12eadbf 100644 --- a/internal/gitaly/service/repository/fetch_bundle_test.go +++ b/internal/gitaly/service/repository/fetch_bundle_test.go @@ -22,8 +22,6 @@ import ( ) func TestServer_FetchBundle_success(t *testing.T) { - testhelper.SkipWithReftable(t, "localrepo.SetDefaultBranch modifies HEAD through the filesystem directly") - t.Parallel() ctx := testhelper.Context(t) diff --git a/internal/gitaly/service/repository/write_ref_test.go b/internal/gitaly/service/repository/write_ref_test.go index a85c84c5e6..5da864b8b7 100644 --- a/internal/gitaly/service/repository/write_ref_test.go +++ b/internal/gitaly/service/repository/write_ref_test.go @@ -250,8 +250,6 @@ func TestWriteRef(t *testing.T) { { desc: "update default branch", setup: func(t *testing.T) setupData { - testhelper.SkipWithReftable(t, "localrepo.SetDefaultBranch modifies HEAD through the filesystem directly") - repo, repoPath := gittest.CreateRepository(t, ctx, cfg) defaultCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(git.DefaultBranch)) -- GitLab From b58d72cd41adcd3e7bab3602d8ebfc35e82d6961 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Mon, 29 Apr 2024 21:54:07 +0200 Subject: [PATCH 7/9] backup: Remove `testhelper.SkipWithReftable()` With the fixes around making reftable work with default branch, remove all the `testhelper.SkipWithReftable()` in the backup package. --- internal/backup/backup_test.go | 4 ---- internal/backup/repository_test.go | 4 ---- 2 files changed, 8 deletions(-) diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index 546b72a798..db1300474f 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -417,8 +417,6 @@ custom_hooks_path = '%[2]s/001.custom_hooks.tar' } func TestManager_Restore_latest(t *testing.T) { - testhelper.SkipWithReftable(t, "localrepo.SetDefaultBranch modifies HEAD through the filesystem directly") - t.Parallel() cfg := testcfg.Build(t) @@ -810,8 +808,6 @@ custom_hooks_path = '%[2]s/%[3]s/002.custom_hooks.tar' } func TestManager_Restore_specific(t *testing.T) { - testhelper.SkipWithReftable(t, "localrepo.SetDefaultBranch modifies HEAD through the filesystem directly") - t.Parallel() const backupID = "abc123" diff --git a/internal/backup/repository_test.go b/internal/backup/repository_test.go index 47cc7f6a06..fe9789562b 100644 --- a/internal/backup/repository_test.go +++ b/internal/backup/repository_test.go @@ -130,8 +130,6 @@ func TestLocalRepository_ResetRefs(t *testing.T) { } func TestRemoteRepository_SetHeadReference(t *testing.T) { - testhelper.SkipWithReftable(t, "SetHeadReference modifies HEAD through the filesystem directly") - cfg := testcfg.Build(t) testcfg.BuildGitalyHooks(t, cfg) cfg.SocketPath = testserver.RunGitalyServer(t, cfg, setup.RegisterAll) @@ -171,8 +169,6 @@ func TestLocalRepository_SetHeadReference(t *testing.T) { t.Skip("local backup manager expects to operate on the local filesystem so cannot operate through praefect") } - testhelper.SkipWithReftable(t, "SetHeadReference modifies HEAD through the filesystem directly") - cfg := testcfg.Build(t) ctx := testhelper.Context(t) -- GitLab From 36bf89330edf687380b041144bc2e2b9a26d0f6a Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Mon, 29 Apr 2024 21:57:30 +0200 Subject: [PATCH 8/9] localrepo: Remove `testhelper.SkipWithReftable()` With the fixes around making reftable work with default branch, remove all the `testhelper.SkipWithReftable()` in the localrepo package. --- internal/git/localrepo/bundle_test.go | 2 -- internal/git/localrepo/refs_external_test.go | 7 ++++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/git/localrepo/bundle_test.go b/internal/git/localrepo/bundle_test.go index d74ccbd4ff..12fe03173c 100644 --- a/internal/git/localrepo/bundle_test.go +++ b/internal/git/localrepo/bundle_test.go @@ -295,8 +295,6 @@ func TestRepo_FetchBundle(t *testing.T) { desc: "HEAD update", updateHead: true, setup: func(t *testing.T, ctx context.Context, cfg config.Cfg, targetRepoPath string) testSetup { - testhelper.SkipWithReftable(t, "localrepo.SetDefaultBranch modifies HEAD through the filesystem directly") - _, sourceRepoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ SkipCreationViaService: true, }) diff --git a/internal/git/localrepo/refs_external_test.go b/internal/git/localrepo/refs_external_test.go index 7ebe9b792b..7aff37e4f8 100644 --- a/internal/git/localrepo/refs_external_test.go +++ b/internal/git/localrepo/refs_external_test.go @@ -202,7 +202,12 @@ func TestRepo_SetDefaultBranch_errors(t *testing.T) { code, ok := command.ExitStatus(err) require.True(t, ok) assert.Equal(t, 1, code) - assert.Regexp(t, "Unable to create .+\\/HEAD\\.lock': File exists.", stderr.String()) + + assert.Regexp(t, gittest.FilesOrReftables( + "Unable to create .+\\/HEAD\\.lock': File exists.", + "error: unable to write symref for HEAD: data is locked", + ), stderr.String()) + ch <- struct{}{} <-doneCh }) -- GitLab From f7f615fa7158da2015a655e4d074dc31f7c5559a Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Fri, 7 Jun 2024 16:45:10 +0200 Subject: [PATCH 9/9] fixup! repository: Remove `testhelper.SkipWithReftable()` --- .../service/repository/write_ref_test.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/internal/gitaly/service/repository/write_ref_test.go b/internal/gitaly/service/repository/write_ref_test.go index 5da864b8b7..8331e96f96 100644 --- a/internal/gitaly/service/repository/write_ref_test.go +++ b/internal/gitaly/service/repository/write_ref_test.go @@ -266,10 +266,20 @@ func TestWriteRef(t *testing.T) { git.NewReference(git.DefaultRef, defaultCommit), git.NewReference("refs/heads/new-default", newCommit), }, - expectedVotes: []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: 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"))}, + }, + ), } }, }, -- GitLab