blob: Standardize returned errors

Replace different representation of the missing repository
argument error with the pre-defined ErrEmptyRepository.
Fix order of arguments validation with localrepo creation in GetBlob.
Replace status.Error(f) with helper.ErrXXX to have a common pattern
and omit possible double-wrapping of the gRPC Status.

4 files changed, 36 insertions, 30 deletions

diff --git a/internal/gitaly/service/blob/blobs.go b/internal/gitaly/service/blob/blobs.go
index f8b72177f..73cef0866 100644
--- a/internal/gitaly/service/blob/blobs.go
+++ b/internal/gitaly/service/blob/blobs.go
@@ -7,6 +7,7 @@ import (
 	"io"
 	"strings"
 
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gitpipe"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
@@ -19,7 +20,7 @@ import (
 
 func verifyListBlobsRequest(req *gitalypb.ListBlobsRequest) error {
 	if req.GetRepository() == nil {
-		return errors.New("empty repository")
+		return gitalyerrors.ErrEmptyRepository
 	}
 	if len(req.GetRevisions()) == 0 {
 		return errors.New("missing revisions")
@@ -237,7 +238,7 @@ func (s *server) ListAllBlobs(req *gitalypb.ListAllBlobsRequest, stream gitalypb
 	ctx := stream.Context()
 
 	if req.GetRepository() == nil {
-		return helper.ErrInvalidArgumentf("empty repository")
+		return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
 	}
 
 	repo := s.localrepo(req.GetRepository())
diff --git a/internal/gitaly/service/blob/get_blob.go b/internal/gitaly/service/blob/get_blob.go
index b3f593e68..8b4046fe0 100644
--- a/internal/gitaly/service/blob/get_blob.go
+++ b/internal/gitaly/service/blob/get_blob.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"io"
 
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
@@ -14,12 +15,12 @@ import (
 func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobService_GetBlobServer) error {
 	ctx := stream.Context()
 
-	repo := s.localrepo(in.GetRepository())
-
 	if err := validateRequest(in); err != nil {
-		return helper.ErrInvalidArgumentf("GetBlob: %v", err)
+		return helper.ErrInvalidArgument(err)
 	}
 
+	repo := s.localrepo(in.GetRepository())
+
 	objectReader, cancel, err := s.catfileCache.ObjectReader(stream.Context(), repo)
 	if err != nil {
 		return helper.ErrInternalf("GetBlob: %v", err)
@@ -70,6 +71,10 @@ func (s *server) GetBlob(in *gitalypb.GetBlobRequest, stream gitalypb.BlobServic
 }
 
 func validateRequest(in *gitalypb.GetBlobRequest) error {
+	if in.GetRepository() == nil {
+		return gitalyerrors.ErrEmptyRepository
+	}
+
 	if len(in.GetOid()) == 0 {
 		return fmt.Errorf("empty Oid")
 	}
diff --git a/internal/gitaly/service/blob/get_blobs.go b/internal/gitaly/service/blob/get_blobs.go
index 18694930f..0417d504e 100644
--- a/internal/gitaly/service/blob/get_blobs.go
+++ b/internal/gitaly/service/blob/get_blobs.go
@@ -2,14 +2,16 @@ package blob
 
 import (
 	"bytes"
+	"errors"
+	"fmt"
 	"io"
 
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v15/streamio"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 var treeEntryToObjectType = map[gitalypb.TreeEntry_EntryType]gitalypb.ObjectType{
@@ -38,14 +40,14 @@ func sendGetBlobsResponse(
 
 		treeEntry, err := tef.FindByRevisionAndPath(ctx, revision, string(path))
 		if err != nil {
-			return err
+			return helper.ErrInternal(err)
 		}
 
 		response := &gitalypb.GetBlobsResponse{Revision: revision, Path: path}
 
 		if treeEntry == nil || len(treeEntry.Oid) == 0 {
 			if err := stream.Send(response); err != nil {
-				return status.Errorf(codes.Unavailable, "GetBlobs: send: %v", err)
+				return helper.ErrUnavailablef("GetBlobs: send: %v", err)
 			}
 
 			continue
@@ -59,7 +61,7 @@ func sendGetBlobsResponse(
 			response.Type = gitalypb.ObjectType_COMMIT
 
 			if err := stream.Send(response); err != nil {
-				return status.Errorf(codes.Unavailable, "GetBlobs: send: %v", err)
+				return helper.ErrUnavailablef("GetBlobs: send: %v", err)
 			}
 
 			continue
@@ -67,7 +69,7 @@ func sendGetBlobsResponse(
 
 		objectInfo, err := objectInfoReader.Info(ctx, git.Revision(treeEntry.Oid))
 		if err != nil {
-			return status.Errorf(codes.Internal, "GetBlobs: %v", err)
+			return helper.ErrInternalf("GetBlobs: %v", err)
 		}
 
 		response.Size = objectInfo.Size
@@ -81,7 +83,7 @@ func sendGetBlobsResponse(
 
 		if response.Type != gitalypb.ObjectType_BLOB {
 			if err := stream.Send(response); err != nil {
-				return status.Errorf(codes.Unavailable, "GetBlobs: send: %v", err)
+				return helper.ErrUnavailablef("GetBlobs: send: %v", err)
 			}
 			continue
 		}
@@ -114,22 +116,22 @@ func sendBlobTreeEntry(
 	// blobObj.
 	if readLimit == 0 {
 		if err := stream.Send(response); err != nil {
-			return status.Errorf(codes.Unavailable, "GetBlobs: send: %v", err)
+			return helper.ErrUnavailablef("GetBlobs: send: %v", err)
 		}
 		return nil
 	}
 
 	blobObj, err := objectReader.Object(ctx, git.Revision(response.Oid))
 	if err != nil {
-		return status.Errorf(codes.Internal, "GetBlobs: %v", err)
+		return helper.ErrInternalf("GetBlobs: %v", err)
 	}
 	defer func() {
 		if _, err := io.Copy(io.Discard, blobObj); err != nil && returnedErr == nil {
-			returnedErr = status.Errorf(codes.Internal, "GetBlobs: discarding data: %v", err)
+			returnedErr = helper.ErrInternalf("GetBlobs: discarding data: %v", err)
 		}
 	}()
 	if blobObj.Type != "blob" {
-		return status.Errorf(codes.Internal, "blob got unexpected type %q", blobObj.Type)
+		return helper.ErrInternalf("blob got unexpected type %q", blobObj.Type)
 	}
 
 	sw := streamio.NewWriter(func(p []byte) error {
@@ -146,7 +148,7 @@ func sendBlobTreeEntry(
 
 	_, err = io.CopyN(sw, blobObj, readLimit)
 	if err != nil {
-		return status.Errorf(codes.Unavailable, "GetBlobs: send: %v", err)
+		return helper.ErrUnavailablef("GetBlobs: send: %v", err)
 	}
 
 	return nil
@@ -154,20 +156,20 @@ func sendBlobTreeEntry(
 
 func (s *server) GetBlobs(req *gitalypb.GetBlobsRequest, stream gitalypb.BlobService_GetBlobsServer) error {
 	if err := validateGetBlobsRequest(req); err != nil {
-		return err
+		return helper.ErrInvalidArgument(err)
 	}
 
 	repo := s.localrepo(req.GetRepository())
 
 	objectReader, cancel, err := s.catfileCache.ObjectReader(stream.Context(), repo)
 	if err != nil {
-		return err
+		return helper.ErrInternal(err)
 	}
 	defer cancel()
 
 	objectInfoReader, cancel, err := s.catfileCache.ObjectInfoReader(stream.Context(), repo)
 	if err != nil {
-		return err
+		return helper.ErrInternal(err)
 	}
 	defer cancel()
 
@@ -176,16 +178,16 @@ func (s *server) GetBlobs(req *gitalypb.GetBlobsRequest, stream gitalypb.BlobSer
 
 func validateGetBlobsRequest(req *gitalypb.GetBlobsRequest) error {
 	if req.Repository == nil {
-		return status.Errorf(codes.InvalidArgument, "GetBlobs: empty Repository")
+		return gitalyerrors.ErrEmptyRepository
 	}
 
 	if len(req.RevisionPaths) == 0 {
-		return status.Errorf(codes.InvalidArgument, "GetBlobs: empty RevisionPaths")
+		return errors.New("GetBlobs: empty RevisionPaths")
 	}
 
 	for _, rp := range req.RevisionPaths {
 		if err := git.ValidateRevision([]byte(rp.Revision)); err != nil {
-			return status.Errorf(codes.InvalidArgument, "GetBlobs: %v", err)
+			return fmt.Errorf("GetBlobs: %v", err)
 		}
 	}
 
diff --git a/internal/gitaly/service/blob/lfs_pointers.go b/internal/gitaly/service/blob/lfs_pointers.go
index 9c66782fb..28c231fa0 100644
--- a/internal/gitaly/service/blob/lfs_pointers.go
+++ b/internal/gitaly/service/blob/lfs_pointers.go
@@ -13,8 +13,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/chunk"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
 )
 
@@ -31,14 +29,14 @@ func (s *server) ListLFSPointers(in *gitalypb.ListLFSPointersRequest, stream git
 	ctx := stream.Context()
 
 	if in.GetRepository() == nil {
-		return status.Error(codes.InvalidArgument, "empty repository")
+		return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
 	}
 	if len(in.Revisions) == 0 {
-		return status.Error(codes.InvalidArgument, "missing revisions")
+		return helper.ErrInvalidArgumentf("missing revisions")
 	}
 	for _, revision := range in.Revisions {
 		if strings.HasPrefix(revision, "-") && revision != "--all" && revision != "--not" {
-			return status.Errorf(codes.InvalidArgument, "invalid revision: %q", revision)
+			return helper.ErrInvalidArgumentf("invalid revision: %q", revision)
 		}
 	}
 
@@ -82,7 +80,7 @@ func (s *server) ListAllLFSPointers(in *gitalypb.ListAllLFSPointersRequest, stre
 	ctx := stream.Context()
 
 	if in.GetRepository() == nil {
-		return status.Error(codes.InvalidArgument, "empty repository")
+		return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
 	}
 
 	repo := s.localrepo(in.GetRepository())
@@ -126,7 +124,7 @@ func (s *server) GetLFSPointers(req *gitalypb.GetLFSPointersRequest, stream gita
 	ctx := stream.Context()
 
 	if err := validateGetLFSPointersRequest(req); err != nil {
-		return status.Errorf(codes.InvalidArgument, "GetLFSPointers: %v", err)
+		return helper.ErrInvalidArgument(err)
 	}
 
 	repo := s.localrepo(req.GetRepository())