updateref: Improve errors to include breadcrumbspks-updateref-improved-errors

When we get reports from customers about issues with running hooks it's
always hard to figure out what exactly is happening. Part of the problem
is that the errors we return from the `UpdaterWithHooks` aren't properly
wrappend and don't have any breadcrumbs. As a result, it's hard to see
without digging deep what code path has actually caused an error.

Improve the situation by properly wrapping errors. While this changes
the error messages, callers shouldn't rely on them anymore anyway and
instead only ever access the structured errors, which remain unchanged.

4 files changed, 12 insertions, 12 deletions

diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go
index fbd615be4..acb91562e 100644
--- a/internal/git/updateref/update_with_hooks.go
+++ b/internal/git/updateref/update_with_hooks.go
@@ -157,7 +157,7 @@ func (u *UpdaterWithHooks) UpdateReference(
 	if tx, err := txinfo.TransactionFromContext(ctx); err == nil {
 		transaction = &tx
 	} else if !errors.Is(err, txinfo.ErrTransactionNotFound) {
-		return err
+		return fmt.Errorf("getting transaction: %w", err)
 	}
 
 	if reference == "" {
@@ -190,12 +190,12 @@ func (u *UpdaterWithHooks) UpdateReference(
 
 	hooksPayload, err := git.NewHooksPayload(u.cfg, quarantinedRepo, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx)).Env()
 	if err != nil {
-		return err
+		return fmt.Errorf("constructing hooks payload: %w", err)
 	}
 
 	var stdout, stderr bytes.Buffer
 	if err := u.hookManager.PreReceiveHook(ctx, quarantinedRepo, pushOptions, []string{hooksPayload}, strings.NewReader(changes), &stdout, &stderr); err != nil {
-		return wrapHookError(err, git.PreReceiveHook, stdout.String(), stderr.String())
+		return fmt.Errorf("running pre-receive hooks: %w", wrapHookError(err, git.PreReceiveHook, stdout.String(), stderr.String()))
 	}
 
 	// Now that Rails has told us that the change is okay via the pre-receive hook, we can
@@ -212,12 +212,12 @@ func (u *UpdaterWithHooks) UpdateReference(
 		// real repository anyway.
 		hooksPayload, err = git.NewHooksPayload(u.cfg, repo, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx)).Env()
 		if err != nil {
-			return err
+			return fmt.Errorf("constructing quarantined hooks payload: %w", err)
 		}
 	}
 
 	if err := u.hookManager.UpdateHook(ctx, quarantinedRepo, reference.String(), oldrev.String(), newrev.String(), []string{hooksPayload}, &stdout, &stderr); err != nil {
-		return wrapHookError(err, git.UpdateHook, stdout.String(), stderr.String())
+		return fmt.Errorf("running update hooks: %w", wrapHookError(err, git.UpdateHook, stdout.String(), stderr.String()))
 	}
 
 	// We are already manually invoking the reference-transaction hook, so there is no need to
@@ -269,7 +269,7 @@ func (u *UpdaterWithHooks) UpdateReference(
 	}
 
 	if err := u.hookManager.PostReceiveHook(ctx, repo, pushOptions, []string{hooksPayload}, strings.NewReader(changes), &stdout, &stderr); err != nil {
-		return wrapHookError(err, git.PostReceiveHook, stdout.String(), stderr.String())
+		return fmt.Errorf("running post-receive hooks: %w", wrapHookError(err, git.PostReceiveHook, stdout.String(), stderr.String()))
 	}
 
 	return nil
diff --git a/internal/gitaly/service/conflicts/resolve_conflicts_test.go b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
index 1103b78dc..0c4f72191 100644
--- a/internal/gitaly/service/conflicts/resolve_conflicts_test.go
+++ b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
@@ -889,7 +889,7 @@ func TestResolveConflictsQuarantine(t *testing.T) {
 	}))
 
 	response, err := stream.CloseAndRecv()
