operations: Refactor Ruby-compatible argument validation

The validation we have for arguments passed to the UserCreateTag RPC is
still carrying around quite some baggage from the old Ruby days. While
this made sense back in the day to reduce friction when migrating the
implementation of this RPC from Ruby to Go, this does feel somewhat
dated nowadays.

Refactor validation of arguments to return Go-style error messagess and
wrap the error in an `InvalidArgument` error code.

2 files changed, 17 insertions, 21 deletions

diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go
index b1f2a635c..d73f6842c 100644
--- a/internal/gitaly/service/operations/tags.go
+++ b/internal/gitaly/service/operations/tags.go
@@ -54,32 +54,28 @@ func (s *Server) UserDeleteTag(ctx context.Context, req *gitalypb.UserDeleteTagR
 }
 
 func validateUserCreateTag(req *gitalypb.UserCreateTagRequest) error {
-	// Emulate validations done by Ruby. A lot of these (e.g. the
-	// upper-case error messages) can be simplified once we're not
-	// doing bug-for-bug Ruby emulation anymore)
 	if len(req.TagName) == 0 {
-		return status.Errorf(codes.InvalidArgument, "empty tag name")
+		return fmt.Errorf("empty tag name")
 	}
 
 	if bytes.Contains(req.TagName, []byte(" ")) {
-		return status.Errorf(codes.Unknown, "Gitlab::Git::CommitError: Could not update refs/tags/%s. Please refresh and try again.", req.TagName)
+		return fmt.Errorf("tag name contains space")
 	}
 
 	if req.User == nil {
-		return status.Errorf(codes.InvalidArgument, "empty user")
+		return fmt.Errorf("empty user")
 	}
 
 	if len(req.TargetRevision) == 0 {
-		return status.Error(codes.InvalidArgument, "empty target revision")
+		return fmt.Errorf("empty target revision")
 	}
 
 	if bytes.Contains(req.Message, []byte("\000")) {
-		return status.Errorf(codes.Unknown, "ArgumentError: string contains null byte")
+		return fmt.Errorf("tag message contains NUL byte")
 	}
 
-	// Our own Go-specific validation
 	if req.GetRepository() == nil {
-		return status.Errorf(codes.Internal, "empty repository")
+		return fmt.Errorf("empty repository")
 	}
 
 	return nil
@@ -87,9 +83,8 @@ func validateUserCreateTag(req *gitalypb.UserCreateTagRequest) error {
 
 //nolint: revive,stylecheck // This is unintentionally missing documentation.
 func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagRequest) (*gitalypb.UserCreateTagResponse, error) {
-	// Validate the request
 	if err := validateUserCreateTag(req); err != nil {
-		return nil, err
+		return nil, helper.ErrInvalidArgumentf("validating request: %w", err)
 	}
 
 	targetRevision := git.Revision(req.TargetRevision)
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index b12b57987..f47ab56ae 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -16,6 +16,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/hook"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
@@ -435,7 +436,7 @@ func TestUserCreateTag_message(t *testing.T) {
 		{
 			desc:    "error: contains null byte",
 			message: "\000",
-			err:     status.Error(codes.Unknown, "ArgumentError: string contains null byte"),
+			err:     helper.ErrInvalidArgumentf("validating request: tag message contains NUL byte"),
 		},
 		{
 			desc:    "annotated: some control characters",
@@ -1159,7 +1160,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) {
 			targetRevision: "",
 			user:           gittest.TestUser,
 			response:       nil,
-			err:            status.Error(codes.InvalidArgument, "empty target revision"),
+			err:            helper.ErrInvalidArgumentf("validating request: empty target revision"),
 		},
 		{
 			desc:           "empty user",
@@ -1167,7 +1168,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) {
 			targetRevision: "master",
 			user:           nil,
 			response:       nil,
-			err:            status.Error(codes.InvalidArgument, "empty user"),
+			err:            helper.ErrInvalidArgumentf("validating request: empty user"),
 		},
 		{
 			desc:           "empty starting point",
@@ -1175,7 +1176,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) {
 			targetRevision: "",
 			user:           gittest.TestUser,
 			response:       nil,
-			err:            status.Error(codes.InvalidArgument, "empty target revision"),
+			err:            helper.ErrInvalidArgumentf("validating request: empty target revision"),
 		},
 		{
 			desc:           "non-existing starting point",
@@ -1183,7 +1184,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) {
 			targetRevision: "i-dont-exist",
 			user:           gittest.TestUser,
 			response:       nil,
-			err:            status.Errorf(codes.FailedPrecondition, "revspec '%s' not found", "i-dont-exist"),
+			err:            helper.ErrFailedPreconditionf("revspec '%s' not found", "i-dont-exist"),
 		},
 		{
 			desc:           "space in lightweight tag name",
@@ -1191,7 +1192,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) {
 			targetRevision: "master",
 			user:           gittest.TestUser,
 			response:       nil,
-			err:            status.Errorf(codes.Unknown, "Gitlab::Git::CommitError: Could not update refs/tags/%s. Please refresh and try again.", "a tag"),
+			err:            helper.ErrInvalidArgumentf("validating request: tag name contains space"),
 		},
 		{
 			desc:           "space in annotated tag name",
@@ -1200,7 +1201,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) {
 			message:        "a message",
 			user:           gittest.TestUser,
 			response:       nil,
-			err:            status.Errorf(codes.Unknown, "Gitlab::Git::CommitError: Could not update refs/tags/%s. Please refresh and try again.", "a tag"),
+			err:            helper.ErrInvalidArgumentf("validating request: tag name contains space"),
 		},
 		{
 			desc:           "newline in lightweight tag name",
@@ -1225,7 +1226,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) {
 			targetRevision: "master",
 			user:           gittest.TestUser,
 			response:       nil,
-			err:            status.Errorf(codes.Unknown, "Gitlab::Git::CommitError: Could not update refs/tags/%s. Please refresh and try again.", injectedTag),
+			err:            helper.ErrInvalidArgumentf("validating request: tag name contains space"),
 		},
 		{
 			desc:           "injection in annotated tag name",
@@ -1234,7 +1235,7 @@ func TestUserCreateTag_invalidArgument(t *testing.T) {
 			message:        "a message",
 			user:           gittest.TestUser,
 			response:       nil,
-			err:            status.Errorf(codes.Unknown, "Gitlab::Git::CommitError: Could not update refs/tags/%s. Please refresh and try again.", injectedTag),
+			err:            helper.ErrInvalidArgumentf("validating request: tag name contains space"),
 		},
 	}