diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d3972d124e6fc65049f70fee6c83cdffcb0d96e6..1ce0ce9ee581e477ffaee24be613525ae52f1d33 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -299,14 +299,15 @@ 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: <<: *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 c25fc5b00599d044965256c9dfd18cb87cb5dd37..8f58d6c3437bcee225cf10cc88da463eb66b68bf 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 $@ diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index 546b72a798fecf9705f330baf98d557e2ad588eb..db1300474f2032152b7d1c2cc4ab47094e58691c 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 47cc7f6a06a9ec7716bb58942aeba883ce33adb8..fe9789562bb867194c0661f8284475a223fbd4c0 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) diff --git a/internal/git/gittest/version.go b/internal/git/gittest/version.go index 0094f6e7fc5619cd1bac4d28ae7e31efa1ce00ea..dd1ae48910923a22842a3190103373b3c3fe4885 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/localrepo/bundle_test.go b/internal/git/localrepo/bundle_test.go index d74ccbd4ffd72c859158d79209334a71aa2dbdca..12fe03173cd19e114d4598ae97d5cac2ae3d7df4 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.go b/internal/git/localrepo/refs.go index cce0092017bf9221e97c0cafa9446d0012919e67..b386e141b7c708921274972b887e98d9775dc2c8 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 0000000000000000000000000000000000000000..7aff37e4f8bcf55b1cb3f8a322c2307ceaa13412 --- /dev/null +++ b/internal/git/localrepo/refs_external_test.go @@ -0,0 +1,264 @@ +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, gittest.FilesOrReftables( + "Unable to create .+\\/HEAD\\.lock': File exists.", + "error: unable to write symref for HEAD: data is locked", + ), 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 9cac85424c1c79738e7dcaea093d79d080229dce..d191cb1099d064625d7c5785876dcaa202436ea6 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/git/updateref/updateref.go b/internal/git/updateref/updateref.go index 08b90d8ea2a82542ab36b20fa9a1d39616882c05..024d3c17808c6a46b965803b160fb72e5f977a03 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 730bd70c2254119aac05d8fab12c9fc36463267a..c8e1949e6aa3c5454d0a60a9bc421155c88f8124 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 b04540a5fec23e3c347a61ebb73162add17f619d..fddb0036b1358a71487a13e714fb8b4666815fb8 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 @@ -109,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) diff --git a/internal/gitaly/service/repository/create_repository_test.go b/internal/gitaly/service/repository/create_repository_test.go index 14999b5c55a54999bb8d076dd360c49e516fde40..7eb65b17b213dd6538884ff1311632c406f06d9d 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 ce6b5b216ffd47a08cc59d90beeaa6bcae1195b3..0ee12eadbf465741d9ec02c6e17d44a54d716a83 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/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go index 8c777eb55279305232a530ca5965c742e21b864a..2baa61479b59e93b502187387a2107b9795fc9d4 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) { diff --git a/internal/gitaly/service/repository/write_ref_test.go b/internal/gitaly/service/repository/write_ref_test.go index a85c84c5e648f384410d493a27db99d76a397984..8331e96f96b108fbc5395350e83c6d4d734ca04c 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)) @@ -268,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"))}, + }, + ), } }, },