Replace MustRunCommand with Exec in cloneRepo

Usages of the MustRunCommand replaced with Exec
in order to break dependency on the global
config.Config variable. All dependent functions
were changed as well. The same as the call site
for them.

In order to verify the list of the parameters used to
run git binary during service method execution the git
binary path replaced and that spyGitCfg used to run the
service. But the actual git still need to be used in other
cases. It was not an issue before as MustRunCommand uses
a global config.Config variable with proper git bin path set.

Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699

14 files changed, 51 insertions, 42 deletions

diff --git a/internal/git/gittest/repo.go b/internal/git/gittest/repo.go
index 0a3f0c036..e6406fcaf 100644
--- a/internal/git/gittest/repo.go
+++ b/internal/git/gittest/repo.go
@@ -97,33 +97,33 @@ func initRepoAt(t testing.TB, cfg config.Cfg, bare bool, storage config.Storage)
 }
 
 // CloneRepoAtStorageRoot clones a new copy of test repository under a subdirectory in the storage root.
-func CloneRepoAtStorageRoot(t testing.TB, storageRoot, relativePath string) *gitalypb.Repository {
-	repo, _, _ := cloneRepo(t, storageRoot, relativePath, testRepo, true)
+func CloneRepoAtStorageRoot(t testing.TB, cfg config.Cfg, storageRoot, relativePath string) *gitalypb.Repository {
+	repo, _, _ := cloneRepo(t, cfg, storageRoot, relativePath, testRepo, true)
 	return repo
 }
 
 // CloneRepoAtStorage clones a new copy of test repository under a subdirectory in the storage root.
 func CloneRepoAtStorage(t testing.TB, storage config.Storage, relativePath string) (*gitalypb.Repository, string, testhelper.Cleanup) {
-	repo, repoPath, cleanup := cloneRepo(t, storage.Path, relativePath, testRepo, true)
+	repo, repoPath, cleanup := cloneRepo(t, config.Config, storage.Path, relativePath, testRepo, true)
 	repo.StorageName = storage.Name
 	return repo, repoPath, cleanup
 }
 
 // CloneRepo creates a bare copy of the test repository.
 func CloneRepo(t testing.TB) (repo *gitalypb.Repository, repoPath string, cleanup func()) {
-	return cloneRepo(t, testhelper.GitlabTestStoragePath(), NewRepositoryName(t, true), testRepo, true)
+	return cloneRepo(t, config.Config, testhelper.GitlabTestStoragePath(), NewRepositoryName(t, true), testRepo, true)
 }
 
 // CloneRepoWithWorktree creates a copy of the test repository with a worktree. This is allows you
 // to run normal 'non-bare' Git commands.
 func CloneRepoWithWorktree(t testing.TB) (repo *gitalypb.Repository, repoPath string, cleanup func()) {
-	return cloneRepo(t, testhelper.GitlabTestStoragePath(), NewRepositoryName(t, false), testRepo, false)
+	return cloneRepo(t, config.Config, testhelper.GitlabTestStoragePath(), NewRepositoryName(t, false), testRepo, false)
 }
 
 // CloneRepoWithWorktreeAtStorage creates a copy of the test repository with a worktree at the storage you want.
 // This is allows you to run normal 'non-bare' Git commands.
-func CloneRepoWithWorktreeAtStorage(t testing.TB, storage config.Storage) (*gitalypb.Repository, string, testhelper.Cleanup) {
-	repo, repoPath, cleanup := cloneRepo(t, storage.Path, NewRepositoryName(t, false), testRepo, false)
+func CloneRepoWithWorktreeAtStorage(t testing.TB, cfg config.Cfg, storage config.Storage) (*gitalypb.Repository, string, testhelper.Cleanup) {
+	repo, repoPath, cleanup := cloneRepo(t, cfg, storage.Path, NewRepositoryName(t, false), testRepo, false)
 	repo.StorageName = storage.Name
 	return repo, repoPath, cleanup
 }
@@ -156,7 +156,7 @@ func isValidRepoPath(absolutePath string) bool {
 	return true
 }
 
-func cloneRepo(t testing.TB, storageRoot, relativePath, repoName string, bare bool) (repo *gitalypb.Repository, repoPath string, cleanup func()) {
+func cloneRepo(t testing.TB, cfg config.Cfg, storageRoot, relativePath, repoName string, bare bool) (repo *gitalypb.Repository, repoPath string, cleanup func()) {
 	repoPath = filepath.Join(storageRoot, relativePath)
 
 	repo = InitRepoDir(t, storageRoot, relativePath)
@@ -168,14 +168,14 @@ func cloneRepo(t testing.TB, storageRoot, relativePath, repoName string, bare bo
 		repo.RelativePath = filepath.Join(relativePath, ".git")
 	}
 
-	testhelper.MustRunCommand(t, nil, "git", append(args, testRepositoryPath(t, repoName), repoPath)...)
+	Exec(t, cfg, append(args, testRepositoryPath(t, repoName), repoPath)...)
 
 	return repo, repoPath, func() { require.NoError(t, os.RemoveAll(repoPath)) }
 }
 
 // CloneBenchRepo creates a bare copy of the benchmarking test repository.
-func CloneBenchRepo(t testing.TB) (repo *gitalypb.Repository, repoPath string, cleanup func()) {
-	return cloneRepo(t, testhelper.GitlabTestStoragePath(), NewRepositoryName(t, true),
+func CloneBenchRepo(t testing.TB, cfg config.Cfg) (repo *gitalypb.Repository, repoPath string, cleanup func()) {
+	return cloneRepo(t, cfg, testhelper.GitlabTestStoragePath(), NewRepositoryName(t, true),
 		"benchmark.git", true)
 }
 
diff --git a/internal/gitaly/service/blob/lfs_pointers_test.go b/internal/gitaly/service/blob/lfs_pointers_test.go
index 09404f330..5d15e3c6e 100644
--- a/internal/gitaly/service/blob/lfs_pointers_test.go
+++ b/internal/gitaly/service/blob/lfs_pointers_test.go
@@ -382,7 +382,7 @@ func TestSuccessfulGetNewLFSPointersRequest(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	repo, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	repo, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	t.Cleanup(cleanup)
 
 	revision := []byte("46abbb087fcc0fd02c340f0f2f052bd2c7708da3")
@@ -790,7 +790,7 @@ func BenchmarkFindLFSPointers(b *testing.B) {
 
 	gitCmdFactory := git.NewExecCommandFactory(cfg)
 
-	repoProto, _, cleanup := gittest.CloneBenchRepo(b)
+	repoProto, _, cleanup := gittest.CloneBenchRepo(b, cfg)
 	b.Cleanup(cleanup)
 	repo := localrepo.New(gitCmdFactory, repoProto, cfg)
 
@@ -814,7 +814,7 @@ func BenchmarkReadLFSPointers(b *testing.B) {
 
 	gitCmdFactory := git.NewExecCommandFactory(cfg)
 
-	repoProto, path, cleanup := gittest.CloneBenchRepo(b)
+	repoProto, path, cleanup := gittest.CloneBenchRepo(b, cfg)
 	b.Cleanup(cleanup)
 	repo := localrepo.New(gitCmdFactory, repoProto, cfg)
 
diff --git a/internal/gitaly/service/commit/testhelper_test.go b/internal/gitaly/service/commit/testhelper_test.go
index 91cf07f35..4a05fc6eb 100644
--- a/internal/gitaly/service/commit/testhelper_test.go
+++ b/internal/gitaly/service/commit/testhelper_test.go
@@ -45,7 +45,7 @@ func setupCommitServiceWithRepo(
 		if bare {
 			return gittest.CloneRepoAtStorage(tb, cfg.Storages[0], t.Name())
 		}
-		return gittest.CloneRepoWithWorktreeAtStorage(tb, cfg.Storages[0])
+		return gittest.CloneRepoWithWorktreeAtStorage(tb, cfg, cfg.Storages[0])
 	})
 }
 
diff --git a/internal/gitaly/service/conflicts/testhelper_test.go b/internal/gitaly/service/conflicts/testhelper_test.go
index 91b10109f..266f55666 100644
--- a/internal/gitaly/service/conflicts/testhelper_test.go
+++ b/internal/gitaly/service/conflicts/testhelper_test.go
@@ -57,7 +57,7 @@ func SetupConflictsService(t testing.TB, bare bool) (config.Cfg, *gitalypb.Repos
 		repo, repoPath, cleanup = gittest.CloneRepoAtStorage(t, cfg.Storages[0], t.Name())
 		t.Cleanup(cleanup)
 	} else {
-		repo, repoPath, cleanup = gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+		repo, repoPath, cleanup = gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 		t.Cleanup(cleanup)
 	}
 
diff --git a/internal/gitaly/service/internalgitaly/walkrepos_test.go b/internal/gitaly/service/internalgitaly/walkrepos_test.go
index a55886217..4a022876f 100644
--- a/internal/gitaly/service/internalgitaly/walkrepos_test.go
+++ b/internal/gitaly/service/internalgitaly/walkrepos_test.go
@@ -11,6 +11,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -35,16 +36,15 @@ func (w *streamWrapper) Send(resp *gitalypb.WalkReposResponse) error {
 }
 
 func TestWalkRepos(t *testing.T) {
-	testRoot := testhelper.TempDir(t)
-
-	storageName := "default"
-	storageRoot := filepath.Join(testRoot, "storage")
+	cfg := testcfg.Build(t)
+	storageName := cfg.Storages[0].Name
+	storageRoot := cfg.Storages[0].Path
 
 	// file walk happens lexicographically, so we delete repository in the middle
 	// of the seqeuence to ensure the walk proceeds normally
-	testRepo1 := gittest.CloneRepoAtStorageRoot(t, storageRoot, "a")
-	deletedRepo := gittest.CloneRepoAtStorageRoot(t, storageRoot, "b")
-	testRepo2 := gittest.CloneRepoAtStorageRoot(t, storageRoot, "c")
+	testRepo1 := gittest.CloneRepoAtStorageRoot(t, cfg, storageRoot, "a")
+	deletedRepo := gittest.CloneRepoAtStorageRoot(t, cfg, storageRoot, "b")
+	testRepo2 := gittest.CloneRepoAtStorageRoot(t, cfg, storageRoot, "c")
 
 	// to test a directory being deleted during a walk, we must delete a directory after
 	// the file walk has started. To achieve that, we wrap the server to pass down a wrapped
diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go
index 7724acefb..ee0b19ded 100644
--- a/internal/gitaly/service/operations/rebase_test.go
+++ b/internal/gitaly/service/operations/rebase_test.go
@@ -511,7 +511,7 @@ func testRebaseRequestWithDeletedFile(t *testing.T, cfg config.Cfg, rubySrv *rub
 
 func testRebaseRequestWithDeletedFileFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
 	ctx, cfg, _, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
-	repoProto, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	repoProto, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	t.Cleanup(cleanup)
 
 	repo := localrepo.New(git.NewExecCommandFactory(cfg), repoProto, cfg)
@@ -567,7 +567,7 @@ func testRebaseOntoRemoteBranchFeatured(t *testing.T, ctx context.Context, cfg c
 
 	repo := localrepo.New(git.NewExecCommandFactory(cfg), repoProto, cfg)
 
-	remoteRepo, remoteRepoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	remoteRepo, remoteRepoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	defer cleanup()
 
 	localBranch := "master"
diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go
index da6b6ff3f..b4e287a55 100644
--- a/internal/gitaly/service/operations/squash_test.go
+++ b/internal/gitaly/service/operations/squash_test.go
@@ -216,7 +216,7 @@ func TestSquashRequestWithRenamedFiles(t *testing.T) {
 
 	ctx, cfg, _, _, client := setupOperationsService(t, ctx)
 
-	repoProto, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	repoProto, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	t.Cleanup(cleanup)
 
 	repo := localrepo.New(git.NewExecCommandFactory(cfg), repoProto, cfg)
diff --git a/internal/gitaly/service/ref/refs_test.go b/internal/gitaly/service/ref/refs_test.go
index cb2d9df0a..48787dd82 100644
--- a/internal/gitaly/service/ref/refs_test.go
+++ b/internal/gitaly/service/ref/refs_test.go
@@ -405,7 +405,7 @@ func TestInvalidRepoFindDefaultBranchNameRequest(t *testing.T) {
 func TestSuccessfulFindAllTagsRequest(t *testing.T) {
 	cfg, client := setupRefServiceWithoutRepo(t)
 
-	repoProto, repoPath, cleanupFn := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	repoProto, repoPath, cleanupFn := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	defer cleanupFn()
 	repo := localrepo.New(git.NewExecCommandFactory(cfg), repoProto, cfg)
 
@@ -603,7 +603,7 @@ func TestSuccessfulFindAllTagsRequest(t *testing.T) {
 func TestFindAllTagNestedTags(t *testing.T) {
 	cfg, client := setupRefServiceWithoutRepo(t)
 
-	testRepoCopy, testRepoCopyPath, cleanupFn := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	testRepoCopy, testRepoCopyPath, cleanupFn := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	defer cleanupFn()
 
 	blobID := "faaf198af3a36dbf41961466703cc1d47c61d051"
@@ -1446,7 +1446,7 @@ func TestSuccessfulFindTagRequest(t *testing.T) {
 func TestFindTagNestedTag(t *testing.T) {
 	cfg, client := setupRefServiceWithoutRepo(t)
 
-	repo, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	repo, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	t.Cleanup(cleanup)
 
 	blobID := "faaf198af3a36dbf41961466703cc1d47c61d051"
diff --git a/internal/gitaly/service/repository/archive_test.go b/internal/gitaly/service/repository/archive_test.go
index aec83ffec..d56fdc7c5 100644
--- a/internal/gitaly/service/repository/archive_test.go
+++ b/internal/gitaly/service/repository/archive_test.go
@@ -482,16 +482,22 @@ env | grep -E "^GL_|CORRELATION|GITALY_"`))
 	require.NoError(t, err)
 	require.NoError(t, tmpFile.Close())
 
-	cfg := testcfg.Build(t, testcfg.WithBase(config.Cfg{Git: config.Git{BinPath: tmpFile.Name()}}))
+	cfg := testcfg.Build(t)
 
 	testhelper.ConfigureGitalyHooksBin(t, cfg)
 
-	serverSocketPath := runRepositoryServerWithConfig(t, cfg, nil)
+	// We re-define path to the git executable to catch parameters used to call it.
+	// This replacement only needs to be done for the configuration used to invoke git commands.
+	// Other operations should use actual path to the git binary to work properly.
+	spyGitCfg := cfg
+	spyGitCfg.Git.BinPath = tmpFile.Name()
+
+	serverSocketPath := runRepositoryServerWithConfig(t, spyGitCfg, nil)
 	cfg.SocketPath = serverSocketPath
 
 	client := newRepositoryClient(t, cfg, serverSocketPath)
 
-	repo, _, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	repo, _, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	t.Cleanup(cleanup)
 
 	commitID := "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863"
diff --git a/internal/gitaly/service/repository/fetch_remote_test.go b/internal/gitaly/service/repository/fetch_remote_test.go
index f27a20bd6..6fc605386 100644
--- a/internal/gitaly/service/repository/fetch_remote_test.go
+++ b/internal/gitaly/service/repository/fetch_remote_test.go
@@ -95,12 +95,15 @@ func TestFetchRemote_sshCommand(t *testing.T) {
 	exit 7`, outputPath)
 	testhelper.WriteExecutable(t, gitPath, []byte(script))
 
-	cfg, repo, _ := testcfg.BuildWithRepo(t, testcfg.WithBase(config.Cfg{
-		Git: config.Git{BinPath: gitPath},
-	}))
+	cfg, repo, _ := testcfg.BuildWithRepo(t)
 
-	client, serverSocketPath := runRepositoryService(t, cfg, nil)
-	cfg.SocketPath = serverSocketPath
+	// We re-define path to the git executable to catch parameters used to call it.
+	// This replacement only needs to be done for the configuration used to invoke git commands.
+	// Other operations should use actual path to the git binary to work properly.
+	spyGitCfg := cfg
+	spyGitCfg.Git.BinPath = gitPath
+
+	client, _ := runRepositoryService(t, spyGitCfg, nil)
 
 	ctx, cancel := testhelper.Context()
 	defer cancel()
diff --git a/internal/gitaly/service/repository/snapshot_test.go b/internal/gitaly/service/repository/snapshot_test.go
index 6a4f8e372..f3c94dabb 100644
--- a/internal/gitaly/service/repository/snapshot_test.go
+++ b/internal/gitaly/service/repository/snapshot_test.go
@@ -166,7 +166,7 @@ func TestGetSnapshotWithDedupe(t *testing.T) {
 func TestGetSnapshotWithDedupeSoftFailures(t *testing.T) {
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
-	testRepo, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	testRepo, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	defer cleanup()
 
 	locator := config.NewLocator(cfg)
diff --git a/internal/gitaly/service/repository/testhelper_test.go b/internal/gitaly/service/repository/testhelper_test.go
index d2a85c4ec..983373d29 100644
--- a/internal/gitaly/service/repository/testhelper_test.go
+++ b/internal/gitaly/service/repository/testhelper_test.go
@@ -157,7 +157,7 @@ func setupRepositoryServiceWithoutRepo(t testing.TB, opts ...testserver.GitalySe
 func setupRepositoryServiceWithWorktree(t testing.TB) (config.Cfg, *gitalypb.Repository, string, gitalypb.RepositoryServiceClient) {
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
-	repo, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	repo, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	t.Cleanup(cleanup)
 
 	return cfg, repo, repoPath, client
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index c0dd80465..555fb28c9 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -216,7 +216,7 @@ type pushData struct {
 }
 
 func newTestPush(t *testing.T, cfg config.Cfg, fileContents []byte) *pushData {
-	_, repoPath, localCleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	_, repoPath, localCleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	defer localCleanup()
 
 	oldHead, newHead := createCommit(t, repoPath, fileContents)
@@ -306,7 +306,7 @@ func TestFailedReceivePackRequestDueToValidationError(t *testing.T) {
 func TestInvalidTimezone(t *testing.T) {
 	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
 
-	_, localRepoPath, localCleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
+	_, localRepoPath, localCleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	defer localCleanup()
 
 	head := text.ChompBytes(testhelper.MustRunCommand(t, nil, "git", "-C", localRepoPath, "rev-parse", "HEAD"))
diff --git a/internal/testhelper/testcfg/gitaly_builder.go b/internal/testhelper/testcfg/gitaly_builder.go
index ed2553730..1f48df715 100644
--- a/internal/testhelper/testcfg/gitaly_builder.go
+++ b/internal/testhelper/testcfg/gitaly_builder.go
@@ -137,7 +137,7 @@ func (gc *GitalyCfgBuilder) BuildWithRepoAt(t testing.TB, relativePath string) (
 	// clone the test repo to the each storage
 	repos := make([]*gitalypb.Repository, len(cfg.Storages))
 	for i, gitalyStorage := range cfg.Storages {
-		repos[i] = gittest.CloneRepoAtStorageRoot(t, gitalyStorage.Path, relativePath)
+		repos[i] = gittest.CloneRepoAtStorageRoot(t, cfg, gitalyStorage.Path, relativePath)
 		repos[i].StorageName = gitalyStorage.Name
 	}