Merge branch 'pks-git-pick-same-cgroups-more-often' into 'master'

git: Pick same cgroups more often

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5025

Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: James Fargher <proglottis@gmail.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

17 files changed, 122 insertions, 227 deletions

diff --git a/cmd/gitaly-ssh/upload_pack_test.go b/cmd/gitaly-ssh/upload_pack_test.go
index 92f562960..b9cb51945 100644
--- a/cmd/gitaly-ssh/upload_pack_test.go
+++ b/cmd/gitaly-ssh/upload_pack_test.go
@@ -3,7 +3,6 @@
 package main
 
 import (
-	"bytes"
 	"fmt"
 	"os"
 	"testing"
@@ -40,7 +39,6 @@ func TestVisibilityOfHiddenRefs(t *testing.T) {
 	existingSha := git.ObjectID("1e292f8fedd741b75372e19097c76d327140c312")
 	keepAroundRef := fmt.Sprintf("%s/%s", keepAroundNamespace, existingSha)
 
-	gitCmdFactory := gittest.NewCommandFactory(t, cfg)
 	localRepo := localrepo.NewTestRepo(t, cfg, repo)
 	updater, err := updateref.New(ctx, localRepo)
 
@@ -81,31 +79,20 @@ func TestVisibilityOfHiddenRefs(t *testing.T) {
 
 			require.NoError(t, err)
 
-			env := []string{
-				fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
-				fmt.Sprintf("GITALY_ADDRESS=unix:%s", socketPath),
-				fmt.Sprintf("GITALY_WD=%s", wd),
-				fmt.Sprintf("PATH=.:%s", os.Getenv("PATH")),
-				fmt.Sprintf("GIT_SSH_COMMAND=%s upload-pack", cfg.BinaryPath("gitaly-ssh")),
-			}
-
-			stdout := &bytes.Buffer{}
-			cmd, err := gitCmdFactory.NewWithoutRepo(ctx, git.SubCmd{
-				Name: "ls-remote",
-				Args: []string{
-					fmt.Sprintf("%s:%s", "git@localhost", repoPath),
-					keepAroundRef,
+			stdout := gittest.ExecOpts(t, cfg, gittest.ExecConfig{
+				Env: []string{
+					fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
+					fmt.Sprintf("GITALY_ADDRESS=unix:%s", socketPath),
+					fmt.Sprintf("GITALY_WD=%s", wd),
+					fmt.Sprintf("PATH=.:%s", os.Getenv("PATH")),
+					fmt.Sprintf("GIT_SSH_COMMAND=%s upload-pack", cfg.BinaryPath("gitaly-ssh")),
 				},
-			}, git.WithEnv(env...), git.WithStdout(stdout))
-			require.NoError(t, err)
-
-			err = cmd.Wait()
-			require.NoError(t, err)
+			}, "ls-remote", fmt.Sprintf("%s:%s", "git@localhost", repoPath), keepAroundRef)
 
 			if test.HiddenRefFound {
-				require.Equal(t, fmt.Sprintf("%s\t%s\n", existingSha, keepAroundRef), stdout.String())
+				require.Equal(t, fmt.Sprintf("%s\t%s\n", existingSha, keepAroundRef), string(stdout))
 			} else {
-				require.NotEqual(t, fmt.Sprintf("%s\t%s\n", existingSha, keepAroundRef), stdout.String())
+				require.NotEqual(t, fmt.Sprintf("%s\t%s\n", existingSha, keepAroundRef), string(stdout))
 			}
 		})
 	}
diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index 3822377cc..a6f0810e7 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -25,8 +25,6 @@ type CommandFactory interface {
 	New(ctx context.Context, repo repository.GitRepo, sc Cmd, opts ...CmdOpt) (*command.Command, error)
 	// NewWithoutRepo creates a command without a target repository.
 	NewWithoutRepo(ctx context.Context, sc Cmd, opts ...CmdOpt) (*command.Command, error)
-	// NewWithDir creates a command without a target repository that would be executed in dir directory.
-	NewWithDir(ctx context.Context, dir string, sc Cmd, opts ...CmdOpt) (*command.Command, error)
 	// GetExecutionEnvironment returns parameters required to execute Git commands.
 	GetExecutionEnvironment(context.Context) ExecutionEnvironment
 	// HooksPath returns the path where Gitaly's Git hooks reside.
@@ -41,8 +39,9 @@ type CommandFactory interface {
 }
 
 type execCommandFactoryConfig struct {
-	hooksPath     string
-	gitBinaryPath string
+	hooksPath      string
+	gitBinaryPath  string
+	cgroupsManager cgroups.Manager
 }
 
 // ExecCommandFactoryOption is an option that can be passed to NewExecCommandFactory.
@@ -69,6 +68,13 @@ func WithGitBinaryPath(path string) ExecCommandFactoryOption {
 	}
 }
 
+// WithCgroupsManager overrides the Cgroups manager used by the command factory.
+func WithCgroupsManager(cgroupsManager cgroups.Manager) ExecCommandFactoryOption {
+	return func(cfg *execCommandFactoryConfig) {
+		cfg.cgroupsManager = cgroupsManager
+	}
+}
+
 type hookDirectories struct {
 	tempHooksPath string
 }
@@ -124,11 +130,16 @@ func NewExecCommandFactory(cfg config.Cfg, opts ...ExecCommandFactoryOption) (_
 	}
 	cleanups = append(cleanups, cleanup)
 
+	cgroupsManager := factoryCfg.cgroupsManager
+	if cgroupsManager == nil {
+		cgroupsManager = cgroups.NewManager(cfg.Cgroups, os.Getpid())
+	}
+
 	gitCmdFactory := &ExecCommandFactory{
 		cfg:            cfg,
 		execEnvs:       execEnvs,
 		locator:        config.NewLocator(cfg),
-		cgroupsManager: cgroups.NewManager(cfg.Cgroups, os.Getpid()),
+		cgroupsManager: cgroupsManager,
 		invalidCommandsMetric: prometheus.NewCounterVec(
 			prometheus.CounterOpts{
 				Name: "gitaly_invalid_commands_total",
@@ -227,23 +238,12 @@ func (cf *ExecCommandFactory) Collect(metrics chan<- prometheus.Metric) {
 
 // New creates a new command for the repo repository.
 func (cf *ExecCommandFactory) New(ctx context.Context, repo repository.GitRepo, sc Cmd, opts ...CmdOpt) (*command.Command, error) {
-	return cf.newCommand(ctx, repo, "", sc, opts...)
+	return cf.newCommand(ctx, repo, sc, opts...)
 }
 
 // NewWithoutRepo creates a command without a target repository.
 func (cf *ExecCommandFactory) NewWithoutRepo(ctx context.Context, sc Cmd, opts ...CmdOpt) (*command.Command, error) {
-	return cf.newCommand(ctx, nil, "", sc, opts...)
-}
-
-// NewWithDir creates a new command.Command whose working directory is set
-// to dir. Arguments are validated before the command is being run. It is
-// invalid to use an empty directory.
-func (cf *ExecCommandFactory) NewWithDir(ctx context.Context, dir string, sc Cmd, opts ...CmdOpt) (*command.Command, error) {
-	if dir == "" {
-		return nil, errors.New("no 'dir' provided")
-	}
-
-	return cf.newCommand(ctx, nil, dir, sc, opts...)
+	return cf.newCommand(ctx, nil, sc, opts...)
 }
 
 // GetExecutionEnvironment returns parameters required to execute Git commands.
@@ -374,7 +374,7 @@ func (cf *ExecCommandFactory) GitVersion(ctx context.Context) (Version, error) {
 // command will be run in the context of that repository. Note that this sets up arguments and
 // environment variables for git, but doesn't run in the directory itself. If a directory
 // is given, then the command will be run in that directory.
-func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.GitRepo, dir string, sc Cmd, opts ...CmdOpt) (*command.Command, error) {
+func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.GitRepo, sc Cmd, opts ...CmdOpt) (*command.Command, error) {
 	config, err := cf.combineOpts(ctx, sc, opts)
 	if err != nil {
 		return nil, err
@@ -387,13 +387,20 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.Gi
 
 	env := config.env
 
+	var repoPath string
 	if repo != nil {
-		repoPath, err := cf.locator.GetRepoPath(repo)
+		var err error
+		repoPath, err = cf.locator.GetRepoPath(repo)
 		if err != nil {
 			return nil, err
 		}
 
 		env = append(alternates.Env(repoPath, repo.GetGitObjectDirectory(), repo.GetGitAlternateObjectDirectories()), env...)
+	}
+
+	if config.worktreePath != "" {
+		args = append([]string{"-C", config.worktreePath}, args...)
+	} else if repoPath != "" {
 		args = append([]string{"--git-dir", repoPath}, args...)
 	}
 
@@ -408,7 +415,6 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.Gi
 
 	command, err := command.New(ctx, append([]string{execEnv.BinaryPath}, args...), append(
 		config.commandOpts,
-		command.WithDir(dir),
 		command.WithEnvironment(env),
 		command.WithCommandName("git", sc.Subcommand()),
 		command.WithCgroup(cf.cgroupsManager, repo),
diff --git a/internal/git/command_factory_cgroup_test.go b/internal/git/command_factory_cgroup_test.go
index 93ce9724f..cf3ebbc72 100644
--- a/internal/git/command_factory_cgroup_test.go
+++ b/internal/git/command_factory_cgroup_test.go
@@ -1,21 +1,19 @@
 //go:build !gitaly_test_sha256
 
-package git
+package git_test
 
 import (
-	"os"
-	"path/filepath"
 	"testing"
 
 	"github.com/prometheus/client_golang/prometheus"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/cgroups"
 	"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"
 )
 
 type mockCgroupsManager struct {
@@ -39,30 +37,16 @@ func (m *mockCgroupsManager) Collect(ch chan<- prometheus.Metric) {}
 func (m *mockCgroupsManager) Describe(ch chan<- *prometheus.Desc) {}
 
 func TestNewCommandAddsToCgroup(t *testing.T) {
-	root := testhelper.TempDir(t)
+	t.Parallel()
 
-	cfg := config.Cfg{
-		BinDir:     filepath.Join(root, "bin.d"),
-		SocketPath: "/path/to/socket",
-		Git: config.Git{
-			IgnoreGitconfig: true,
-		},
-		Cgroups: cgroups.Config{
-			Repositories: cgroups.Repositories{
-				Count: 1,
-			},
-		},
-		Storages: []config.Storage{{
-			Name: "storage-1",
-			Path: root,
-		}},
-	}
+	ctx := testhelper.Context(t)
+	cfg := testcfg.Build(t)
 
-	require.NoError(t, os.MkdirAll(cfg.BinDir, 0o755))
+	repo, _ := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
+		SkipCreationViaService: true,
+	})
 
-	gitCmdFactory := newCommandFactory(t, cfg, WithSkipHooks())
-
-	testCases := []struct {
+	for _, tc := range []struct {
 		desc      string
 		cgroupsFF bool
 	}{
@@ -74,28 +58,25 @@ func TestNewCommandAddsToCgroup(t *testing.T) {
 			desc:      "cgroups feature flag off",
 			cgroupsFF: false,
 		},
-	}
-
-	for _, tc := range testCases {
+	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			dir := testhelper.TempDir(t)
+			ctx := featureflag.IncomingCtxWithFeatureFlag(ctx, featureflag.RunCommandsInCGroup, tc.cgroupsFF)
 
 			var manager mockCgroupsManager
-			gitCmdFactory.cgroupsManager = &manager
-			ctx := testhelper.Context(t)
-
-			ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, featureflag.RunCommandsInCGroup, tc.cgroupsFF)
-
-			cmd := SubCmd{
-				Name: "hash-object",
-			}
-
-			_, err := gitCmdFactory.NewWithDir(ctx, dir, &cmd)
+			gitCmdFactory := gittest.NewCommandFactory(t, cfg, git.WithCgroupsManager(&manager))
+
+			cmd, err := gitCmdFactory.New(ctx, repo, &git.SubCmd{
+				Name: "rev-parse",
+				Flags: []git.Option{
+					git.Flag{Name: "--is-bare-repository"},
+				},
+			})
 			require.NoError(t, err)
+			require.NoError(t, cmd.Wait())
 
 			if tc.cgroupsFF {
 				require.Len(t, manager.commands, 1)
-				assert.Contains(t, manager.commands[0].Args(), "hash-object")
+				require.Contains(t, manager.commands[0].Args(), "rev-parse")
 				return
 			}
 
diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go
index 45a2b8e2e..26b570564 100644
--- a/internal/git/command_factory_test.go
+++ b/internal/git/command_factory_test.go
@@ -97,56 +97,6 @@ func TestExecCommandFactory_globalGitConfigIgnored(t *testing.T) {
 	}
 }
 
-func TestExecCommandFactory_NewWithDir(t *testing.T) {
-	cfg := testcfg.Build(t)
-
-	gitCmdFactory, cleanup, err := git.NewExecCommandFactory(cfg)
-	require.NoError(t, err)
-	defer cleanup()
-
-	t.Run("no dir specified", func(t *testing.T) {
-		ctx := testhelper.Context(t)
-
-		_, err := gitCmdFactory.NewWithDir(ctx, "", nil, nil)
-		require.Error(t, err)
-		require.Contains(t, err.Error(), "no 'dir' provided")
-	})
-
-	t.Run("runs in dir", func(t *testing.T) {
-		repoPath := testhelper.TempDir(t)
-
-		gittest.Exec(t, cfg, "init", repoPath)
-		gittest.Exec(t, cfg, "-C", repoPath, "commit", "--allow-empty", "-m", "initial commit")
-		ctx := testhelper.Context(t)
-
-		var stderr bytes.Buffer
-		cmd, err := gitCmdFactory.NewWithDir(ctx, repoPath, git.SubCmd{
-			Name: "rev-parse",
-			Args: []string{"master"},
-		}, git.WithStderr(&stderr))
-		require.NoError(t, err)
-
-		revData, err := io.ReadAll(cmd)
-		require.NoError(t, err)
-
-		require.NoError(t, cmd.Wait(), stderr.String())
-
-		require.Equal(t, "99ed180822d96f70810847eba6d0d168c582258d", text.ChompBytes(revData))
-	})
-
-	t.Run("doesn't runs in non existing dir", func(t *testing.T) {
-		ctx := testhelper.Context(t)
-
-		var stderr bytes.Buffer
-		_, err := gitCmdFactory.NewWithDir(ctx, "non-existing-dir", git.SubCmd{
-			Name: "rev-parse",
-			Args: []string{"master"},
-		}, git.WithStderr(&stderr))
-		require.Error(t, err)
-		require.Contains(t, err.Error(), "no such file or directory")
-	})
-}
-
 func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 	testhelper.Unsetenv(t, "GITALY_TESTING_GIT_BINARY")
 	testhelper.Unsetenv(t, "GITALY_TESTING_BUNDLED_GIT_PATH")
diff --git a/internal/git/command_options.go b/internal/git/command_options.go
index c74b6c1e3..3c882bd12 100644
--- a/internal/git/command_options.go
+++ b/internal/git/command_options.go
@@ -170,6 +170,7 @@ type cmdCfg struct {
 	globals         []GlobalOption
 	commandOpts     []command.Option
 	hooksConfigured bool
+	worktreePath    string
 }
 
 // CmdOpt is an option for running a command
@@ -320,3 +321,11 @@ func WithFinalizer(finalizer func(*command.Command)) CmdOpt {
 		return nil
 	}
 }
+
+// WithWorktree sets up the Git command to run in the given worktree path by using the `-C` switch.
+func WithWorktree(worktreePath string) CmdOpt {
+	return func(_ context.Context, _ config.Cfg, _ CommandFactory, c *cmdCfg) error {
+		c.worktreePath = worktreePath
+		return nil
+	}
+}
diff --git a/internal/git/gittest/intercepting_command_factory.go b/internal/git/gittest/intercepting_command_factory.go
index dd9cc5e46..d4fed3a86 100644
--- a/internal/git/gittest/intercepting_command_factory.go
+++ b/internal/git/gittest/intercepting_command_factory.go
@@ -119,13 +119,6 @@ func (f *InterceptingCommandFactory) NewWithoutRepo(ctx context.Context, sc git.
 	)...)
 }
 
-// NewWithDir creates a new Git command in the given directory using the intercepting script.
-func (f *InterceptingCommandFactory) NewWithDir(ctx context.Context, dir string, sc git.Cmd, opts ...git.CmdOpt) (*command.Command, error) {
-	return f.interceptingCommandFactory.NewWithDir(ctx, dir, sc, append(
-		opts, git.WithEnv(f.realCommandFactory.GetExecutionEnvironment(ctx).EnvironmentVariables...),
-	)...)
-}
-
 // GetExecutionEnvironment returns the execution environment of the intercetping command factory.
 // The Git binary path will point to the intercepting script, while environment variables will
 // point to the intercepted Git installation.
diff --git a/internal/git/gittest/intercepting_command_factory_test.go b/internal/git/gittest/intercepting_command_factory_test.go
index b6f332dbc..a8a8a5fd9 100644
--- a/internal/git/gittest/intercepting_command_factory_test.go
+++ b/internal/git/gittest/intercepting_command_factory_test.go
@@ -33,17 +33,6 @@ func TestInterceptingCommandFactory(t *testing.T) {
 		require.Equal(t, expectedString, stdout.String())
 	})
 
-	t.Run("NewWithDir", func(t *testing.T) {
-		var stdout bytes.Buffer
-		cmd, err := factory.NewWithDir(ctx, repoPath, git.SubCmd{
-			Name: "rev-parse",
-			Args: []string{"something"},
-		}, git.WithStdout(&stdout))
-		require.NoError(t, err)
-		require.NoError(t, cmd.Wait())
-		require.Equal(t, expectedString, stdout.String())
-	})
-
 	t.Run("NewWithoutRepo", func(t *testing.T) {
 		var stdout bytes.Buffer
 		cmd, err := factory.NewWithoutRepo(ctx, git.SubCmd{
diff --git a/internal/gitaly/service/operations/apply_patch.go b/internal/gitaly/service/operations/apply_patch.go
index 0c053280f..e2419ab43 100644
--- a/internal/gitaly/service/operations/apply_patch.go
+++ b/internal/gitaly/service/operations/apply_patch.go
@@ -111,7 +111,7 @@ func (s *Server) userApplyPatch(ctx context.Context, header *gitalypb.UserApplyP
 	}()
 
 	var stdout, stderr bytes.Buffer
-	cmd, err := s.gitCmdFactory.NewWithDir(ctx, worktreePath,
+	if err := repo.ExecAndWait(ctx,
 		git.SubCmd{
 			Name: "am",
 			Flags: []git.Option{
@@ -131,12 +131,8 @@ func (s *Server) userApplyPatch(ctx context.Context, header *gitalypb.UserApplyP
 		git.WithStdout(&stdout),
 		git.WithStderr(&stderr),
 		git.WithRefTxHook(header.Repository),
-	)
-	if err != nil {
-		return fmt.Errorf("create git am: %w", err)
-	}
-
-	if err := cmd.Wait(); err != nil {
+		git.WithWorktree(worktreePath),
+	); err != nil {
 		// The Ruby implementation doesn't include stderr in errors, which makes
 		// it difficult to determine the cause of an error. This special cases the
 		// user facing patching error which is returned usually to maintain test
@@ -150,7 +146,7 @@ func (s *Server) userApplyPatch(ctx context.Context, header *gitalypb.UserApplyP
 	}
 
 	var revParseStdout, revParseStderr bytes.Buffer
-	revParseCmd, err := s.gitCmdFactory.NewWithDir(ctx, worktreePath,
+	if err := repo.ExecAndWait(ctx,
 		git.SubCmd{
 			Name: "rev-parse",
 			Flags: []git.Option{
@@ -161,12 +157,8 @@ func (s *Server) userApplyPatch(ctx context.Context, header *gitalypb.UserApplyP
 		},
 		git.WithStdout(&revParseStdout),
 		git.WithStderr(&revParseStderr),
-	)
-	if err != nil {
-		return fmt.Errorf("create git rev-parse: %w", gitError{ErrMsg: revParseStderr.String(), Err: err})
-	}
-
-	if err := revParseCmd.Wait(); err != nil {
+		git.WithWorktree(worktreePath),
+	); err != nil {
 		return fmt.Errorf("get patched commit: %w", gitError{ErrMsg: revParseStderr.String(), Err: err})
 	}
 
diff --git a/internal/gitaly/service/repository/redirecting_test_server_test.go b/internal/gitaly/service/repository/redirecting_test_server_test.go
index 2b3fe9e95..08125a366 100644
--- a/internal/gitaly/service/repository/redirecting_test_server_test.go
+++ b/internal/gitaly/service/repository/redirecting_test_server_test.go
@@ -9,7 +9,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
@@ -46,21 +45,11 @@ func TestRedirectingServerRedirects(t *testing.T) {
 	dir := testhelper.TempDir(t)
 
 	httpServerState, redirectingServer := StartRedirectingTestServer()
-	ctx := testhelper.Context(t)
 
 	var stderr bytes.Buffer
-	cmd, err := gittest.NewCommandFactory(t, cfg).NewWithoutRepo(ctx, git.SubCmd{
-		Name: "clone",
-		Flags: []git.Option{
-			git.Flag{Name: "--bare"},
-		},
-		Args: []string{
-			redirectingServer.URL, dir,
-		},
-	}, git.WithConfig(git.ConfigPair{Key: "http.followRedirects", Value: "true"}), git.WithDisabledHooks(), git.WithStderr(&stderr))
-	require.NoError(t, err)
-
-	require.Error(t, cmd.Wait())
+	cloneCmd := gittest.NewCommand(t, cfg, "clone", "--bare", redirectingServer.URL, dir)
+	cloneCmd.Stderr = &stderr
+	require.Error(t, cloneCmd.Run())
 	require.Contains(t, stderr.String(), "unable to update url base from redirection")
 
 	redirectingServer.Close()
diff --git a/internal/gitaly/service/smarthttp/inforefs.go b/internal/gitaly/service/smarthttp/inforefs.go
index 8799f517f..431429eb0 100644
--- a/internal/gitaly/service/smarthttp/inforefs.go
+++ b/internal/gitaly/service/smarthttp/inforefs.go
@@ -70,7 +70,7 @@ func (s *server) handleInfoRefs(ctx context.Context, service, repoPath string, r
 	}
 	cmdOpts = append(cmdOpts, git.WithConfig(config...))
 
-	cmd, err := s.gitCmdFactory.NewWithoutRepo(ctx, git.SubCmd{
+	cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(), git.SubCmd{
 		Name:  service,
 		Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}, git.Flag{Name: "--advertise-refs"}},
 		Args:  []string{repoPath},
diff --git a/internal/gitaly/service/smarthttp/receive_pack.go b/internal/gitaly/service/smarthttp/receive_pack.go
index f7c1e123e..5fed6f6b0 100644
--- a/internal/gitaly/service/smarthttp/receive_pack.go
+++ b/internal/gitaly/service/smarthttp/receive_pack.go
@@ -50,7 +50,7 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac
 		return err
 	}
 
-	cmd, err := s.gitCmdFactory.NewWithoutRepo(ctx,
+	cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(),
 		git.SubCmd{
 			Name:  "receive-pack",
 			Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}},
diff --git a/internal/gitaly/service/smarthttp/upload_pack.go b/internal/gitaly/service/smarthttp/upload_pack.go
index 2fd4956dc..83076ee47 100644
--- a/internal/gitaly/service/smarthttp/upload_pack.go
+++ b/internal/gitaly/service/smarthttp/upload_pack.go
@@ -142,7 +142,7 @@ func (s *server) runUploadPack(ctx context.Context, req basicPostUploadPackReque
 		git.WithPackObjectsHookEnv(req.GetRepository(), "http"),
 	}
 
-	cmd, err := s.gitCmdFactory.NewWithoutRepo(ctx, git.SubCmd{
+	cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(), git.SubCmd{
 		Name:  "upload-pack",
 		Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}},
 		Args:  []string{repoPath},
diff --git a/internal/gitaly/service/ssh/monitor_stdin_command.go b/internal/gitaly/service/ssh/monitor_stdin_command.go
index dbed02dc3..f3bf43a70 100644
--- a/internal/gitaly/service/ssh/monitor_stdin_command.go
+++ b/internal/gitaly/service/ssh/monitor_stdin_command.go
@@ -8,15 +8,24 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/pktline"
+	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
 
-func monitorStdinCommand(ctx context.Context, gitCmdFactory git.CommandFactory, stdin io.Reader, stdout, stderr io.Writer, sc git.SubCmd, opts ...git.CmdOpt) (*command.Command, *pktline.ReadMonitor, error) {
+func monitorStdinCommand(
+	ctx context.Context,
+	gitCmdFactory git.CommandFactory,
+	repo *gitalypb.Repository,
+	stdin io.Reader,
+	stdout, stderr io.Writer,
+	sc git.SubCmd,
+	opts ...git.CmdOpt,
+) (*command.Command, *pktline.ReadMonitor, error) {
 	stdinPipe, monitor, cleanup, err := pktline.NewReadMonitor(ctx, stdin)
 	if err != nil {
 		return nil, nil, fmt.Errorf("create monitor: %w", err)
 	}
 
-	cmd, err := gitCmdFactory.NewWithoutRepo(ctx, sc, append([]git.CmdOpt{
+	cmd, err := gitCmdFactory.New(ctx, repo, sc, append([]git.CmdOpt{
 		git.WithStdin(stdinPipe),
 		git.WithStdout(stdout),
 		git.WithStderr(stderr),
diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go
index c8d0657ba..4f2a84988 100644
--- a/internal/gitaly/service/ssh/receive_pack.go
+++ b/internal/gitaly/service/ssh/receive_pack.go
@@ -76,7 +76,7 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer,
 		return err
 	}
 
-	cmd, err := s.gitCmdFactory.NewWithoutRepo(ctx,
+	cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(),
 		git.SubCmd{
 			Name: "receive-pack",
 			Args: []string{repoPath},
diff --git a/internal/gitaly/service/ssh/upload_archive.go b/internal/gitaly/service/ssh/upload_archive.go
index 72b094311..273975437 100644
--- a/internal/gitaly/service/ssh/upload_archive.go
+++ b/internal/gitaly/service/ssh/upload_archive.go
@@ -53,7 +53,7 @@ func (s *server) sshUploadArchive(stream gitalypb.SSHService_SSHUploadArchiveSer
 		return stream.Send(&gitalypb.SSHUploadArchiveResponse{Stderr: p})
 	})
 
-	cmd, monitor, err := monitorStdinCommand(ctx, s.gitCmdFactory, stdin, stdout, stderr, git.SubCmd{
+	cmd, monitor, err := monitorStdinCommand(ctx, s.gitCmdFactory, req.GetRepository(), stdin, stdout, stderr, git.SubCmd{
 		Name: "upload-archive",
 		Args: []string{repoPath},
 	})
diff --git a/internal/gitaly/service/ssh/upload_pack.go b/internal/gitaly/service/ssh/upload_pack.go
index 05e06cf3e..5313aaf16 100644
--- a/internal/gitaly/service/ssh/upload_pack.go
+++ b/internal/gitaly/service/ssh/upload_pack.go
@@ -128,7 +128,7 @@ func (s *server) sshUploadPack(rpcContext context.Context, req sshUploadPackRequ
 	var stderrBuilder strings.Builder
 	stderr = io.MultiWriter(stderr, &stderrBuilder)
 
-	cmd, monitor, err := monitorStdinCommand(ctx, s.gitCmdFactory, stdin, stdout, stderr, git.SubCmd{
+	cmd, monitor, err := monitorStdinCommand(ctx, s.gitCmdFactory, repo, stdin, stdout, stderr, git.SubCmd{
 		Name: "upload-pack",
 		Args: []string{repoPath},
 	}, commandOpts...)
diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go
index aeb0e7695..4c2c45472 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -51,8 +51,8 @@ func runClone(
 	ctx context.Context,
 	cfg config.Cfg,
 	withSidechannel bool,
-	cloneCmd git.Cmd,
 	request *gitalypb.SSHUploadPackRequest,
+	args ...string,
 ) error {
 	payload, err := protojson.Marshal(request)
 	require.NoError(t, err)
@@ -62,23 +62,21 @@ func runClone(
 		flagsWithValues = append(flagsWithValues, flag.FormatWithValue(value))
 	}
 
-	env := []string{
+	var output bytes.Buffer
+	cloneCmd := gittest.NewCommand(t, cfg, append([]string{"clone"}, args...)...)
+	cloneCmd.Stdout = &output
+	cloneCmd.Stderr = &output
+	cloneCmd.Env = append(cloneCmd.Env,
 		fmt.Sprintf("GITALY_ADDRESS=%s", cfg.SocketPath),
 		fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
 		fmt.Sprintf("GITALY_FEATUREFLAGS=%s", strings.Join(flagsWithValues, ",")),
 		fmt.Sprintf(`GIT_SSH_COMMAND=%s upload-pack`, cfg.BinaryPath("gitaly-ssh")),
-	}
+	)
 	if withSidechannel {
-		env = append(env, "GITALY_USE_SIDECHANNEL=1")
+		cloneCmd.Env = append(cloneCmd.Env, "GITALY_USE_SIDECHANNEL=1")
 	}
 
-	var output bytes.Buffer
-	gitCommand, err := gittest.NewCommandFactory(t, cfg).NewWithoutRepo(ctx,
-		cloneCmd, git.WithStdout(&output), git.WithStderr(&output), git.WithEnv(env...), git.WithDisabledHooks(),
-	)
-	require.NoError(t, err)
-
-	if err := gitCommand.Wait(); err != nil {
+	if err := cloneCmd.Run(); err != nil {
 		return fmt.Errorf("Failed to run `git clone`: %q", output.Bytes())
 	}
 
@@ -537,7 +535,7 @@ func testUploadPackSuccessful(t *testing.T, sidechannel bool, opts ...testcfg.Op
 	for _, tc := range []struct {
 		desc             string
 		request          *gitalypb.SSHUploadPackRequest
-		cloneFlags       []git.Option
+		cloneArgs        []string
 		deepen           float64
 		verify           func(t *testing.T, localRepoPath string)
 		expectedProtocol string
@@ -561,8 +559,8 @@ func testUploadPackSuccessful(t *testing.T, sidechannel bool, opts ...testcfg.Op
 			request: &gitalypb.SSHUploadPackRequest{
 				Repository: repo,
 			},
-			cloneFlags: []git.Option{
-				git.ValueFlag{Name: "--depth", Value: "1"},
+			cloneArgs: []string{
+				"--depth=1",
 			},
 			deepen: 1,
 		},
@@ -572,8 +570,8 @@ func testUploadPackSuccessful(t *testing.T, sidechannel bool, opts ...testcfg.Op
 				Repository:  repo,
 				GitProtocol: git.ProtocolV2,
 			},
-			cloneFlags: []git.Option{
-				git.ValueFlag{Name: "--depth", Value: "1"},
+			cloneArgs: []string{
+				"--depth=1",
 			},
 			deepen:           1,
 			expectedProtocol: git.ProtocolV2,
@@ -583,8 +581,8 @@ func testUploadPackSuccessful(t *testing.T, sidechannel bool, opts ...testcfg.Op
 			request: &gitalypb.SSHUploadPackRequest{
 				Repository: repo,
 			},
-			cloneFlags: []git.Option{
-				git.ValueFlag{Name: "--filter", Value: "blob:limit=1024"},
+			cloneArgs: []string{
+				"--filter=blob:limit=1024",
 			},
 			verify: func(t *testing.T, repoPath string) {
 				gittest.RequireObjectNotExists(t, cfg, repoPath, largeBlobID)
@@ -593,8 +591,8 @@ func testUploadPackSuccessful(t *testing.T, sidechannel bool, opts ...testcfg.Op
 		},
 		{
 			desc: "hidden tags",
-			cloneFlags: []git.Option{
-				git.Flag{Name: "--mirror"},
+			cloneArgs: []string{
+				"--mirror",
 			},
 			request: &gitalypb.SSHUploadPackRequest{
 				Repository: repo,
@@ -619,11 +617,11 @@ func testUploadPackSuccessful(t *testing.T, sidechannel bool, opts ...testcfg.Op
 			negotiationMetrics.Reset()
 			protocolDetectingFactory.Reset(t)
 
-			require.NoError(t, runClone(t, ctx, cfg, sidechannel, git.SubCmd{
-				Name:  "clone",
-				Args:  []string{"git@localhost:test/test.git", localRepoPath},
-				Flags: tc.cloneFlags,
-			}, tc.request))
+			require.NoError(t, runClone(t, ctx, cfg, sidechannel, tc.request,
+				append([]string{
+					"git@localhost:test/test.git", localRepoPath,
+				}, tc.cloneArgs...)...,
+			))
 
 			requireRevisionsEqual(t, cfg, repoPath, localRepoPath, "refs/heads/main")
 
@@ -679,12 +677,9 @@ func TestUploadPack_packObjectsHook(t *testing.T) {
 
 	localRepoPath := testhelper.TempDir(t)
 
-	err := runClone(t, ctx, cfg, false, git.SubCmd{
-		Name: "clone", Args: []string{"git@localhost:test/test.git", localRepoPath},
-	}, &gitalypb.SSHUploadPackRequest{
+	require.NoError(t, runClone(t, ctx, cfg, false, &gitalypb.SSHUploadPackRequest{
 		Repository: repo,
-	})
-	require.NoError(t, err)
+	}, "git@localhost:test/test.git", localRepoPath))
 
 	require.Equal(t, []byte("I was invoked\n"), testhelper.MustReadFile(t, outputPath))
 }
@@ -756,17 +751,12 @@ func TestUploadPack_invalidStorage(t *testing.T) {
 
 	localRepoPath := testhelper.TempDir(t)
 
-	err := runClone(t, ctx, cfg, false, git.SubCmd{
-		Name: "clone",
-		Args: []string{
-			"git@localhost:test/test.git", localRepoPath,
-		},
-	}, &gitalypb.SSHUploadPackRequest{
+	err := runClone(t, ctx, cfg, false, &gitalypb.SSHUploadPackRequest{
 		Repository: &gitalypb.Repository{
 			StorageName:  "foobar",
 			RelativePath: repo.GetRelativePath(),
 		},
-	})
+	}, "git@localhost:test/test.git", localRepoPath)
 	require.Error(t, err)
 
 	if testhelper.IsPraefectEnabled() {