diff options
author | Toon Claes <toon@gitlab.com> | 2022-11-09 14:03:09 +0300 |
---|---|---|
committer | Toon Claes <toon@gitlab.com> | 2022-11-09 14:03:09 +0300 |
commit | 6b198af0c271cccede1e7d8114367be81faa33b9 (patch) | |
tree | db14c02355806aeecc34cb65aa627ac475a7f153 | |
parent | 2a4b8505acc10b6d4c89d1c6c7b319c2ddadedfc (diff) | |
parent | 19593012e0beec38efce71407aa92fc9cddea775 (diff) |
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>
-rw-r--r-- | cmd/gitaly-ssh/upload_pack_test.go | 33 | ||||
-rw-r--r-- | internal/git/command_factory.go | 48 | ||||
-rw-r--r-- | internal/git/command_factory_cgroup_test.go | 65 | ||||
-rw-r--r-- | internal/git/command_factory_test.go | 50 | ||||
-rw-r--r-- | internal/git/command_options.go | 9 | ||||
-rw-r--r-- | internal/git/gittest/intercepting_command_factory.go | 7 | ||||
-rw-r--r-- | internal/git/gittest/intercepting_command_factory_test.go | 11 | ||||
-rw-r--r-- | internal/gitaly/service/operations/apply_patch.go | 20 | ||||
-rw-r--r-- | internal/gitaly/service/repository/redirecting_test_server_test.go | 17 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/inforefs.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/receive_pack.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/upload_pack.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/monitor_stdin_command.go | 13 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/receive_pack.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/upload_archive.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/upload_pack.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/upload_pack_test.go | 64 |
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() { |