Merge branch 'wc/fix-hook-err-detect' into 'master'

hook: Stop overmatching hooks errors

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

Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Pavlo Strokov <pstrokov@gitlab.com>

2 files changed, 38 insertions, 4 deletions

diff --git a/internal/git/updateref/update_with_hooks_test.go b/internal/git/updateref/update_with_hooks_test.go
index 88753701d..bea8839bd 100644
--- a/internal/git/updateref/update_with_hooks_test.go
+++ b/internal/git/updateref/update_with_hooks_test.go
@@ -229,7 +229,7 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) {
 			expectedErr: "reference-transaction failure",
 		},
 		{
-			desc: "post-receive error is ignored",
+			desc: "post-receive custom hooks error is ignored",
 			preReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
 				return nil
 			},
@@ -242,9 +242,28 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) {
 			postReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
 				_, err := io.Copy(stderr, strings.NewReader("post-receive failure"))
 				require.NoError(t, err)
-				return errors.New("ignored")
+				return hook.NewCustomHookError(errors.New("ignored"))
+			},
+			expectedRefDeletion: true,
+		},
+		{
+			desc: "post-receive non-custom hooks error returned",
+			preReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
+				return nil
+			},
+			update: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, ref, oldValue, newValue string, env []string, stdout, stderr io.Writer) error {
+				return nil
+			},
+			referenceTransaction: func(t *testing.T, ctx context.Context, state hook.ReferenceTransactionState, env []string, stdin io.Reader) error {
+				return nil
+			},
+			postReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error {
+				_, err := io.Copy(stderr, strings.NewReader("post-receive failure"))
+				require.NoError(t, err)
+				return errors.New("uh oh")
 			},
 			expectedRefDeletion: true,
+			expectedErr:         "running post-receive hooks: uh oh",
 		},
 	}
 
diff --git a/internal/gitaly/hook/custom.go b/internal/gitaly/hook/custom.go
index d63fc076e..6ccf968a0 100644
--- a/internal/gitaly/hook/custom.go
+++ b/internal/gitaly/hook/custom.go
@@ -20,7 +20,22 @@ import (
 type customHooksExecutor func(ctx context.Context, args, env []string, stdin io.Reader, stdout, stderr io.Writer) error
 
 // CustomHookError is returned in case custom hooks return an error.
-type CustomHookError error
+type CustomHookError struct {
+	err error
+}
+
+// NewCustomHookError creates a new CustomHookError.
+func NewCustomHookError(e error) CustomHookError {
+	return CustomHookError{err: e}
+}
+
+func (e CustomHookError) Error() string {
+	return e.err.Error()
+}
+
+func (e CustomHookError) Unwrap() error {
+	return e.err
+}
 
 // newCustomHooksExecutor creates a new hooks executor for custom hooks. Hooks
 // are looked up and executed in the following order:
@@ -84,7 +99,7 @@ func (m *GitLabHookManager) newCustomHooksExecutor(repo *gitalypb.Repository, ho
 				// not be modified, but instead used as-is as the hooks' error
 				// message given that they may contain output that should be shown
 				// to the user.
-				return CustomHookError(fmt.Errorf("error executing %q: %w", hookFile, err))
+				return NewCustomHookError(fmt.Errorf("error executing %q: %w", hookFile, err))
 			}
 		}