Merge branch 'pks-hook-refinements' into 'master'

Refinements for the hook manager

See merge request gitlab-org/gitaly!2537

6 files changed, 71 insertions, 67 deletions

diff --git a/internal/gitaly/hook/custom.go b/internal/gitaly/hook/custom.go
index 166544ed8..1e295159a 100644
--- a/internal/gitaly/hook/custom.go
+++ b/internal/gitaly/hook/custom.go
@@ -12,17 +12,24 @@ import (
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/internal/command"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
-// CustomHooksExecutor executes all custom hooks for a given repository and hook name
-type CustomHooksExecutor func(ctx context.Context, args, env []string, stdin io.Reader, stdout, stderr io.Writer) error
+// customHooksExecutor executes all custom hooks for a given repository and hook name
+type customHooksExecutor func(ctx context.Context, args, env []string, stdin io.Reader, stdout, stderr io.Writer) error
 
 // lookup hook files in this order:
 //
 // 1. <repository>.git/custom_hooks/<hook_name> - per project hook
 // 2. <repository>.git/custom_hooks/<hook_name>.d/* - per project hooks
 // 3. <repository>.git/hooks/<hook_name>.d/* - global hooks
-func (m *GitLabHookManager) NewCustomHooksExecutor(repoPath, customHooksDir, hookName string) (CustomHooksExecutor, error) {
+func (m *GitLabHookManager) newCustomHooksExecutor(repo *gitalypb.Repository, hookName string) (customHooksExecutor, error) {
+	repoPath, err := helper.GetRepoPath(repo)
+	if err != nil {
+		return nil, err
+	}
+
 	var hookFiles []string
 	projectCustomHookFile := filepath.Join(repoPath, "custom_hooks", hookName)
 	s, err := os.Stat(projectCustomHookFile)
@@ -37,7 +44,7 @@ func (m *GitLabHookManager) NewCustomHooksExecutor(repoPath, customHooksDir, hoo
 	}
 	hookFiles = append(hookFiles, files...)
 
-	globalCustomHooksDir := filepath.Join(customHooksDir, fmt.Sprintf("%s.d", hookName))
+	globalCustomHooksDir := filepath.Join(m.hooksConfig.CustomHooksDir, fmt.Sprintf("%s.d", hookName))
 
 	files, err = matchFiles(globalCustomHooksDir)
 	if err != nil {
diff --git a/internal/gitaly/hook/custom_test.go b/internal/gitaly/hook/custom_test.go
index 3ab97ee22..c20675d78 100644
--- a/internal/gitaly/hook/custom_test.go
+++ b/internal/gitaly/hook/custom_test.go
@@ -12,8 +12,10 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
 // printAllScript is a bash script that prints out stdin, the arguments,
@@ -47,7 +49,7 @@ echo "$0"
 exit 0`)
 
 func TestCustomHooksSuccess(t *testing.T) {
-	_, testRepoPath, cleanup := testhelper.NewTestRepo(t)
+	testRepo, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
 	testCases := []struct {
@@ -84,21 +86,21 @@ func TestCustomHooksSuccess(t *testing.T) {
 
 			// hook is in project custom hook directory <repository>.git/custom_hooks/<hook_name>
 			hookDir := filepath.Join(testRepoPath, "custom_hooks")
-			callAndVerifyHooks(t, testRepoPath, tc.hookName, globalCustomHooksDir, hookDir, tc.stdin, tc.args, tc.env)
+			callAndVerifyHooks(t, testRepo, tc.hookName, globalCustomHooksDir, hookDir, tc.stdin, tc.args, tc.env)
 
 			// hook is in project custom hooks directory <repository>.git/custom_hooks/<hook_name>.d/*
 			hookDir = filepath.Join(testRepoPath, "custom_hooks", fmt.Sprintf("%s.d", tc.hookName))
-			callAndVerifyHooks(t, testRepoPath, tc.hookName, globalCustomHooksDir, hookDir, tc.stdin, tc.args, tc.env)
+			callAndVerifyHooks(t, testRepo, tc.hookName, globalCustomHooksDir, hookDir, tc.stdin, tc.args, tc.env)
 
 			// hook is in global custom hooks directory <global_custom_hooks_dir>/<hook_name>.d/*
 			hookDir = filepath.Join(globalCustomHooksDir, fmt.Sprintf("%s.d", tc.hookName))
-			callAndVerifyHooks(t, testRepoPath, tc.hookName, globalCustomHooksDir, hookDir, tc.stdin, tc.args, tc.env)
+			callAndVerifyHooks(t, testRepo, tc.hookName, globalCustomHooksDir, hookDir, tc.stdin, tc.args, tc.env)
 		})
 	}
 }
 
 func TestCustomHookPartialFailure(t *testing.T) {
-	_, testRepoPath, cleanup := testhelper.NewTestRepo(t)
+	testRepo, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
 	globalCustomHooksDir, cleanup := testhelper.TempDir(t)
@@ -147,8 +149,13 @@ func TestCustomHookPartialFailure(t *testing.T) {
 			cleanup = writeCustomHook(t, tc.hook, globalHookPath, globalHookScript)
 			defer cleanup()
 
-			mgr := GitLabHookManager{}
-			caller, err := mgr.NewCustomHooksExecutor(testRepoPath, globalCustomHooksDir, tc.hook)
+			mgr := GitLabHookManager{
+				hooksConfig: config.Hooks{
+					CustomHooksDir: globalCustomHooksDir,
+				},
+			}
+
+			caller, err := mgr.newCustomHooksExecutor(testRepo, tc.hook)
 			require.NoError(t, err)
 
 			var stdout, stderr bytes.Buffer
@@ -168,7 +175,7 @@ func TestCustomHookPartialFailure(t *testing.T) {
 }
 
 func TestCustomHooksMultipleHooks(t *testing.T) {
-	_, testRepoPath, cleanup := testhelper.NewTestRepo(t)
+	testRepo, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
 	globalCustomHooksDir, cleanup := testhelper.TempDir(t)
@@ -196,8 +203,12 @@ func TestCustomHooksMultipleHooks(t *testing.T) {
 		expectedExecutedScripts = append(expectedExecutedScripts, filepath.Join(globalHooksPath, fileName))
 	}
 
-	mgr := GitLabHookManager{}
-	hooksExecutor, err := mgr.NewCustomHooksExecutor(testRepoPath, globalCustomHooksDir, "update")
+	mgr := GitLabHookManager{
+		hooksConfig: config.Hooks{
+			CustomHooksDir: globalCustomHooksDir,
+		},
+	}
+	hooksExecutor, err := mgr.newCustomHooksExecutor(testRepo, "update")
 	require.NoError(t, err)
 
 	var stdout, stderr bytes.Buffer
@@ -213,7 +224,7 @@ func TestCustomHooksMultipleHooks(t *testing.T) {
 }
 
 func TestMultilineStdin(t *testing.T) {
-	_, testRepoPath, cleanup := testhelper.NewTestRepo(t)
+	testRepo, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
 	globalCustomHooksDir, cleanup := testhelper.TempDir(t)
@@ -225,9 +236,13 @@ func TestMultilineStdin(t *testing.T) {
 	projectHooksPath := filepath.Join(testRepoPath, "custom_hooks", "pre-receive.d")
 
 	writeCustomHook(t, "pre-receive-script", projectHooksPath, printStdinScript)
+	mgr := GitLabHookManager{
+		hooksConfig: config.Hooks{
+			CustomHooksDir: globalCustomHooksDir,
+		},
+	}
 
-	mgr := GitLabHookManager{}
-	hooksExecutor, err := mgr.NewCustomHooksExecutor(testRepoPath, globalCustomHooksDir, "pre-receive")
+	hooksExecutor, err := mgr.newCustomHooksExecutor(testRepo, "pre-receive")
 	require.NoError(t, err)
 
 	changes := `old1 new1 ref1
@@ -242,7 +257,7 @@ old3 new3 ref3
 }
 
 func TestMultipleScriptsStdin(t *testing.T) {
-	_, testRepoPath, cleanup := testhelper.NewTestRepo(t)
+	testRepo, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
 	globalCustomHooksDir, cleanup := testhelper.TempDir(t)
@@ -259,8 +274,13 @@ func TestMultipleScriptsStdin(t *testing.T) {
 		writeCustomHook(t, fileName, projectHooksPath, printStdinScript)
 	}
 
-	mgr := GitLabHookManager{}
-	hooksExecutor, err := mgr.NewCustomHooksExecutor(testRepoPath, globalCustomHooksDir, "pre-receive")
+	mgr := GitLabHookManager{
+		hooksConfig: config.Hooks{
+			CustomHooksDir: globalCustomHooksDir,
+		},
+	}
+
+	hooksExecutor, err := mgr.newCustomHooksExecutor(testRepo, "pre-receive")
 	require.NoError(t, err)
 
 	changes := "oldref11 newref00 ref123445"
@@ -277,7 +297,7 @@ func TestMultipleScriptsStdin(t *testing.T) {
 	}
 }
 
-func callAndVerifyHooks(t *testing.T, repoPath, hookName, globalHooksDir, hookDir, stdin string, args, env []string) {
+func callAndVerifyHooks(t *testing.T, repo *gitalypb.Repository, hookName, globalHooksDir, hookDir, stdin string, args, env []string) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 	var stdout, stderr bytes.Buffer
@@ -285,8 +305,13 @@ func callAndVerifyHooks(t *testing.T, repoPath, hookName, globalHooksDir, hookDi
 	cleanup := writeCustomHook(t, hookName, hookDir, printAllScript)
 	defer cleanup()
 
-	mgr := GitLabHookManager{}
-	callHooks, err := mgr.NewCustomHooksExecutor(repoPath, globalHooksDir, hookName)
+	mgr := GitLabHookManager{
+		hooksConfig: config.Hooks{
+			CustomHooksDir: globalHooksDir,
+		},
+	}
+
+	callHooks, err := mgr.newCustomHooksExecutor(repo, hookName)
 	require.NoError(t, err)
 
 	require.NoError(t, callHooks(ctx, args, env, bytes.NewBufferString(stdin), &stdout, &stderr))
diff --git a/internal/gitaly/hook/manager.go b/internal/gitaly/hook/manager.go
index 5da7cc511..09cb82df1 100644
--- a/internal/gitaly/hook/manager.go
+++ b/internal/gitaly/hook/manager.go
@@ -27,10 +27,6 @@ type Manager interface {
 	// ReferenceTransactionHook executes the reference-transaction Git hook. stdin must contain
 	// all references to be updated and match the format specified in githooks(5).
 	ReferenceTransactionHook(ctx context.Context, env []string, stdin io.Reader) error
-
-	// NewCustomHooksExecutor creates a new executor for custom hooks. This function will get
-	// removed as soon as the Ruby hook implementation gets deleted.
-	NewCustomHooksExecutor(repoPath, customHooksDir, hookName string) (CustomHooksExecutor, error)
 }
 
 // GitLabHookManager is a hook manager containing Git hook business logic. It
diff --git a/internal/gitaly/hook/postreceive.go b/internal/gitaly/hook/postreceive.go
index 26e058ea3..d67fd2ee7 100644
--- a/internal/gitaly/hook/postreceive.go
+++ b/internal/gitaly/hook/postreceive.go
@@ -146,12 +146,7 @@ func (m *GitLabHookManager) PostReceiveHook(ctx context.Context, repo *gitalypb.
 		return errors.New("")
 	}
 
-	// custom hooks execution
-	repoPath, err := helper.GetRepoPath(repo)
-	if err != nil {
-		return err
-	}
-	executor, err := m.NewCustomHooksExecutor(repoPath, m.hooksConfig.CustomHooksDir, "post-receive")
+	executor, err := m.newCustomHooksExecutor(repo, "post-receive")
 	if err != nil {
 		return helper.ErrInternalf("creating custom hooks executor: %v", err)
 	}
diff --git a/internal/gitaly/hook/prereceive.go b/internal/gitaly/hook/prereceive.go
index dccc64396..441318aa7 100644
--- a/internal/gitaly/hook/prereceive.go
+++ b/internal/gitaly/hook/prereceive.go
@@ -40,31 +40,6 @@ func getRelativeObjectDirs(repoPath, gitObjectDir, gitAlternateObjectDirs string
 	return gitObjDirRel, gitAltObjDirsRel, nil
 }
 
-func (m *GitLabHookManager) executeCustomHooks(ctx context.Context, stdout, stderr io.Writer, changes []byte, repo *gitalypb.Repository, env []string) error {
-	// custom hooks execution
-	repoPath, err := helper.GetRepoPath(repo)
-	if err != nil {
-		return err
-	}
-	executor, err := m.NewCustomHooksExecutor(repoPath, m.hooksConfig.CustomHooksDir, "pre-receive")
-	if err != nil {
-		return fmt.Errorf("creating custom hooks executor: %w", err)
-	}
-
-	if err = executor(
-		ctx,
-		nil,
-		env,
-		bytes.NewReader(changes),
-		stdout,
-		stderr,
-	); err != nil {
-		return fmt.Errorf("executing custom hooks: %w", err)
-	}
-
-	return nil
-}
-
 func (m *GitLabHookManager) PreReceiveHook(ctx context.Context, repo *gitalypb.Repository, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
 	if gitObjDir, gitAltObjDirs := getEnvVar("GIT_OBJECT_DIRECTORY", env), getEnvVar("GIT_ALTERNATE_OBJECT_DIRECTORIES", env); gitObjDir != "" && gitAltObjDirs != "" {
 		repoPath, err := helper.GetRepoPath(repo)
@@ -104,8 +79,20 @@ func (m *GitLabHookManager) PreReceiveHook(ctx context.Context, repo *gitalypb.R
 			return errors.New(message)
 		}
 
-		if err := m.executeCustomHooks(ctx, stdout, stderr, changes, repo, env); err != nil {
-			return err
+		executor, err := m.newCustomHooksExecutor(repo, "pre-receive")
+		if err != nil {
+			return fmt.Errorf("creating custom hooks executor: %w", err)
+		}
+
+		if err = executor(
+			ctx,
+			nil,
+			env,
+			bytes.NewReader(changes),
+			stdout,
+			stderr,
+		); err != nil {
+			return fmt.Errorf("executing custom hooks: %w", err)
 		}
 
 		// reference counter
diff --git a/internal/gitaly/hook/update.go b/internal/gitaly/hook/update.go
index 52a14bd40..2290ff427 100644
--- a/internal/gitaly/hook/update.go
+++ b/internal/gitaly/hook/update.go
@@ -4,18 +4,12 @@ import (
 	"context"
 	"io"
 
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
 func (m *GitLabHookManager) UpdateHook(ctx context.Context, repo *gitalypb.Repository, ref, oldValue, newValue string, env []string, stdout, stderr io.Writer) error {
-	repoPath, err := helper.GetRepoPath(repo)
-	if err != nil {
-		return err
-	}
-
-	executor, err := m.NewCustomHooksExecutor(repoPath, config.Config.Hooks.CustomHooksDir, "update")
+	executor, err := m.newCustomHooksExecutor(repo, "update")
 	if err != nil {
 		return helper.ErrInternal(err)
 	}