Merge branch 'ps-smarthttp-fix' into 'master'

smarthttp: Standardization of errors creation

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4983

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Pavlo Strokov <pstrokov@gitlab.com>

5 files changed, 24 insertions, 32 deletions

diff --git a/internal/gitaly/service/smarthttp/cache.go b/internal/gitaly/service/smarthttp/cache.go
index a60b16959..3ef3a877d 100644
--- a/internal/gitaly/service/smarthttp/cache.go
+++ b/internal/gitaly/service/smarthttp/cache.go
@@ -10,9 +10,8 @@ import (
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/cache"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 type infoRefCache struct {
@@ -67,7 +66,7 @@ func (c infoRefCache) tryCache(ctx context.Context, in *gitalypb.InfoRefsRequest
 		logger.Info("cache hit for UploadPack response")
 
 		if _, err := io.Copy(w, stream); err != nil {
-			return status.Errorf(codes.Internal, "GetInfoRefs: cache copy: %v", err)
+			return helper.ErrInternalf("cache copy: %w", err)
 		}
 
 		return nil
diff --git a/internal/gitaly/service/smarthttp/inforefs.go b/internal/gitaly/service/smarthttp/inforefs.go
index 9a48d26e6..c3d4dfd84 100644
--- a/internal/gitaly/service/smarthttp/inforefs.go
+++ b/internal/gitaly/service/smarthttp/inforefs.go
@@ -9,10 +9,9 @@ import (
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/pktline"
+	"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"
 )
 
 const (
@@ -68,26 +67,23 @@ func (s *server) handleInfoRefs(ctx context.Context, service, repoPath string, r
 		Args:  []string{repoPath},
 	}, cmdOpts...)
 	if err != nil {
-		if _, ok := status.FromError(err); ok {
-			return err
-		}
-		return status.Errorf(codes.Internal, "GetInfoRefs: cmd: %v", err)
+		return helper.ErrInternalf("cmd: %w", err)
 	}
 
 	if _, err := pktline.WriteString(w, fmt.Sprintf("# service=git-%s\n", service)); err != nil {
-		return status.Errorf(codes.Internal, "GetInfoRefs: pktLine: %v", err)
+		return helper.ErrInternalf("pktLine: %w", err)
 	}
 
 	if err := pktline.WriteFlush(w); err != nil {
-		return status.Errorf(codes.Internal, "GetInfoRefs: pktFlush: %v", err)
+		return helper.ErrInternalf("pktFlush: %w", err)
 	}
 
 	if _, err := io.Copy(w, cmd); err != nil {
-		return status.Errorf(codes.Internal, "GetInfoRefs: %v", err)
+		return helper.ErrInternalf("send: %w", err)
 	}
 
 	if err := cmd.Wait(); err != nil {
-		return status.Errorf(codes.Internal, "GetInfoRefs: %v", err)
+		return helper.ErrInternalf("wait: %w", err)
 	}
 
 	return nil
diff --git a/internal/gitaly/service/smarthttp/receive_pack.go b/internal/gitaly/service/smarthttp/receive_pack.go
index 323b6742c..8b454665a 100644
--- a/internal/gitaly/service/smarthttp/receive_pack.go
+++ b/internal/gitaly/service/smarthttp/receive_pack.go
@@ -5,14 +5,13 @@ import (
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	log "github.com/sirupsen/logrus"
+	gitalyerrors "gitlab.com/gitlab-org/gitaly/v15/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/transaction/voting"
 	"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"
 )
 
 func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePackServer) error {
@@ -64,11 +63,11 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac
 		git.WithConfig(config...),
 	)
 	if err != nil {
-		return status.Errorf(codes.Unavailable, "PostReceivePack: %v", err)
+		return helper.ErrUnavailable(err)
 	}
 
 	if err := cmd.Wait(); err != nil {
-		return status.Errorf(codes.Unavailable, "PostReceivePack: %v", err)
+		return helper.ErrUnavailable(err)
 	}
 
 	// In cases where all reference updates are rejected by git-receive-pack(1), we would end up
@@ -88,7 +87,7 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac
 		// To avoid this error being presented to the end user, ignore it when the
 		// transaction was stopped.
 		if !errors.Is(err, transaction.ErrTransactionStopped) {
-			return status.Errorf(codes.Aborted, "final transactional vote: %v", err)
+			return helper.ErrAbortedf("final transactional vote: %w", err)
 		}
 	}
 
