Merge branch 'pks-reftx-hook-preparations' into 'master'

Prepare for global reference-transaction hook executions

See merge request gitlab-org/gitaly!2825

15 files changed, 78 insertions, 45 deletions

diff --git a/internal/git/hooks/hooks.go b/internal/git/hooks/hooks.go
index aabb2f5fa..72b3a0df1 100644
--- a/internal/git/hooks/hooks.go
+++ b/internal/git/hooks/hooks.go
@@ -14,7 +14,7 @@ var Override string
 // environment variable GITALY_TESTING_NO_GIT_HOOKS is set to "1", Path
 // will return an empty directory, which has the effect that no Git hooks
 // will run at all.
-func Path() string {
+func Path(cfg config.Cfg) string {
 	if len(Override) > 0 {
 		return Override
 	}
@@ -23,7 +23,7 @@ func Path() string {
 		return "/var/empty"
 	}
 
-	return filepath.Join(config.Config.Ruby.Dir, "git-hooks")
+	return filepath.Join(cfg.Ruby.Dir, "git-hooks")
 }
 
 // GitPushOptions turns a slice of git push option values into a GIT_PUSH_OPTION_COUNT and individual
diff --git a/internal/git/hooks/hooks_test.go b/internal/git/hooks/hooks_test.go
index 1ea9266bf..a55989288 100644
--- a/internal/git/hooks/hooks_test.go
+++ b/internal/git/hooks/hooks_test.go
@@ -15,14 +15,14 @@ func TestPath(t *testing.T) {
 	config.Config.Ruby.Dir = "/bazqux/gitaly-ruby"
 
 	t.Run("default", func(t *testing.T) {
-		require.Equal(t, "/bazqux/gitaly-ruby/git-hooks", Path())
+		require.Equal(t, "/bazqux/gitaly-ruby/git-hooks", Path(config.Config))
 	})
 
 	t.Run("with an override", func(t *testing.T) {
 		Override = "/override/hooks"
 		defer func() { Override = "" }()
 
-		require.Equal(t, "/override/hooks", Path())
+		require.Equal(t, "/override/hooks", Path(config.Config))
 	})
 
 	t.Run("when an env override", func(t *testing.T) {
@@ -31,7 +31,7 @@ func TestPath(t *testing.T) {
 		os.Setenv(key, "1")
 		defer os.Unsetenv(key)
 
-		require.Equal(t, "/var/empty", Path())
+		require.Equal(t, "/var/empty", Path(config.Config))
 	})
 }
 
diff --git a/internal/git/objectpool/clone.go b/internal/git/objectpool/clone.go
index 0364be2dd..af9b00688 100644
--- a/internal/git/objectpool/clone.go
+++ b/internal/git/objectpool/clone.go
@@ -27,6 +27,7 @@ func (o *ObjectPool) clone(ctx context.Context, repo *gitalypb.Repository) error
 			},
 			Args: []string{repoPath, o.FullPath()},
 		},
