updateref: Don't return gRPC-type errors in hook updater

Internal packages that aren't directly implementing gRPC services
shouldn't typically return gRPC-style errors. Refactor the hook updater
to return normal errors instead.

2 files changed, 10 insertions, 11 deletions

diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go
index acb91562e..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"
@@ -161,13 +160,13 @@ func (u *UpdaterWithHooks) UpdateReference(
 	}
 
 	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)
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)
 		})
 	}
 }