diff --git a/internal/git/gittest/repository_suite.go b/internal/git/gittest/repository_suite.go index 885af8b4059ede9c84ad2252e27f1cb6b4fbf049..2ac6f18b72a4ffb76d95b903e5907664b25366c5 100644 --- a/internal/git/gittest/repository_suite.go +++ b/internal/git/gittest/repository_suite.go @@ -8,6 +8,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/text" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" ) @@ -19,7 +20,7 @@ type GetRepositoryFunc func(t testing.TB, ctx context.Context) (git.Repository, func TestRepository(t *testing.T, cfg config.Cfg, getRepository GetRepositoryFunc) { for _, tc := range []struct { desc string - test func(*testing.T, config.Cfg, GetRepositoryFunc) + test func(*testing.T, context.Context, config.Cfg, GetRepositoryFunc) }{ { desc: "ResolveRevision", @@ -35,14 +36,14 @@ func TestRepository(t *testing.T, cfg config.Cfg, getRepository GetRepositoryFun }, } { t.Run(tc.desc, func(t *testing.T) { - tc.test(t, cfg, getRepository) + testhelper.NewFeatureSets(featureflag.HeadAsDefaultBranch).Run(t, func(t *testing.T, ctx context.Context) { + tc.test(t, ctx, cfg, getRepository) + }) }) } } -func testRepositoryResolveRevision(t *testing.T, cfg config.Cfg, getRepository GetRepositoryFunc) { - ctx := testhelper.Context(t) - +func testRepositoryResolveRevision(t *testing.T, ctx context.Context, cfg config.Cfg, getRepository GetRepositoryFunc) { repo, repoPath := getRepository(t, ctx) firstParentCommitID := WriteCommit(t, cfg, repoPath, WithMessage("first parent")) @@ -96,9 +97,7 @@ func testRepositoryResolveRevision(t *testing.T, cfg config.Cfg, getRepository G } } -func testRepositoryHasBranches(t *testing.T, cfg config.Cfg, getRepository GetRepositoryFunc) { - ctx := testhelper.Context(t) - +func testRepositoryHasBranches(t *testing.T, ctx context.Context, cfg config.Cfg, getRepository GetRepositoryFunc) { repo, repoPath := getRepository(t, ctx) emptyCommit := text.ChompBytes(Exec(t, cfg, "-C", repoPath, "commit-tree", DefaultObjectHash.EmptyTreeOID.String())) @@ -116,9 +115,7 @@ func testRepositoryHasBranches(t *testing.T, cfg config.Cfg, getRepository GetRe require.True(t, hasBranches) } -func testRepositoryGetDefaultBranch(t *testing.T, cfg config.Cfg, getRepository GetRepositoryFunc) { - ctx := testhelper.Context(t) - +func testRepositoryGetDefaultBranch(t *testing.T, ctx context.Context, cfg config.Cfg, getRepository GetRepositoryFunc) { for _, tc := range []struct { desc string repo func(t *testing.T) git.Repository @@ -142,7 +139,10 @@ func testRepositoryGetDefaultBranch(t *testing.T, cfg config.Cfg, getRepository WriteCommit(t, cfg, repoPath, WithParents(oid), WithBranch("master")) return repo }, - expectedName: git.LegacyDefaultRef, + expectedName: testhelper.EnabledOrDisabledFlag(ctx, featureflag.HeadAsDefaultBranch, + git.DefaultRef, + git.LegacyDefaultRef, + ), }, { desc: "no branches", @@ -150,6 +150,10 @@ func testRepositoryGetDefaultBranch(t *testing.T, cfg config.Cfg, getRepository repo, _ := getRepository(t, ctx) return repo }, + expectedName: testhelper.EnabledOrDisabledFlag(ctx, featureflag.HeadAsDefaultBranch, + git.DefaultRef, + git.ReferenceName(""), + ), }, { desc: "one branch", @@ -158,7 +162,10 @@ func testRepositoryGetDefaultBranch(t *testing.T, cfg config.Cfg, getRepository WriteCommit(t, cfg, repoPath, WithBranch("apple")) return repo }, - expectedName: git.NewReferenceNameFromBranchName("apple"), + expectedName: testhelper.EnabledOrDisabledFlag(ctx, featureflag.HeadAsDefaultBranch, + git.DefaultRef, + git.NewReferenceNameFromBranchName("apple"), + ), }, { desc: "no default branches", @@ -168,7 +175,10 @@ func testRepositoryGetDefaultBranch(t *testing.T, cfg config.Cfg, getRepository WriteCommit(t, cfg, repoPath, WithParents(oid), WithBranch("banana")) return repo }, - expectedName: git.NewReferenceNameFromBranchName("apple"), + expectedName: testhelper.EnabledOrDisabledFlag(ctx, featureflag.HeadAsDefaultBranch, + git.DefaultRef, + git.NewReferenceNameFromBranchName("apple"), + ), }, { desc: "test repo HEAD set", diff --git a/internal/git/localrepo/refs.go b/internal/git/localrepo/refs.go index 398b2950f1d86f88684b86ed01d4f3c6a13f136b..6325a49a02e9d486b8c9ebdd719a0701c96bfafa 100644 --- a/internal/git/localrepo/refs.go +++ b/internal/git/localrepo/refs.go @@ -14,6 +14,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/command" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/safe" ) @@ -326,6 +327,10 @@ func (repo *Repo) GetRemoteReferences(ctx context.Context, remote string, opts . // GetDefaultBranch determines the default branch name func (repo *Repo) GetDefaultBranch(ctx context.Context) (git.ReferenceName, error) { + if featureflag.HeadAsDefaultBranch.IsEnabled(ctx) { + return repo.headReference(ctx) + } + branches, err := repo.GetBranches(ctx) if err != nil { return "", err diff --git a/internal/git/localrepo/refs_test.go b/internal/git/localrepo/refs_test.go index c46f19d62b9e208b0185d4d8544131c9d167c764..b1aec6ffefad8d5951a5b8bfe0d5fc575def5dd6 100644 --- a/internal/git/localrepo/refs_test.go +++ b/internal/git/localrepo/refs_test.go @@ -21,6 +21,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/text" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/safe" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" @@ -488,7 +489,10 @@ func TestRepo_UpdateRef(t *testing.T) { } func TestRepo_SetDefaultBranch(t *testing.T) { - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.HeadAsDefaultBranch).Run(t, testRepoSetDefaultBranch) +} + +func testRepoSetDefaultBranch(t *testing.T, ctx context.Context) { cfg, repo, repoPath := setupRepo(t) gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master")) @@ -507,9 +511,12 @@ func TestRepo_SetDefaultBranch(t *testing.T) { expectedRef: "refs/heads/feature", }, { - desc: "unknown ref", - ref: "refs/heads/non_existent_ref", - expectedRef: git.LegacyDefaultRef, + desc: "unknown ref", + ref: "refs/heads/non_existent_ref", + expectedRef: testhelper.EnabledOrDisabledFlag(ctx, featureflag.HeadAsDefaultBranch, + git.ReferenceName("refs/heads/non_existent_ref"), + git.LegacyDefaultRef, + ), }, } for _, tc := range testCases { @@ -565,8 +572,10 @@ func (b *blockingManager) Stop(_ context.Context, _ txinfo.Transaction) error { } func TestRepo_SetDefaultBranch_errors(t *testing.T) { - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.HeadAsDefaultBranch).Run(t, testRepoSetDefaultBranchErrors) +} +func testRepoSetDefaultBranchErrors(t *testing.T, ctx context.Context) { t.Run("malformed refname", func(t *testing.T) { _, repo, _ := setupRepo(t) diff --git a/internal/gitaly/service/operations/apply_patch_test.go b/internal/gitaly/service/operations/apply_patch_test.go index 516ab189bd6c1f93dae60139c71a4436b6ac010c..1ba57d29e01f89eb690f008bc85695003d51b3c5 100644 --- a/internal/gitaly/service/operations/apply_patch_test.go +++ b/internal/gitaly/service/operations/apply_patch_test.go @@ -130,26 +130,6 @@ To restore the original branch and stop patching, run "git am --abort". {Mode: "100644", Path: "file", Content: "patch 1"}, }, }, - { - desc: "creating a new branch from the first listed branch works", - baseTree: []gittest.TreeEntry{ - {Path: "file", Mode: "100644", Content: "base-content"}, - }, - baseReference: "refs/heads/a", - extraBranches: []string{"refs/heads/b"}, - targetBranch: "new-branch", - patches: []patchDescription{ - { - newTree: []gittest.TreeEntry{ - {Path: "file", Mode: "100644", Content: "patch 1"}, - }, - }, - }, - expectedBranchCreation: true, - expectedTree: []gittest.TreeEntry{ - {Mode: "100644", Path: "file", Content: "patch 1"}, - }, - }, { desc: "multiple patches apply cleanly", baseTree: []gittest.TreeEntry{ diff --git a/internal/gitaly/testhelper_test.go b/internal/gitaly/testhelper_test.go index e56b93600dd9fd9e58c89c26314981248a345938..f92be02346f8dc0e679b8bd86c0b453970e8d544 100644 --- a/internal/gitaly/testhelper_test.go +++ b/internal/gitaly/testhelper_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "google.golang.org/protobuf/proto" ) @@ -76,5 +77,16 @@ func RequireDefaultBranch(tb testing.TB, ctx context.Context, repo *localrepo.Re actualDefaultBranch, err := repo.GetDefaultBranch(ctx) require.NoError(tb, err) + + // Often it is convenient to leave test-case assertions at their zero value + // when it isn't relevant to the test at hand. Before HeadAsDefaultBranch + // empty repositories would return an empty string as the default branch, + // with HeadAsDefaultBranch there is always a default branch. So here we + // assume when the default branch is not relevant to the test that it will + // be git.DefaultRef. + if featureflag.HeadAsDefaultBranch.IsEnabled(ctx) && expectedDefaultBranch == "" { + expectedDefaultBranch = git.DefaultRef + } + require.Equal(tb, expectedDefaultBranch, actualDefaultBranch) } diff --git a/internal/gitaly/transaction_manager_test.go b/internal/gitaly/transaction_manager_test.go index 616a013b819d8841320ad8eae9e6859ec8f2788b..da30653dcfac0e6be4cc3fc12f1f8972ca9b002b 100644 --- a/internal/gitaly/transaction_manager_test.go +++ b/internal/gitaly/transaction_manager_test.go @@ -21,6 +21,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" @@ -64,12 +65,14 @@ func validCustomHooks(tb testing.TB) []byte { func noopTransactionFinalizer() {} func TestTransactionManager(t *testing.T) { + testhelper.NewFeatureSets(featureflag.HeadAsDefaultBranch).Run(t, testTransactionManager) +} + +func testTransactionManager(t *testing.T, ctx context.Context) { umask := perm.GetUmask() t.Parallel() - ctx := testhelper.Context(t) - type testCommit struct { OID git.ObjectID } @@ -343,8 +346,11 @@ func TestTransactionManager(t *testing.T) { }, }, expectedState: StateAssertion{ - DefaultBranch: "refs/heads/parent", - References: []git.Reference{{Name: "refs/heads/parent", Target: setup.Commits.First.OID.String()}}, + DefaultBranch: testhelper.EnabledOrDisabledFlag(ctx, featureflag.HeadAsDefaultBranch, + git.DefaultRef, + git.ReferenceName("refs/heads/parent"), + ), + References: []git.Reference{{Name: "refs/heads/parent", Target: setup.Commits.First.OID.String()}}, Database: DatabaseState{ string(keyAppliedLogIndex(getRepositoryID(setup.Repository))): LogIndex(1).toProto(), }, @@ -417,8 +423,11 @@ func TestTransactionManager(t *testing.T) { }, }, expectedState: StateAssertion{ - DefaultBranch: "refs/heads/parent/child", - References: []git.Reference{{Name: "refs/heads/parent/child", Target: setup.Commits.First.OID.String()}}, + DefaultBranch: testhelper.EnabledOrDisabledFlag(ctx, featureflag.HeadAsDefaultBranch, + git.DefaultRef, + git.ReferenceName("refs/heads/parent/child"), + ), + References: []git.Reference{{Name: "refs/heads/parent/child", Target: setup.Commits.First.OID.String()}}, Database: DatabaseState{ string(keyAppliedLogIndex(getRepositoryID(setup.Repository))): LogIndex(1).toProto(), }, diff --git a/internal/metadata/featureflag/ff_head_as_default_branch.go b/internal/metadata/featureflag/ff_head_as_default_branch.go new file mode 100644 index 0000000000000000000000000000000000000000..f65777e9d15b24df2394d25efdcc00b27613e2c7 --- /dev/null +++ b/internal/metadata/featureflag/ff_head_as_default_branch.go @@ -0,0 +1,10 @@ +package featureflag + +// HeadAsDefaultBranch enables using HEAD as the only source of truth for the +// default branch. +var HeadAsDefaultBranch = NewFeatureFlag( + "head_as_default_branch", + "v15.10.0", + "", + false, +) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 0639e433af2afc0098d8065362252209c3e7bb23..c58eaac23fe6da65ee91e6e82e30280a9467f75b 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -202,6 +202,12 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context { // Randomly enable the use of the catfile cache in localrepo.ReadObject. ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.LocalrepoReadObjectCached, rnd.Int()%2 == 0) + // Default branch is often called but not explicitly tested very often. So + // to save having to change too many tests we randomly enable it when it + // shouldn't affect anything, and explicitly test enabled/disabled as + // required. + ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.HeadAsDefaultBranch, rnd.Int()%2 == 0) + for _, opt := range opts { ctx = opt(ctx) }