Merge branch 'pks-updateref-improved-errors' into 'master'

updateref: Improve errors to include breadcrumbs

See merge request gitlab-org/gitaly!4713

7 files changed, 34 insertions, 30 deletions

diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go
index fbd615be4..23155bc40 100644
--- a/internal/git/updateref/update_with_hooks.go
+++ b/internal/git/updateref/update_with_hooks.go
@@ -15,7 +15,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/storage"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/transaction/txinfo"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
@@ -157,17 +156,17 @@ 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 == "" {
-		return helper.ErrInternalf("UpdateReference: got no reference")
+		return fmt.Errorf("reference cannot be empty")
 	}
 	if err := git.ValidateObjectID(oldrev.String()); err != nil {
-		return helper.ErrInternalf("UpdateReference: got invalid old value: %w", err)
+		return fmt.Errorf("validating old value: %w", err)
 	}
 	if err := git.ValidateObjectID(newrev.String()); err != nil {
-		return helper.ErrInternalf("UpdateReference: got invalid new value: %w", err)
+		return fmt.Errorf("validating new value: %w", err)
 	}
 
 	changes := fmt.Sprintf("%s %s %s\n", oldrev, newrev, reference)
@@ -190,12 +189,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 +211,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 +268,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/git/updateref/update_with_hooks_test.go b/internal/git/updateref/update_with_hooks_test.go
index 592f1526b..f04dac5a9 100644
--- a/internal/git/updateref/update_with_hooks_test.go
+++ b/internal/git/updateref/update_with_hooks_test.go
@@ -48,46 +48,46 @@ func TestUpdaterWithHooks_UpdateReference_invalidParameters(t *testing.T) {
 		desc           string
 		ref            git.ReferenceName
 		newRev, oldRev git.ObjectID
-		expectedErr    string
+		expectedErr    error
 	}{
 		{
 			desc:        "missing reference",
 			oldRev:      revA,
 			newRev:      revB,
-			expectedErr: "got no reference",
+			expectedErr: fmt.Errorf("reference cannot be empty"),
 		},
 		{
 			desc:        "missing old rev",
 			ref:         "refs/heads/master",
 			newRev:      revB,
-			expectedErr: "got invalid old value",
+			expectedErr: fmt.Errorf("validating old value: %w", fmt.Errorf("%w: %q", git.ErrInvalidObjectID, "")),
 		},
 		{
 			desc:        "missing new rev",
 			ref:         "refs/heads/master",
 			oldRev:      revB,
-			expectedErr: "got invalid new value",
+			expectedErr: fmt.Errorf("validating new value: %w", fmt.Errorf("%w: %q", git.ErrInvalidObjectID, "")),
 		},
 		{
 			desc:        "invalid old rev",
 			ref:         "refs/heads/master",
 			newRev:      revA,
 			oldRev:      "foobar",
-			expectedErr: "got invalid old value",
+			expectedErr: fmt.Errorf("validating old value: %w", fmt.Errorf("%w: %q", git.ErrInvalidObjectID, "foobar")),
 		},
 		{
 			desc:        "invalid new rev",
 			ref:         "refs/heads/master",
 			newRev:      "foobar",
 			oldRev:      revB,
-			expectedErr: "got invalid new value",
+			expectedErr: fmt.Errorf("validating new value: %w", fmt.Errorf("%w: %q", git.ErrInvalidObjectID, "foobar")),
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			err := updater.UpdateReference(ctx, repo, user, nil, tc.ref, tc.newRev, tc.oldRev)
-			require.Contains(t, err.Error(), tc.expectedErr)
+			require.Equal(t, tc.expectedErr, err)
 		})
 	}
 }
diff --git a/internal/gitaly/service/conflicts/resolve_conflicts_test.go b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
index 83ccc258e..6dfda1eab 100644
--- a/internal/gitaly/service/conflicts/resolve_conflicts_test.go
+++ b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
@@ -891,7 +891,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 98444630c..3909632a0 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -458,7 +458,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{
@@ -479,7 +479,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{
@@ -726,7 +726,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{
@@ -841,7 +841,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/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go
index b35a078b8..7b8994479 100644
--- a/internal/gitaly/service/operations/cherry_pick.go
+++ b/internal/gitaly/service/operations/cherry_pick.go
@@ -175,13 +175,15 @@ 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 featureflag.CherryPickStructuredErrors.IsEnabled(ctx) {
-			if errors.As(err, &updateref.CustomHookError{}) {
+			var customHookErr updateref.CustomHookError
+
+			if errors.As(err, &customHookErr) {
 				detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
 					helper.ErrFailedPrecondition(errors.New("access check failed")),
 					&gitalypb.UserCherryPickError{
 						Error: &gitalypb.UserCherryPickError_AccessCheck{
 							AccessCheck: &gitalypb.AccessCheckError{
-								ErrorMessage: strings.TrimSuffix(err.Error(), "\n"),
+								ErrorMessage: strings.TrimSuffix(customHookErr.Error(), "\n"),
 							},
 						},
 					})
@@ -196,9 +198,10 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 			return nil, fmt.Errorf("update reference with hooks: %w", err)
 		}
 
-		if errors.As(err, &updateref.CustomHookError{}) {
+		var customHookErr updateref.CustomHookError
+		if errors.As(err, &customHookErr) {
 			return &gitalypb.UserCherryPickResponse{
-				PreReceiveError: err.Error(),
+				PreReceiveError: customHookErr.Error(),
 			}, nil
 		}
 
diff --git a/internal/gitaly/service/operations/rebase.go b/internal/gitaly/service/operations/rebase.go
index 8e1340fce..1ab3458e8 100644
--- a/internal/gitaly/service/operations/rebase.go
+++ b/internal/gitaly/service/operations/rebase.go
@@ -130,16 +130,18 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba
 		branch,
 		newrev,
 		oldrev,
-		header.GitPushOptions...); err != nil {
+		header.GitPushOptions...,
+	); err != nil {
+		var customHookErr updateref.CustomHookError
 		switch {
-		case errors.As(err, &updateref.CustomHookError{}):
+		case errors.As(err, &customHookErr):
 			if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) {
 				detailedErr, err := helper.ErrWithDetails(
 					helper.ErrPermissionDeniedf("access check: %q", err),
 					&gitalypb.UserRebaseConfirmableError{
 						Error: &gitalypb.UserRebaseConfirmableError_AccessCheck{
 							AccessCheck: &gitalypb.AccessCheckError{
-								ErrorMessage: err.Error(),
+								ErrorMessage: customHookErr.Error(),
 							},
 						},
 					},
diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go
index 0455bee51..a21af7644 100644
--- a/internal/gitaly/service/operations/rebase_test.go
+++ b/internal/gitaly/service/operations/rebase_test.go
@@ -552,7 +552,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{