+		git.WithRefTxHook(ctx, repo, o.cfg),
 	)
 	if err != nil {
 		return err
diff --git a/internal/git/receivepack.go b/internal/git/receivepack.go
index 46a5a45e6..c62cf8a5f 100644
--- a/internal/git/receivepack.go
+++ b/internal/git/receivepack.go
@@ -4,13 +4,14 @@ import (
 	"fmt"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
+	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 )
 
 // ReceivePackConfig contains config options we want to enforce when
 // receiving a push with git-receive-pack.
-func ReceivePackConfig() []Option {
+func ReceivePackConfig(cfg config.Cfg) []Option {
 	return []Option{
-		ValueFlag{"-c", fmt.Sprintf("core.hooksPath=%s", hooks.Path())},
+		ValueFlag{"-c", fmt.Sprintf("core.hooksPath=%s", hooks.Path(cfg))},
 
 		// In case the repository belongs to an object pool, we want to prevent
 		// Git from including the pool's refs in the ref advertisement. We do
diff --git a/internal/git/safecmd.go b/internal/git/safecmd.go
index d7f011a51..49be4deab 100644
--- a/internal/git/safecmd.go
+++ b/internal/git/safecmd.go
@@ -329,11 +329,17 @@ func SafeBareCmd(ctx context.Context, stream CmdStream, env []string, globals []
 }
 
 // SafeBareCmdInDir runs SafeBareCmd in the dir.
-func SafeBareCmdInDir(ctx context.Context, dir string, stream CmdStream, env []string, globals []Option, sc Cmd) (*command.Command, error) {
+func SafeBareCmdInDir(ctx context.Context, dir string, stream CmdStream, env []string, globals []Option, sc Cmd, opts ...CmdOpt) (*command.Command, error) {
 	if dir == "" {
 		return nil, errors.New("no 'dir' provided")
 	}
 
+	cc := &cmdCfg{}
+
+	if err := handleOpts(ctx, sc, cc, opts); err != nil {
+		return nil, err
+	}
+
 	args, err := combineArgs(globals, sc)
 	if err != nil {
 		return nil, err
@@ -362,7 +368,13 @@ func SafeStdinCmd(ctx context.Context, repo repository.GitRepo, globals []Option
 
 // SafeCmdWithoutRepo works like Command but without a git repository. It
 // validates the arguments in the command before executing.
-func SafeCmdWithoutRepo(ctx context.Context, stream CmdStream, globals []Option, sc SubCmd) (*command.Command, error) {
+func SafeCmdWithoutRepo(ctx context.Context, stream CmdStream, globals []Option, sc SubCmd, opts ...CmdOpt) (*command.Command, error) {
+	cc := &cmdCfg{}
+
+	if err := handleOpts(ctx, sc, cc, opts); err != nil {
+		return nil, err
+	}
+
 	args, err := combineArgs(globals, sc)
 	if err != nil {
 		return nil, err
diff --git a/internal/git/safecmd_test.go b/internal/git/safecmd_test.go
index 735b34d61..25ce89576 100644
--- a/internal/git/safecmd_test.go
+++ b/internal/git/safecmd_test.go
@@ -120,7 +120,7 @@ func TestSafeCmdInvalidArg(t *testing.T) {
 }
 
 func TestSafeCmdValid(t *testing.T) {
-	testRepo, _, cleanup := testhelper.NewTestRepo(t)
+	testRepo, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
 	ctx, cancel := testhelper.Context()
@@ -132,15 +132,18 @@ func TestSafeCmdValid(t *testing.T) {
 	endOfOptions := "--end-of-options"
 
 	for _, tt := range []struct {
+		desc       string
 		globals    []git.Option
 		subCmd     git.SubCmd
 		expectArgs []string
 	}{
 		{
+			desc:       "no args",
 			subCmd:     git.SubCmd{Name: "meow"},
 			expectArgs: []string{"meow", endOfOptions},
 		},
 		{
+			desc: "single option",
 			globals: []git.Option{
 				git.Flag{Name: "--aaaa-bbbb"},
 			},
@@ -148,6 +151,7 @@ func TestSafeCmdValid(t *testing.T) {
 			expectArgs: []string{"--aaaa-bbbb", "cccc", endOfOptions},
 		},
 		{
+			desc: "empty arg and postsep args",
 			subCmd: git.SubCmd{
 				Name:        "meow",
 				Args:        []string{""},
@@ -156,6 +160,7 @@ func TestSafeCmdValid(t *testing.T) {
 			expectArgs: []string{"meow", "", endOfOptions, "--", "-woof", ""},
 		},
 		{
+			desc: "full blown",
 			globals: []git.Option{
 				git.Flag{Name: "-a"},
 				git.ValueFlag{"-b", "c"},
@@ -173,6 +178,7 @@ func TestSafeCmdValid(t *testing.T) {
 			expectArgs: []string{"-a", "-b", "c", "d", "-e", "-f", "g", "-h=i", "1", "2", endOfOptions, "--", "3", "4", "5"},
 		},
 		{
+			desc: "output to stdout",
 			subCmd: git.SubCmd{
 				Name: "noun",
 				Flags: []git.Option{
@@ -184,6 +190,7 @@ func TestSafeCmdValid(t *testing.T) {
 			expectArgs: []string{"noun", "verb", "-", "--adjective", endOfOptions},
 		},
 		{
+			desc: "multiple value flags",
 			globals: []git.Option{
 				git.Flag{Name: "--contributing"},
 				git.ValueFlag{"--author", "a-gopher"},
@@ -199,29 +206,36 @@ func TestSafeCmdValid(t *testing.T) {
 			expectArgs: []string{"--contributing", "--author", "a-gopher", "accept", "--is-important", "--why", "looking-for-first-contribution", "mr", endOfOptions},
 		},
 	} {
-		opts := []git.CmdOpt{git.WithRefTxHook(ctx, &gitalypb.Repository{}, config.Config)}
-		cmd, err := git.SafeCmd(ctx, testRepo, tt.globals, tt.subCmd, opts...)
-		require.NoError(t, err)
-		// ignore first 3 indeterministic args (executable path and repo args)
-		require.Equal(t, tt.expectArgs, cmd.Args()[3:])
+		t.Run(tt.desc, func(t *testing.T) {
+			opts := []git.CmdOpt{git.WithRefTxHook(ctx, &gitalypb.Repository{}, config.Config)}
 
-		cmd, err = git.SafeCmdWithEnv(ctx, nil, testRepo, tt.globals, tt.subCmd, opts...)
-		require.NoError(t, err)
-		// ignore first 3 indeterministic args (executable path and repo args)
-		require.Equal(t, tt.expectArgs, cmd.Args()[3:])
+			cmd, err := git.SafeCmd(ctx, testRepo, tt.globals, tt.subCmd, opts...)
+			require.NoError(t, err)
+			// ignore first 3 indeterministic args (executable path and repo args)
+			require.Equal(t, tt.expectArgs, cmd.Args()[3:])
 
-		cmd, err = git.SafeStdinCmd(ctx, testRepo, tt.globals, tt.subCmd, opts...)
-		require.NoError(t, err)
-		require.Equal(t, tt.expectArgs, cmd.Args()[3:])
+			cmd, err = git.SafeCmdWithEnv(ctx, nil, testRepo, tt.globals, tt.subCmd, opts...)
+			require.NoError(t, err)
+			// ignore first 3 indeterministic args (executable path and repo args)
+			require.Equal(t, tt.expectArgs, cmd.Args()[3:])
 
-		cmd, err = git.SafeBareCmd(ctx, git.CmdStream{}, nil, tt.globals, tt.subCmd, opts...)
-		require.NoError(t, err)
-		// ignore first indeterministic arg (executable path)
-		require.Equal(t, tt.expectArgs, cmd.Args()[1:])
+			cmd, err = git.SafeStdinCmd(ctx, testRepo, tt.globals, tt.subCmd, opts...)
+			require.NoError(t, err)
+			require.Equal(t, tt.expectArgs, cmd.Args()[3:])
 
-		cmd, err = git.SafeCmdWithoutRepo(ctx, git.CmdStream{}, tt.globals, tt.subCmd)
-		require.NoError(t, err)
-		require.Equal(t, tt.expectArgs, cmd.Args()[1:])
+			cmd, err = git.SafeBareCmd(ctx, git.CmdStream{}, nil, tt.globals, tt.subCmd, opts...)
+			require.NoError(t, err)
+			// ignore first indeterministic arg (executable path)
+			require.Equal(t, tt.expectArgs, cmd.Args()[1:])
+
+			cmd, err = git.SafeCmdWithoutRepo(ctx, git.CmdStream{}, tt.globals, tt.subCmd, opts...)
+			require.NoError(t, err)
+			require.Equal(t, tt.expectArgs, cmd.Args()[1:])
+
+			cmd, err = git.SafeBareCmdInDir(ctx, testRepoPath, git.CmdStream{}, nil, tt.globals, tt.subCmd, opts...)
+			require.NoError(t, err)
+			require.Equal(t, tt.expectArgs, cmd.Args()[1:])
+		})
 	}
 }
 
diff --git a/internal/git/subcommand.go b/internal/git/subcommand.go
index 9f75fdc94..0f1864b2a 100644
--- a/internal/git/subcommand.go
+++ b/internal/git/subcommand.go
@@ -27,6 +27,7 @@ const (
 	scBundle         = "bundle"
 	scArchive        = "archive"
 	scFormatPatch    = "format-patch"
+	scInit           = "init"
 )
 
 var knownReadOnlyCmds = map[string]struct{}{
@@ -60,6 +61,7 @@ var knownNoRefUpdates = map[string]struct{}{
 	scRepack:         struct{}{},
 	scPackRefs:       struct{}{},
 	scHashObject:     struct{}{},
+	scInit:           struct{}{},
 }
 
 // mayUpdateRef indicates if a subcommand is known to update references.
diff --git a/internal/gitaly/rubyserver/rubyserver.go b/internal/gitaly/rubyserver/rubyserver.go
index 1d7542859..16cde6095 100644
--- a/internal/gitaly/rubyserver/rubyserver.go
+++ b/internal/gitaly/rubyserver/rubyserver.go
@@ -91,7 +91,7 @@ func (s *Server) start() error {
 		"GITALY_RUBY_GITALY_BIN_DIR="+cfg.BinDir,
 		"GITALY_RUBY_DIR="+cfg.Ruby.Dir,
 		"GITALY_VERSION="+version.GetVersion(),
-		"GITALY_GIT_HOOKS_DIR="+hooks.Path(),
+		"GITALY_GIT_HOOKS_DIR="+hooks.Path(cfg),
 		"GITALY_SOCKET="+cfg.GitalyInternalSocketPath(),
 		"GITALY_TOKEN="+cfg.Auth.Token,
 		"GITALY_RUGGED_GIT_CONFIG_SEARCH_PATH="+cfg.Ruby.RuggedGitConfigSearchPath)
diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go
index 32ec3db84..ce59b5cc9 100644
--- a/internal/gitaly/service/operations/squash.go
+++ b/internal/gitaly/service/operations/squash.go
@@ -216,7 +216,7 @@ func (s *server) userSquashWithDiffInFiles(ctx context.Context, req *gitalypb.Us
 		return "", fmt.Errorf("create sparse checkout file: %w", err)
 	}
 
-	if err := s.checkout(ctx, worktreePath, req); err != nil {
+	if err := s.checkout(ctx, repo, worktreePath, req); err != nil {
 		if !errors.Is(err, errNoFilesCheckedOut) {
 			return "", fmt.Errorf("perform 'git checkout' with core.sparseCheckout true: %w", err)
 		}
@@ -226,12 +226,12 @@ func (s *server) userSquashWithDiffInFiles(ctx context.Context, req *gitalypb.Us
 			return "", fmt.Errorf("on 'git config core.sparseCheckout false': %w", err)
 		}
 
-		if err := s.checkout(ctx, worktreePath, req); err != nil {
+		if err := s.checkout(ctx, repo, worktreePath, req); err != nil {
 			return "", fmt.Errorf("perform 'git checkout' with core.sparseCheckout false: %w", err)
 		}
 	}
 
-	sha, err := s.applyDiff(ctx, req, worktreePath, env)
+	sha, err := s.applyDiff(ctx, repo, req, worktreePath, env)
 	if err != nil {
 		return "", fmt.Errorf("apply diff: %w", err)
 	}
@@ -239,7 +239,7 @@ func (s *server) userSquashWithDiffInFiles(ctx context.Context, req *gitalypb.Us
 	return sha, nil
 }
 
-func (s *server) checkout(ctx context.Context, worktreePath string, req *gitalypb.UserSquashRequest) error {
+func (s *server) checkout(ctx context.Context, repo *gitalypb.Repository, worktreePath string, req *gitalypb.UserSquashRequest) error {
 	var stderr bytes.Buffer
 	checkoutCmd, err := git.SafeBareCmdInDir(ctx, worktreePath, git.CmdStream{Err: &stderr}, nil, nil,
 		git.SubCmd{
@@ -247,6 +247,7 @@ func (s *server) checkout(ctx context.Context, worktreePath string, req *gitalyp
 			Flags: []git.Option{git.Flag{Name: "--detach"}},
 			Args:  []string{req.GetStartSha()},
 		},
+		git.WithRefTxHook(ctx, repo, s.cfg),
 	)
 	if err != nil {
 		return fmt.Errorf("create 'git checkout': %w", gitError{ErrMsg: stderr.String(), Err: err})
@@ -297,7 +298,7 @@ func (s *server) userSquashWithNoDiff(ctx context.Context, req *gitalypb.UserSqu
 		}
 	}(filepath.Base(worktreePath))
 
-	sha, err := s.applyDiff(ctx, req, worktreePath, env)
+	sha, err := s.applyDiff(ctx, repo, req, worktreePath, env)
 	if err != nil {
 		return "", fmt.Errorf("apply diff: %w", err)
 	}
@@ -355,7 +356,7 @@ func (s *server) removeWorktree(ctx context.Context, repo *gitalypb.Repository,
 	return nil
 }
 
-func (s *server) applyDiff(ctx context.Context, req *gitalypb.UserSquashRequest, worktreePath string, env []string) (string, error) {
+func (s *server) applyDiff(ctx context.Context, repo *gitalypb.Repository, req *gitalypb.UserSquashRequest, worktreePath string, env []string) (string, error) {
 	diffRange := diffRange(req)
 
 	var diffStderr bytes.Buffer
@@ -381,7 +382,7 @@ func (s *server) applyDiff(ctx context.Context, req *gitalypb.UserSquashRequest,
 			git.Flag{Name: "--3way"},
 			git.Flag{Name: "--whitespace=nowarn"},
 		},
-	})
+	}, git.WithRefTxHook(ctx, repo, s.cfg))
 	if err != nil {
 		return "", fmt.Errorf("creation of 'git apply' for range %q: %w", diffRange, gitError{ErrMsg: applyStderr.String(), Err: err})
 	}
@@ -413,7 +414,7 @@ func (s *server) applyDiff(ctx context.Context, req *gitalypb.UserSquashRequest,
 			git.Flag{Name: "--quiet"},
 			git.ValueFlag{Name: "--message", Value: string(req.GetCommitMessage())},
 		},
-	})
+	}, git.WithRefTxHook(ctx, repo, s.cfg))
 	if err != nil {
 		return "", fmt.Errorf("creation of 'git commit': %w", gitError{ErrMsg: commitStderr.String(), Err: err})
 	}
diff --git a/internal/gitaly/service/repository/create_from_bundle.go b/internal/gitaly/service/repository/create_from_bundle.go
index 67031d8ee..a92486fdc 100644
--- a/internal/gitaly/service/repository/create_from_bundle.go
+++ b/internal/gitaly/service/repository/create_from_bundle.go
@@ -80,6 +80,7 @@ func (s *server) CreateRepositoryFromBundle(stream gitalypb.RepositoryService_Cr
 			},
 			PostSepArgs: []string{bundlePath, repoPath},
 		},
+		git.WithRefTxHook(ctx, repo, s.cfg),
 	)
 	if err != nil {
 		cleanError := sanitizedError(repoPath, "CreateRepositoryFromBundle: cmd start failed: %v", err)
@@ -99,6 +100,7 @@ func (s *server) CreateRepositoryFromBundle(stream gitalypb.RepositoryService_Cr
 			Flags: []git.Option{git.Flag{Name: "--quiet"}},
 			Args:  []string{bundlePath, "refs/*:refs/*"},
 		},
+		git.WithRefTxHook(ctx, repo, s.cfg),
 	)
 	if err != nil {
 		cleanError := sanitizedError(repoPath, "CreateRepositoryFromBundle: cmd start failed fetching refs: %v", err)
diff --git a/internal/gitaly/service/smarthttp/inforefs.go b/internal/gitaly/service/smarthttp/inforefs.go
index 71034daaa..57448c876 100644
--- a/internal/gitaly/service/smarthttp/inforefs.go
+++ b/internal/gitaly/service/smarthttp/inforefs.go
@@ -51,7 +51,7 @@ func (s *server) handleInfoRefs(ctx context.Context, service string, req *gitaly
 	var globalOpts []git.Option
 	cmdOpts := []git.CmdOpt{git.WithGitProtocol(ctx, req)}
 	if service == "receive-pack" {
-		globalOpts = append(globalOpts, git.ReceivePackConfig()...)
+		globalOpts = append(globalOpts, git.ReceivePackConfig(config.Config)...)
 		cmdOpts = append(cmdOpts, git.WithRefTxHook(ctx, req.Repository, config.Config))
 	}
 
diff --git a/internal/gitaly/service/smarthttp/receive_pack.go b/internal/gitaly/service/smarthttp/receive_pack.go
index f1187a2b3..119fe6c2f 100644
--- a/internal/gitaly/service/smarthttp/receive_pack.go
+++ b/internal/gitaly/service/smarthttp/receive_pack.go
@@ -43,7 +43,7 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac
 		return err
 	}
 
-	globalOpts := git.ReceivePackConfig()
+	globalOpts := git.ReceivePackConfig(config.Config)
 	for _, o := range req.GitConfigOptions {
 		globalOpts = append(globalOpts, git.ValueFlag{"-c", o})
 	}
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index 82bb8a995..9a1bc7008 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -146,10 +146,10 @@ func TestFailedReceivePackRequestDueToHooksFailure(t *testing.T) {
 	}(hooks.Override)
 	hooks.Override = hookDir
 
-	require.NoError(t, os.MkdirAll(hooks.Path(), 0755))
+	require.NoError(t, os.MkdirAll(hooks.Path(config.Config), 0755))
 
 	hookContent := []byte("#!/bin/sh\nexit 1")
-	ioutil.WriteFile(filepath.Join(hooks.Path(), "pre-receive"), hookContent, 0755)
+	ioutil.WriteFile(filepath.Join(hooks.Path(config.Config), "pre-receive"), hookContent, 0755)
 
 	serverSocketPath, stop := runSmartHTTPServer(t)
 	defer stop()
diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go
index d949eeb87..05dff7b56 100644
--- a/internal/gitaly/service/ssh/receive_pack.go
+++ b/internal/gitaly/service/ssh/receive_pack.go
@@ -61,7 +61,7 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer,
 		return err
 	}
 
-	globalOpts := git.ReceivePackConfig()
+	globalOpts := git.ReceivePackConfig(config.Config)
 	for _, o := range req.GitConfigOptions {
 		globalOpts = append(globalOpts, git.ValueFlag{"-c", o})
 	}
diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go
index 8fdae975d..a22daf1a6 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -153,10 +153,10 @@ func TestReceivePackPushHookFailure(t *testing.T) {
 	defer func(old string) { hooks.Override = old }(hooks.Override)
 	hooks.Override = hookDir
 
-	require.NoError(t, os.MkdirAll(hooks.Path(), 0755))
+	require.NoError(t, os.MkdirAll(hooks.Path(config.Config), 0755))
 
 	hookContent := []byte("#!/bin/sh\nexit 1")
-	ioutil.WriteFile(filepath.Join(hooks.Path(), "pre-receive"), hookContent, 0755)
+	ioutil.WriteFile(filepath.Join(hooks.Path(config.Config), "pre-receive"), hookContent, 0755)
 
 	_, _, err := testCloneAndPush(t, serverSocketPath, pushParams{storageName: testhelper.DefaultStorageName, glID: "1"})
 	require.Error(t, err)