diff --git a/internal/gitaly/repoutil/create.go b/internal/gitaly/repoutil/create.go index d097c21c019f7f87253ab111dac519f92f206635..c1c517a968e0e684f65340658113339aa3910d56 100644 --- a/internal/gitaly/repoutil/create.go +++ b/internal/gitaly/repoutil/create.go @@ -9,14 +9,11 @@ import ( "io/fs" "os" "path/filepath" - "runtime/trace" "time" "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" - "gitlab.com/gitlab-org/gitaly/v16/internal/git/housekeeping" - housekeepingcfg "gitlab.com/gitlab-org/gitaly/v16/internal/git/housekeeping/config" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/git/stats" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" @@ -294,23 +291,6 @@ func Create( repoCounter.Increment(repository) if tx := storage.ExtractTransaction(ctx); tx != nil { - // Git allows writing unreachable objects into the repository that are missing their dependencies. The reachable - // ones are checked through connectivity checks but unreachable ones are not. - // - // Transactions rely on a property that all objects in the repository have all of their dependencies met. This allows - // us to skip full connectivity checks, and simply check that the immediate dependencies of the newly written objects - // are satisfied. Repository creations are used in various contexts and not all of them guarantee this property. Perform - // a full repack to drop all unreachable objects. This way we're certain all of the objects committed through a repository - // creation have their dependencies satisified. Ideally we would only perform a connectivity check of the new objects, - // and record the dependencies that must exist in the repository already. Repository creations should generally include - // all objects so the rewriting should not be needed. Issue: https://gitlab.com/gitlab-org/gitaly/-/issues/5969 - if err := performFullRepack(ctx, localrepo.New(logger, locator, gitCmdFactory, catfileCache, &gitalypb.Repository{ - StorageName: repository.GetStorageName(), - RelativePath: repository.GetRelativePath(), - })); err != nil { - return fmt.Errorf("perform full repack: %w", err) - } - originalRelativePath, err := filepath.Rel(tx.FS().Root(), targetPath) if err != nil { return fmt.Errorf("original relative path: %w", err) @@ -329,26 +309,6 @@ func Create( return nil } -// performFullRepack performs a full repack and drops all unreachable objects. -func performFullRepack(ctx context.Context, repo *localrepo.Repo) (returnedErr error) { - defer trace.StartRegion(ctx, "packObjects").End() - - if err := housekeeping.PerformRepack(ctx, repo, - housekeepingcfg.RepackObjectsConfig{}, - // Do a full repack. By using `-a` instead of `-A` we will immediately discard unreachable - // objects instead of exploding them into loose objects. - gitcmd.Flag{Name: "-a"}, - // Don't include objects part of alternate. - gitcmd.Flag{Name: "-l"}, - // Delete loose objects made redundant by this repack and redundant packfiles. - gitcmd.Flag{Name: "-d"}, - ); err != nil { - return fmt.Errorf("perform repack: %w", err) - } - - return nil -} - func writeRefs( ctx context.Context, w io.Writer, diff --git a/internal/gitaly/service/repository/create_fork_test.go b/internal/gitaly/service/repository/create_fork_test.go index 2b58184f8dc130f18c5995e7ad412e99ac73f03a..ea24823643aa8f20ef2a14e2f0e55bc0af83c07c 100644 --- a/internal/gitaly/service/repository/create_fork_test.go +++ b/internal/gitaly/service/repository/create_fork_test.go @@ -107,6 +107,7 @@ func TestCreateFork_revision(t *testing.T) { revision []byte expectedRefs []git.ReferenceName expectedErr string + packRefs bool }{ { name: "all branches", @@ -133,6 +134,18 @@ func TestCreateFork_revision(t *testing.T) { revision: []byte(ambiguousRef), expectedErr: "checking whether HEAD reference is sane: exit status 128", }, + { + name: "all branches with packed refs", + revision: nil, + expectedRefs: []git.ReferenceName{mainRef, wipRef, "refs/tags/v1.0.0"}, + packRefs: true, + }, + { + name: "single revision with packed refs", + revision: []byte(wipRef), + expectedRefs: []git.ReferenceName{wipRef}, + packRefs: true, + }, } { t.Run(tt.name, func(t *testing.T) { repo, repoPath := gittest.CreateRepository(t, ctx, cfg) @@ -151,6 +164,10 @@ func TestCreateFork_revision(t *testing.T) { require.NoError(t, err) } + if tt.packRefs { + gittest.Exec(t, cfg, "-C", repoPath, "pack-refs", "--all") + } + forkedRepo := &gitalypb.Repository{ RelativePath: gittest.NewRepositoryName(t), StorageName: repo.GetStorageName(), diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_alternate_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_alternate_test.go index 26d8c1d3339e6dfee2d546d07e000af354012a28..4352d0ea5f218c3595c22d74c1b8260a4b257e81 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_alternate_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_alternate_test.go @@ -87,13 +87,11 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, }, "member": { Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, Alternate: "../../pool/objects", @@ -174,16 +172,8 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, }, ), - Packfiles: &PackfilesState{ - Packfiles: []*PackfileState{ - { - Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, - setup.Commits.First.OID, - }, - HasReverseIndex: true, - }, - }, + Objects: []git.ObjectID{ + setup.Commits.First.OID, }, }, "member": { @@ -216,19 +206,9 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, }, ), - Packfiles: &PackfilesState{ - PooledObjects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, - setup.Commits.First.OID, - }, - Packfiles: []*PackfileState{ - { - Objects: []git.ObjectID{ - setup.Commits.Second.OID, - }, - HasReverseIndex: true, - }, - }, + Objects: []git.ObjectID{ + setup.Commits.First.OID, + setup.Commits.Second.OID, }, Alternate: "../../pool/objects", }, @@ -313,13 +293,11 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, }, "member": { Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, Alternate: "../../pool/objects", @@ -417,37 +395,17 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, }, ), - Packfiles: &PackfilesState{ - LooseObjects: []git.ObjectID{ - setup.Commits.Second.OID, - }, - Packfiles: []*PackfileState{ - { - Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, - setup.Commits.First.OID, - }, - HasReverseIndex: true, - }, - }, + Objects: []git.ObjectID{ + setup.Commits.First.OID, + setup.Commits.Second.OID, }, }, "member": { // The objects should have been copied over to the repository when it was // disconnected from the alternate. - Packfiles: &PackfilesState{ - LooseObjects: []git.ObjectID{ - setup.Commits.Second.OID, - }, - Packfiles: []*PackfileState{ - { - Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, - setup.Commits.First.OID, - }, - HasReverseIndex: true, - }, - }, + Objects: []git.ObjectID{ + setup.Commits.First.OID, + setup.Commits.Second.OID, }, }, }, @@ -569,19 +527,9 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, }, ), - Packfiles: &PackfilesState{ - LooseObjects: []git.ObjectID{ - setup.Commits.Second.OID, - }, - Packfiles: []*PackfileState{ - { - Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, - setup.Commits.First.OID, - }, - HasReverseIndex: true, - }, - }, + Objects: []git.ObjectID{ + setup.Commits.First.OID, + setup.Commits.Second.OID, }, }, "member": { @@ -622,19 +570,9 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact ), // The objects should have been copied over to the repository when it was // disconnected from the alternate. - Packfiles: &PackfilesState{ - LooseObjects: []git.ObjectID{ - setup.Commits.Second.OID, - }, - Packfiles: []*PackfileState{ - { - Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, - setup.Commits.First.OID, - }, - HasReverseIndex: true, - }, - }, + Objects: []git.ObjectID{ + setup.Commits.First.OID, + setup.Commits.Second.OID, }, }, }, @@ -1039,7 +977,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, }, @@ -1080,7 +1017,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, Alternate: "../../pool/objects", @@ -1170,7 +1106,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, }, @@ -1291,14 +1226,12 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, }, "member": { DefaultBranch: "refs/heads/main", Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, Alternate: "../../pool/objects", @@ -1347,13 +1280,11 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, }, "member": { Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, Alternate: "../../pool/objects", @@ -1468,7 +1399,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, CustomHooks: testhelper.DirectoryState{ @@ -1513,7 +1443,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, setup.Commits.Third.OID, @@ -1522,7 +1451,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact "repository-3": { DefaultBranch: "refs/heads/main", Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, setup.Commits.Third.OID, @@ -1575,7 +1503,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, CustomHooks: testhelper.DirectoryState{ @@ -1620,7 +1547,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, setup.Commits.Third.OID, @@ -1628,7 +1554,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, "repository-3": { Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, setup.Commits.Third.OID, @@ -1722,7 +1647,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, }, @@ -1758,7 +1682,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, }, @@ -1808,7 +1731,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, }, @@ -1844,7 +1766,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, }, @@ -1934,7 +1855,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, }, @@ -1970,7 +1890,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, }, @@ -2020,7 +1939,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, }, @@ -2056,7 +1974,6 @@ func generateAlternateTests(t *testing.T, setup testTransactionSetup) []transact }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, }, 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 42f413f69cdccebc346edbc78130af5d229dee5b..0227192ec0cbbb7b0209019b36b2d82cb15429fc 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping_test.go @@ -1179,7 +1179,6 @@ func generateHousekeepingPackRefsTests(t *testing.T, ctx context.Context, testPa }, "member": { Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, Alternate: "../../pool/objects", 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 ee1c32cd974ae450b896511a5e0cc30154196722..b9d87e46eb22a4bbdb38018913d16bd2d1334ce5 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_repo_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_repo_test.go @@ -140,7 +140,6 @@ func generateCreateRepositoryTests(t *testing.T, setup testTransactionSetup) []t }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, }, @@ -245,7 +244,6 @@ func generateCreateRepositoryTests(t *testing.T, setup testTransactionSetup) []t }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, CustomHooks: testhelper.DirectoryState{ @@ -301,7 +299,6 @@ func generateCreateRepositoryTests(t *testing.T, setup testTransactionSetup) []t }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, }, @@ -353,7 +350,6 @@ func generateCreateRepositoryTests(t *testing.T, setup testTransactionSetup) []t }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, }, @@ -481,7 +477,6 @@ func generateCreateRepositoryTests(t *testing.T, setup testTransactionSetup) []t }, Objects: []git.ObjectID{ setup.Commits.First.OID, - setup.ObjectHash.EmptyTreeOID, }, }, }, @@ -579,7 +574,6 @@ func generateCreateRepositoryTests(t *testing.T, setup testTransactionSetup) []t }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, }, CustomHooks: testhelper.DirectoryState{ @@ -624,7 +618,6 @@ func generateCreateRepositoryTests(t *testing.T, setup testTransactionSetup) []t }, ), Objects: []git.ObjectID{ - setup.ObjectHash.EmptyTreeOID, setup.Commits.First.OID, setup.Commits.Second.OID, setup.Commits.Third.OID,