updateref: Convert `HookError` to `CustomHookError`

We have a `HookError` type that gets returned whenever the execution of
a Git hook fails. In general though the informaton that this error holds
is not of any importance, but we only really care about special-handling
errors raised by custom hooks.

Refactor the error to a more specialized `CustomHookError` to make it
more useful.

9 files changed, 66 insertions, 53 deletions

diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go
index 0a27327df..6b78cb0be 100644
--- a/internal/git/updateref/update_with_hooks.go
+++ b/internal/git/updateref/update_with_hooks.go
@@ -30,15 +30,15 @@ type UpdaterWithHooks struct {
 	catfileCache  catfile.Cache
 }
 
-// HookError contains an error message when executing a hook.
-type HookError struct {
+// CustomHookError contains an error message when executing a custom hook.
+type CustomHookError struct {
 	err    error
 	stdout string
 	stderr string
 }
 
 // Error returns an error message.
-func (e HookError) Error() string {
+func (e CustomHookError) Error() string {
 	// If custom hooks write the "GitLab: " or "GL-HOOK-ERR: " prefix to either their stderr or
 	// their stdout, then this prefix is taken as a hint by Rails to print the error as-is in
 	// the web interface. We must thus make sure to not modify these custom hook error messages
@@ -50,31 +50,44 @@ func (e HookError) Error() string {
 	//
 	// Eventually, we should find a solution which allows us to bubble up the error in hook
 	// package such that we can also make proper use of structured errors for custom hooks.
-	var customHookError hook.CustomHookError
-	if errors.As(e.err, &customHookError) {
-		if len(strings.TrimSpace(e.stderr)) > 0 {
-			return e.stderr
-		}
-		if len(strings.TrimSpace(e.stdout)) > 0 {
-			return e.stdout
-		}
-	} else {
-		if len(strings.TrimSpace(e.stderr)) > 0 {
-			return fmt.Sprintf("%v, stderr: %q", e.err.Error(), e.stderr)
-		}
-		if len(strings.TrimSpace(e.stdout)) > 0 {
-			return fmt.Sprintf("%v, stdout: %q", e.err.Error(), e.stdout)
-		}
+	if len(strings.TrimSpace(e.stderr)) > 0 {
+		return e.stderr
+	}
+	if len(strings.TrimSpace(e.stdout)) > 0 {
+		return e.stdout
 	}
 
 	return e.err.Error()
 }
 
 // Unwrap will return the embedded error.
-func (e HookError) Unwrap() error {
+func (e CustomHookError) Unwrap() error {
 	return e.err
 }
 
+// wrapHookError wraps errors returned by the hook manager into either a CustomHookError if it
+// returned a `hook.CustomHookError`, or alternatively return the error with stderr or stdout
+// appended to the message.
+func wrapHookError(err error, stdout, stderr string) error {
+	var customHookErr hook.CustomHookError
+	if errors.As(err, &customHookErr) {
+		return CustomHookError{
+			err:    err,
+			stdout: stdout,
+			stderr: stderr,
+		}
+	}
+
+	if len(strings.TrimSpace(stderr)) > 0 {
+		return fmt.Errorf("%w, stderr: %q", err, stderr)
+	}
+	if len(strings.TrimSpace(stdout)) > 0 {
+		return fmt.Errorf("%w, stdout: %q", err, stdout)
+	}
+
+	return err
+}
+
 // Error reports an error in git update-ref
 type Error struct {
 	// Reference is the name of the reference that would have been updated.
@@ -161,7 +174,7 @@ func (u *UpdaterWithHooks) UpdateReference(
 
 	var stdout, stderr bytes.Buffer
 	if err := u.hookManager.PreReceiveHook(ctx, quarantinedRepo, pushOptions, []string{hooksPayload}, strings.NewReader(changes), &stdout, &stderr); err != nil {
-		return HookError{err: err, stdout: stdout.String(), stderr: stderr.String()}
+		return wrapHookError(err, stdout.String(), stderr.String())
 	}
 
 	// Now that Rails has told us that the change is okay via the pre-receive hook, we can
@@ -183,7 +196,7 @@ func (u *UpdaterWithHooks) UpdateReference(
 	}
 
 	if err := u.hookManager.UpdateHook(ctx, quarantinedRepo, reference.String(), oldrev.String(), newrev.String(), []string{hooksPayload}, &stdout, &stderr); err != nil {
-		return HookError{err: err, stdout: stdout.String(), stderr: stderr.String()}
+		return wrapHookError(err, stdout.String(), stderr.String())
 	}
 
 	// We are already manually invoking the reference-transaction hook, so there is no need to
@@ -235,7 +248,7 @@ func (u *UpdaterWithHooks) UpdateReference(
 	}
 
 	if err := u.hookManager.PostReceiveHook(ctx, repo, pushOptions, []string{hooksPayload}, strings.NewReader(changes), &stdout, &stderr); err != nil {
-		return HookError{err: err, stdout: stdout.String(), stderr: stderr.String()}
+		return wrapHookError(err, stdout.String(), stderr.String())
 	}
 
 	return nil
diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go
index 736f3c155..e8bba4ab5 100644
--- a/internal/gitaly/service/operations/branches.go
+++ b/internal/gitaly/service/operations/branches.go
@@ -56,10 +56,10 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, startPointOID, git.ZeroOID); err != nil {
-		var hookError updateref.HookError
-		if errors.As(err, &hookError) {
+		var customHookErr updateref.CustomHookError
+		if errors.As(err, &customHookErr) {
 			return &gitalypb.UserCreateBranchResponse{
-				PreReceiveError: hookError.Error(),
+				PreReceiveError: customHookErr.Error(),
 			}, nil
 		}
 
@@ -124,10 +124,10 @@ func (s *Server) UserUpdateBranch(ctx context.Context, req *gitalypb.UserUpdateB
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, newOID, oldOID); err != nil {
-		var hookError updateref.HookError
-		if errors.As(err, &hookError) {
+		var customHookErr updateref.CustomHookError
+		if errors.As(err, &customHookErr) {
 			return &gitalypb.UserUpdateBranchResponse{
-				PreReceiveError: hookError.Error(),
+				PreReceiveError: customHookErr.Error(),
 			}, nil
 		}
 
@@ -164,10 +164,10 @@ func (s *Server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteB
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, nil, referenceName, git.ZeroOID, referenceValue); err != nil {
-		var hookError updateref.HookError
-		if errors.As(err, &hookError) {
+		var customHookErr updateref.CustomHookError
+		if errors.As(err, &customHookErr) {
 			return &gitalypb.UserDeleteBranchResponse{
-				PreReceiveError: hookError.Error(),
+				PreReceiveError: customHookErr.Error(),
 			}, nil
 		}
 
diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go
index 801ee93c1..53a0953c4 100644
--- a/internal/gitaly/service/operations/cherry_pick.go
+++ b/internal/gitaly/service/operations/cherry_pick.go
@@ -108,7 +108,7 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, newrev, oldrev); err != nil {
-		if errors.As(err, &updateref.HookError{}) {
+		if errors.As(err, &updateref.CustomHookError{}) {
 			return &gitalypb.UserCherryPickResponse{
 				PreReceiveError: err.Error(),
 			}, nil
diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go
index 6b4f4539d..203537277 100644
--- a/internal/gitaly/service/operations/commit_files.go
+++ b/internal/gitaly/service/operations/commit_files.go
@@ -60,9 +60,9 @@ func (s *Server) UserCommitFiles(stream gitalypb.OperationService_UserCommitFile
 		}
 
 		var (
-			response   gitalypb.UserCommitFilesResponse
-			indexError git2go.IndexError
-			hookError  updateref.HookError
+			response      gitalypb.UserCommitFilesResponse
+			indexError    git2go.IndexError
+			customHookErr updateref.CustomHookError
 		)
 
 		switch {
@@ -74,8 +74,8 @@ func (s *Server) UserCommitFiles(stream gitalypb.OperationService_UserCommitFile
 			response = gitalypb.UserCommitFilesResponse{IndexError: "A file with this name already exists"}
 		case errors.As(err, new(git2go.FileNotFoundError)):
 			response = gitalypb.UserCommitFilesResponse{IndexError: "A file with this name doesn't exist"}
-		case errors.As(err, &hookError):
-			response = gitalypb.UserCommitFilesResponse{PreReceiveError: hookError.Error()}
+		case errors.As(err, &customHookErr):
+			response = gitalypb.UserCommitFilesResponse{PreReceiveError: customHookErr.Error()}
 		case errors.As(err, new(git2go.InvalidArgumentError)):
 			return helper.ErrInvalidArgument(err)
 		default:
diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index c193fa6d6..d89b8c092 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -228,10 +228,10 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, in.GetRepository(), in.User, quarantineDir, referenceName, commitID, revision); err != nil {
-		var hookError updateref.HookError
-		if errors.As(err, &hookError) {
+		var customHookErr updateref.CustomHookError
+		if errors.As(err, &customHookErr) {
 			return &gitalypb.UserFFBranchResponse{
-				PreReceiveError: hookError.Error(),
+				PreReceiveError: customHookErr.Error(),
 			}, nil
 		}
 
diff --git a/internal/gitaly/service/operations/rebase.go b/internal/gitaly/service/operations/rebase.go
index e23a18fe9..152b9afc3 100644
--- a/internal/gitaly/service/operations/rebase.go
+++ b/internal/gitaly/service/operations/rebase.go
@@ -128,7 +128,7 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba
 		oldrev,
 		header.GitPushOptions...); err != nil {
 		switch {
-		case errors.As(err, &updateref.HookError{}):
+		case errors.As(err, &updateref.CustomHookError{}):
 			if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) {
 				detailedErr, err := helper.ErrWithDetails(
 					helper.ErrPermissionDeniedf("access check: %q", err),
diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go
index 9b57b63b2..e5ebc280e 100644
--- a/internal/gitaly/service/operations/revert.go
+++ b/internal/gitaly/service/operations/revert.go
@@ -106,10 +106,10 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, newrev, oldrev); err != nil {
-		var hookError updateref.HookError
-		if errors.As(err, &hookError) {
+		var customHookErr updateref.CustomHookError
+		if errors.As(err, &customHookErr) {
 			return &gitalypb.UserRevertResponse{
-				PreReceiveError: hookError.Error(),
+				PreReceiveError: customHookErr.Error(),
 			}, nil
 		}
 
diff --git a/internal/gitaly/service/operations/submodules.go b/internal/gitaly/service/operations/submodules.go
index 68375c226..cbbd83894 100644
--- a/internal/gitaly/service/operations/submodules.go
+++ b/internal/gitaly/service/operations/submodules.go
@@ -152,10 +152,10 @@ func (s *Server) userUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpda
 		commitID,
 		branchOID,
 	); err != nil {
-		var hookError updateref.HookError
-		if errors.As(err, &hookError) {
+		var customHookErr updateref.CustomHookError
+		if errors.As(err, &customHookErr) {
 			return &gitalypb.UserUpdateSubmoduleResponse{
-				PreReceiveError: hookError.Error(),
+				PreReceiveError: customHookErr.Error(),
 			}, nil
 		}
 
diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go
index 678e1d68c..c685d680d 100644
--- a/internal/gitaly/service/operations/tags.go
+++ b/internal/gitaly/service/operations/tags.go
@@ -35,10 +35,10 @@ func (s *Server) UserDeleteTag(ctx context.Context, req *gitalypb.UserDeleteTagR
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, nil, referenceName, git.ZeroOID, revision); err != nil {
-		var hookError updateref.HookError
-		if errors.As(err, &hookError) {
+		var customHookErr updateref.CustomHookError
+		if errors.As(err, &customHookErr) {
 			return &gitalypb.UserDeleteTagResponse{
-				PreReceiveError: hookError.Error(),
+				PreReceiveError: customHookErr.Error(),
 			}, nil
 		}
 
@@ -111,10 +111,10 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR
 
 	referenceName := git.ReferenceName(fmt.Sprintf("refs/tags/%s", req.TagName))
 	if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, quarantineDir, referenceName, tagID, git.ZeroOID); err != nil {
-		var hookError updateref.HookError
-		if errors.As(err, &hookError) {
+		var customHookErrr updateref.CustomHookError
+		if errors.As(err, &customHookErrr) {
 			return &gitalypb.UserCreateTagResponse{
-				PreReceiveError: hookError.Error(),
+				PreReceiveError: customHookErrr.Error(),
 			}, nil
 		}