@@ -97,13 +96,13 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac
 
 func validateReceivePackRequest(req *gitalypb.PostReceivePackRequest) error {
 	if req.GlId == "" {
-		return status.Errorf(codes.InvalidArgument, "PostReceivePack: empty GlId")
+		return helper.ErrInvalidArgumentf("empty GlId")
 	}
 	if req.Data != nil {
-		return status.Errorf(codes.InvalidArgument, "PostReceivePack: non-empty Data")
+		return helper.ErrInvalidArgumentf("non-empty Data")
 	}
 	if req.Repository == nil {
-		return helper.ErrInvalidArgumentf("PostReceivePack: empty Repository")
+		return helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
 	}
 
 	return nil
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index 723c92da7..c847a01d7 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -381,7 +381,7 @@ func TestPostReceivePack_requestValidation(t *testing.T) {
 					return helper.ErrInvalidArgumentf("repo scoped: invalid Repository")
 				}
 
-				return helper.ErrInvalidArgumentf("GetStorageByName: no such storage: %q", "fake")
+				return helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: "fake"`)
 			}(),
 		},
 		{
@@ -392,7 +392,7 @@ func TestPostReceivePack_requestValidation(t *testing.T) {
 					return helper.ErrInvalidArgumentf("repo scoped: empty Repository")
 				}
 
-				return helper.ErrInvalidArgumentf("PostReceivePack: empty Repository")
+				return helper.ErrInvalidArgumentf("empty Repository")
 			}(),
 		},
 		{
@@ -409,7 +409,7 @@ func TestPostReceivePack_requestValidation(t *testing.T) {
 					return helper.ErrNotFoundf("mutator call: route repository mutator: get repository id: repository %q/%q not found", cfg.Storages[0].Name, "path/to/repo")
 				}
 
-				return helper.ErrInvalidArgumentf("PostReceivePack: empty GlId")
+				return helper.ErrInvalidArgumentf("empty GlId")
 			}(),
 		},
 		{
@@ -427,7 +427,7 @@ func TestPostReceivePack_requestValidation(t *testing.T) {
 					return helper.ErrNotFoundf("mutator call: route repository mutator: get repository id: repository %q/%q not found", cfg.Storages[0].Name, "path/to/repo")
 				}
 
-				return helper.ErrInvalidArgumentf("PostReceivePack: non-empty Data")
+				return helper.ErrInvalidArgumentf("non-empty Data")
 			}(),
 		},
 	} {
diff --git a/internal/gitaly/service/smarthttp/upload_pack.go b/internal/gitaly/service/smarthttp/upload_pack.go
index b0353da19..49b03682d 100644
--- a/internal/gitaly/service/smarthttp/upload_pack.go
+++ b/internal/gitaly/service/smarthttp/upload_pack.go
@@ -14,8 +14,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/sidechannel"
 	"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"
 )
 
 type basicPostUploadPackRequest interface {
@@ -33,7 +31,7 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 	}
 
 	if req.Data != nil {
-		return status.Errorf(codes.InvalidArgument, "non-empty Data")
+		return helper.ErrInvalidArgumentf("non-empty Data")
 	}
 
 	repoPath, gitConfig, err := s.validateUploadPackRequest(ctx, req)
@@ -61,7 +59,7 @@ func (s *server) PostUploadPackWithSidechannel(ctx context.Context, req *gitalyp
 
 	conn, err := sidechannel.OpenSidechannel(ctx)
 	if err != nil {
-		return nil, status.Errorf(codes.Internal, "open sidechannel: %v", err)
+		return nil, helper.ErrInternalf("open sidechannel: %w", err)
 	}
 	defer conn.Close()
 
@@ -70,7 +68,7 @@ func (s *server) PostUploadPackWithSidechannel(ctx context.Context, req *gitalyp
 	}
 
 	if err := conn.Close(); err != nil {
-		return nil, status.Errorf(codes.Internal, "close sidechannel connection: %v", err)
+		return nil, helper.ErrInternalf("close sidechannel connection: %w", err)
 	}
 
 	return &gitalypb.PostUploadPackWithSidechannelResponse{}, nil
@@ -145,13 +143,13 @@ func (s *server) runUploadPack(ctx context.Context, req basicPostUploadPackReque
 		Args:  []string{repoPath},
 	}, commandOpts...)
 	if err != nil {
-		return helper.ErrUnavailablef("cmd: %v", err)
+		return helper.ErrUnavailablef("cmd: %w", err)
 	}
 
 	// Use a custom buffer size to minimize the number of system calls.
 	respBytes, err := io.CopyBuffer(stdout, cmd, make([]byte, 64*1024))
 	if err != nil {
-		return helper.ErrUnavailablef("Fail to transfer git data: %v", err)
+		return helper.ErrUnavailablef("Fail to transfer git data: %w", err)
 	}
 
 	if err := cmd.Wait(); err != nil {