-	require.EqualError(t, err, `rpc error: code = Unknown desc = af339cb882d1e3cf8d6751651e58bbaff0265d6e
+	require.EqualError(t, err, `rpc error: code = Unknown desc = running pre-receive hooks: af339cb882d1e3cf8d6751651e58bbaff0265d6e
 tree 89fad81bbfa38070b90ca8f4c404625bf0999013
 parent 29449b1d52cd77fd060a083a1de691bbaf12d8af
 parent 26dac52be85c92742b2c0c19eb7303de9feccb63
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index 35e9e5fd8..2ac365b5d 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -456,7 +456,7 @@ func TestUserDeleteBranch_allowed(t *testing.T) {
 				return false, "something something", nil
 			},
 			expectedErr: errWithDetails(t,
-				helper.ErrPermissionDeniedf("deletion denied by access checks: GitLab: something something"),
+				helper.ErrPermissionDeniedf("deletion denied by access checks: running pre-receive hooks: GitLab: something something"),
 				&gitalypb.UserDeleteBranchError{
 					Error: &gitalypb.UserDeleteBranchError_AccessCheck{
 						AccessCheck: &gitalypb.AccessCheckError{
@@ -477,7 +477,7 @@ func TestUserDeleteBranch_allowed(t *testing.T) {
 				return false, "something something", fmt.Errorf("something else")
 			},
 			expectedErr: errWithDetails(t,
-				helper.ErrPermissionDeniedf("deletion denied by access checks: GitLab: something else"),
+				helper.ErrPermissionDeniedf("deletion denied by access checks: running pre-receive hooks: GitLab: something else"),
 				&gitalypb.UserDeleteBranchError{
 					Error: &gitalypb.UserDeleteBranchError_AccessCheck{
 						AccessCheck: &gitalypb.AccessCheckError{
@@ -724,7 +724,7 @@ func TestUserDeleteBranch_hookFailure(t *testing.T) {
 
 			response, err := client.UserDeleteBranch(ctx, request)
 			testhelper.RequireGrpcError(t, errWithDetails(t,
-				helper.ErrPermissionDeniedf("deletion denied by custom hooks: %s\n", "GL_ID=user-123"),
+				helper.ErrPermissionDeniedf("deletion denied by custom hooks: running %s hooks: %s\n", tc.hookName, "GL_ID=user-123"),
 				&gitalypb.UserDeleteBranchError{
 					Error: &gitalypb.UserDeleteBranchError_CustomHook{
 						CustomHook: &gitalypb.CustomHookError{
@@ -839,7 +839,7 @@ func TestBranchHookOutput(t *testing.T) {
 
 				deleteResponse, err := client.UserDeleteBranch(ctx, deleteRequest)
 				testhelper.RequireGrpcError(t, errWithDetails(t,
-					helper.ErrPermissionDeniedf("deletion denied by custom hooks: %s", expectedError),
+					helper.ErrPermissionDeniedf("deletion denied by custom hooks: running %s hooks: %s", hookTestCase.hookName, expectedError),
 					&gitalypb.UserDeleteBranchError{
 						Error: &gitalypb.UserDeleteBranchError_CustomHook{
 							CustomHook: &gitalypb.CustomHookError{
diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go
index 25f813af0..56efad913 100644
--- a/internal/gitaly/service/operations/rebase_test.go
+++ b/internal/gitaly/service/operations/rebase_test.go
@@ -550,7 +550,7 @@ func testUserRebaseConfirmablePreReceiveError(t *testing.T, ctx context.Context)
 
 			if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) {
 				testhelper.RequireGrpcError(t, errWithDetails(t,
-					helper.ErrPermissionDeniedf(`access check: "failure\n"`),
+					helper.ErrPermissionDeniedf(`access check: "running %s hooks: failure\n"`, hookName),
 					&gitalypb.UserRebaseConfirmableError{
 						Error: &gitalypb.UserRebaseConfirmableError_AccessCheck{
 							AccessCheck: &gitalypb.AccessCheckError{