From 4bae171f032574ef57fc583c5543410633868015 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 7 Nov 2022 07:39:05 +0100 Subject: gittest: Convert `NewRepositoryName()` to always create ".git" suffix The `NewRepositoryName()` function computes a random repository hash for the caller. This function has two different modes: one to create a bare repository path with ".git" suffix, and one non-bare one without it. There is basically not a single instance though where we'd want this function to create a repository path without the ".git" suffix. In production systems we always have that suffix as we only ever have bare repositories. Which means that tests which do use a non-bare repository path don't match our typical hashed storage layout. This is something we should avoid though: it's preferable to have our test data as close to production data as possible. In the preceding commits we have already converted all remaining callers that create non-bare repository paths to stop doing that. Let's thus drop the parameter and unconditionally create bare repository paths so that noone can be tempted to create a non-bare one anymore. --- internal/backup/backup_test.go | 6 +++--- internal/git/gittest/repo.go | 13 ++++--------- internal/git/housekeeping/object_pool_test.go | 2 +- internal/git/housekeeping/testhelper_test.go | 2 +- internal/gitaly/service/repository/create_fork_test.go | 6 +++--- .../repository/create_repository_from_bundle_test.go | 2 +- .../repository/create_repository_from_snapshot_test.go | 6 +++--- .../service/repository/create_repository_from_url_test.go | 2 +- .../gitaly/service/repository/create_repository_test.go | 2 +- internal/gitaly/service/repository/remove_test.go | 2 +- internal/gitaly/service/repository/snapshot_test.go | 2 +- internal/gitaly/service/repository/util_test.go | 2 +- internal/praefect/coordinator_test.go | 2 +- internal/praefect/router_per_repository_test.go | 2 +- 14 files changed, 23 insertions(+), 28 deletions(-) diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index 6eb0a150c..5184a99a7 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -81,7 +81,7 @@ func TestManager_Create(t *testing.T) { setup: func(tb testing.TB) (*gitalypb.Repository, string) { emptyRepo, repoPath := gittest.CreateRepository(tb, ctx, cfg) nonexistentRepo := proto.Clone(emptyRepo).(*gitalypb.Repository) - nonexistentRepo.RelativePath = gittest.NewRepositoryName(t, true) + nonexistentRepo.RelativePath = gittest.NewRepositoryName(t) return nonexistentRepo, repoPath }, createsBundle: false, @@ -286,7 +286,7 @@ func testManagerRestore(t *testing.T, ctx context.Context) { repo := &gitalypb.Repository{ StorageName: "default", - RelativePath: gittest.NewRepositoryName(tb, true), + RelativePath: gittest.NewRepositoryName(tb), } _, err := repoClient.CreateRepository(ctx, &gitalypb.CreateRepositoryRequest{Repository: repo}) @@ -378,7 +378,7 @@ func testManagerRestore(t *testing.T, ctx context.Context) { setup: func(tb testing.TB) (*gitalypb.Repository, *git.Checksum) { repo := &gitalypb.Repository{ StorageName: "default", - RelativePath: gittest.NewRepositoryName(tb, true), + RelativePath: gittest.NewRepositoryName(tb), } relativePath := stripRelativePath(tb, repo) diff --git a/internal/git/gittest/repo.go b/internal/git/gittest/repo.go index 162183d54..6250f9cc5 100644 --- a/internal/git/gittest/repo.go +++ b/internal/git/gittest/repo.go @@ -60,14 +60,9 @@ func NewObjectPoolName(tb testing.TB) string { } // NewRepositoryName returns a random repository hash -// in format '@hashed/[0-9a-f]{2}/[0-9a-f]{2}/[0-9a-f]{64}(.git)?'. -func NewRepositoryName(tb testing.TB, bare bool) string { - suffix := "" - if bare { - suffix = ".git" - } - - return filepath.Join("@hashed", newDiskHash(tb)+suffix) +// in format '@hashed/[0-9a-f]{2}/[0-9a-f]{2}/[0-9a-f]{64}.git'. +func NewRepositoryName(tb testing.TB) string { + return filepath.Join("@hashed", newDiskHash(tb)+".git") } // newDiskHash generates a random directory path following the Rails app's @@ -136,7 +131,7 @@ func CreateRepository(tb testing.TB, ctx context.Context, cfg config.Cfg, config storage = opts.Storage } - relativePath := NewRepositoryName(tb, true) + relativePath := NewRepositoryName(tb) if opts.RelativePath != "" { relativePath = opts.RelativePath } diff --git a/internal/git/housekeeping/object_pool_test.go b/internal/git/housekeeping/object_pool_test.go index eabbabc62..f534ab7ee 100644 --- a/internal/git/housekeeping/object_pool_test.go +++ b/internal/git/housekeeping/object_pool_test.go @@ -52,7 +52,7 @@ func TestIsPoolRepository(t *testing.T) { { desc: "normal repos dont match", repo: &gitalypb.Repository{ - RelativePath: "@hashed/" + gittest.NewRepositoryName(t, true), + RelativePath: "@hashed/" + gittest.NewRepositoryName(t), }, }, } { diff --git a/internal/git/housekeeping/testhelper_test.go b/internal/git/housekeeping/testhelper_test.go index ccb0e1a28..49499923f 100644 --- a/internal/git/housekeeping/testhelper_test.go +++ b/internal/git/housekeeping/testhelper_test.go @@ -15,7 +15,7 @@ func testRepoAndPool(t *testing.T, desc string, testFunc func(t *testing.T, rela t.Helper() t.Run(desc, func(t *testing.T) { t.Run("normal repository", func(t *testing.T) { - testFunc(t, gittest.NewRepositoryName(t, true)) + testFunc(t, gittest.NewRepositoryName(t)) }) t.Run("object pool", func(t *testing.T) { diff --git a/internal/gitaly/service/repository/create_fork_test.go b/internal/gitaly/service/repository/create_fork_test.go index 9542d24f1..e26f140c1 100644 --- a/internal/gitaly/service/repository/create_fork_test.go +++ b/internal/gitaly/service/repository/create_fork_test.go @@ -82,7 +82,7 @@ func TestCreateFork_successful(t *testing.T) { ctx = testhelper.MergeOutgoingMetadata(ctx, testcfg.GitalyServersMetadataFromCfg(t, cfg)) forkedRepo := &gitalypb.Repository{ - RelativePath: gittest.NewRepositoryName(t, true), + RelativePath: gittest.NewRepositoryName(t), StorageName: repo.GetStorageName(), } @@ -133,7 +133,7 @@ func TestCreateFork_refs(t *testing.T) { ctx = testhelper.MergeOutgoingMetadata(ctx, testcfg.GitalyServersMetadataFromCfg(t, cfg)) targetRepo := &gitalypb.Repository{ - RelativePath: gittest.NewRepositoryName(t, true), + RelativePath: gittest.NewRepositoryName(t), StorageName: sourceRepo.GetStorageName(), } @@ -191,7 +191,7 @@ func TestCreateFork_fsck(t *testing.T) { ) forkedRepo := &gitalypb.Repository{ - RelativePath: gittest.NewRepositoryName(t, true), + RelativePath: gittest.NewRepositoryName(t), StorageName: repo.GetStorageName(), } diff --git a/internal/gitaly/service/repository/create_repository_from_bundle_test.go b/internal/gitaly/service/repository/create_repository_from_bundle_test.go index 3de17233b..f8ff51178 100644 --- a/internal/gitaly/service/repository/create_repository_from_bundle_test.go +++ b/internal/gitaly/service/repository/create_repository_from_bundle_test.go @@ -132,7 +132,7 @@ func TestCreateRepositoryFromBundle_transactional(t *testing.T) { createdRepo := &gitalypb.Repository{ StorageName: repoProto.GetStorageName(), - RelativePath: gittest.NewRepositoryName(t, true), + RelativePath: gittest.NewRepositoryName(t), } require.NoError(t, stream.Send(&gitalypb.CreateRepositoryFromBundleRequest{ diff --git a/internal/gitaly/service/repository/create_repository_from_snapshot_test.go b/internal/gitaly/service/repository/create_repository_from_snapshot_test.go index 8c16bf0cd..c519661e6 100644 --- a/internal/gitaly/service/repository/create_repository_from_snapshot_test.go +++ b/internal/gitaly/service/repository/create_repository_from_snapshot_test.go @@ -161,7 +161,7 @@ func TestCreateRepositoryFromSnapshot_badURL(t *testing.T) { req := &gitalypb.CreateRepositoryFromSnapshotRequest{ Repository: &gitalypb.Repository{ StorageName: cfg.Storages[0].Name, - RelativePath: gittest.NewRepositoryName(t, true), + RelativePath: gittest.NewRepositoryName(t), }, HttpUrl: "invalid!scheme://invalid.invalid", } @@ -227,7 +227,7 @@ func TestCreateRepositoryFromSnapshot_invalidArguments(t *testing.T) { req := &gitalypb.CreateRepositoryFromSnapshotRequest{ Repository: &gitalypb.Repository{ StorageName: cfg.Storages[0].Name, - RelativePath: gittest.NewRepositoryName(t, true), + RelativePath: gittest.NewRepositoryName(t), }, HttpUrl: srv.URL + tc.url, HttpAuth: tc.auth, @@ -304,7 +304,7 @@ func TestCreateRepositoryFromSnapshot_resolvedAddressSuccess(t *testing.T) { srv := httptest.NewServer(&tarTesthandler{tarData: bytes.NewReader(data), secret: secret, host: host}) defer srv.Close() - repoRelativePath := gittest.NewRepositoryName(t, true) + repoRelativePath := gittest.NewRepositoryName(t) repo := &gitalypb.Repository{ StorageName: cfg.Storages[0].Name, diff --git a/internal/gitaly/service/repository/create_repository_from_url_test.go b/internal/gitaly/service/repository/create_repository_from_url_test.go index cc3aaed26..711325db2 100644 --- a/internal/gitaly/service/repository/create_repository_from_url_test.go +++ b/internal/gitaly/service/repository/create_repository_from_url_test.go @@ -207,7 +207,7 @@ func TestCreateRepositoryFromURL_fsck(t *testing.T) { ) targetRepoProto := &gitalypb.Repository{ - RelativePath: gittest.NewRepositoryName(t, true), + RelativePath: gittest.NewRepositoryName(t), StorageName: cfg.Storages[0].Name, } diff --git a/internal/gitaly/service/repository/create_repository_test.go b/internal/gitaly/service/repository/create_repository_test.go index 40a51873a..6894eed3c 100644 --- a/internal/gitaly/service/repository/create_repository_test.go +++ b/internal/gitaly/service/repository/create_repository_test.go @@ -106,7 +106,7 @@ func TestCreateRepository_withDefaultBranch(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - repo := &gitalypb.Repository{StorageName: cfg.Storages[0].Name, RelativePath: gittest.NewRepositoryName(t, true)} + repo := &gitalypb.Repository{StorageName: cfg.Storages[0].Name, RelativePath: gittest.NewRepositoryName(t)} req := &gitalypb.CreateRepositoryRequest{Repository: repo, DefaultBranch: []byte(tc.defaultBranch)} _, err := client.CreateRepository(ctx, req) diff --git a/internal/gitaly/service/repository/remove_test.go b/internal/gitaly/service/repository/remove_test.go index 4c94f2cd2..2ef7689a3 100644 --- a/internal/gitaly/service/repository/remove_test.go +++ b/internal/gitaly/service/repository/remove_test.go @@ -23,7 +23,7 @@ func TestRemoveRepository(t *testing.T) { repo := &gitalypb.Repository{ StorageName: cfg.Storages[0].Name, - RelativePath: gittest.NewRepositoryName(t, true), + RelativePath: gittest.NewRepositoryName(t), } _, err := client.CreateRepository(ctx, &gitalypb.CreateRepositoryRequest{ diff --git a/internal/gitaly/service/repository/snapshot_test.go b/internal/gitaly/service/repository/snapshot_test.go index dab52c629..ed36fdf5b 100644 --- a/internal/gitaly/service/repository/snapshot_test.go +++ b/internal/gitaly/service/repository/snapshot_test.go @@ -254,7 +254,7 @@ func copyRepoUsingSnapshot(t *testing.T, ctx context.Context, cfg config.Cfg, cl repoCopy := &gitalypb.Repository{ StorageName: cfg.Storages[0].Name, - RelativePath: gittest.NewRepositoryName(t, true), + RelativePath: gittest.NewRepositoryName(t), } createRepoReq := &gitalypb.CreateRepositoryFromSnapshotRequest{ diff --git a/internal/gitaly/service/repository/util_test.go b/internal/gitaly/service/repository/util_test.go index 8991ecad8..346d7c64e 100644 --- a/internal/gitaly/service/repository/util_test.go +++ b/internal/gitaly/service/repository/util_test.go @@ -258,7 +258,7 @@ func TestCreateRepository(t *testing.T) { t.Run(tc.desc, func(t *testing.T) { repo := &gitalypb.Repository{ StorageName: cfg.Storages[0].Name, - RelativePath: gittest.NewRepositoryName(t, true), + RelativePath: gittest.NewRepositoryName(t), } if tc.transactional { diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go index ec726b1b5..c15f4b59e 100644 --- a/internal/praefect/coordinator_test.go +++ b/internal/praefect/coordinator_test.go @@ -789,7 +789,7 @@ func TestStreamDirector_maintenanceRPCs(t *testing.T) { repository := &gitalypb.Repository{ StorageName: "default", - RelativePath: gittest.NewRepositoryName(t, true), + RelativePath: gittest.NewRepositoryName(t), } primaryRepository := &gitalypb.Repository{ StorageName: primaryStorage, diff --git a/internal/praefect/router_per_repository_test.go b/internal/praefect/router_per_repository_test.go index 905b56531..fa60d9e33 100644 --- a/internal/praefect/router_per_repository_test.go +++ b/internal/praefect/router_per_repository_test.go @@ -459,7 +459,7 @@ func TestPerRepositoryRouter_RouteRepositoryMaintenance(t *testing.T) { var ( virtualStorage = "virtual-storage-1" - relativePath = gittest.NewRepositoryName(t, true) + relativePath = gittest.NewRepositoryName(t) ) configuredStorages := []string{"primary", "secondary-1", "secondary-2"} -- cgit v1.2.3