remote: Standardization of errors creation

Replace usage of status.Errorf() with helper.Err<StatusCode>f() to
make error creation more standard.
Remove redundant prefix from error returned by RPC call.
Replace old %v with new %w for proper error wrapping.
Add context to the error to make it more descriptive and standard.
Replace fmt.Errorf() with errors.New() if used without a reason.

3 files changed, 12 insertions, 15 deletions

diff --git a/internal/gitaly/service/remote/find_remote_repository.go b/internal/gitaly/service/remote/find_remote_repository.go
index 4b537ec3e..39785b8c9 100644
--- a/internal/gitaly/service/remote/find_remote_repository.go
+++ b/internal/gitaly/service/remote/find_remote_repository.go
@@ -6,14 +6,13 @@ import (
 	"io"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
+	"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"
 )
 
 func (s *server) FindRemoteRepository(ctx context.Context, req *gitalypb.FindRemoteRepositoryRequest) (*gitalypb.FindRemoteRepositoryResponse, error) {
 	if req.GetRemote() == "" {
-		return nil, status.Error(codes.InvalidArgument, "FindRemoteRepository: empty remote can't be checked.")
+		return nil, helper.ErrInvalidArgumentf("empty remote can't be checked.")
 	}
 
 	cmd, err := s.gitCmdFactory.NewWithoutRepo(ctx,
@@ -26,12 +25,12 @@ func (s *server) FindRemoteRepository(ctx context.Context, req *gitalypb.FindRem
 		},
 	)
 	if err != nil {
-		return nil, status.Errorf(codes.Internal, "error executing git command: %s", err)
+		return nil, helper.ErrInternalf("error executing git command: %w", err)
 	}
 
 	output, err := io.ReadAll(cmd)
 	if err != nil {
-		return nil, status.Errorf(codes.Internal, "unable to read stdout: %s", err)
+		return nil, helper.ErrInternalf("unable to read stdout: %w", err)
 	}
 	if err := cmd.Wait(); err != nil {
 		return &gitalypb.FindRemoteRepositoryResponse{Exists: false}, nil
diff --git a/internal/gitaly/service/remote/find_remote_root_ref.go b/internal/gitaly/service/remote/find_remote_root_ref.go
index a43de7422..0c7fa4953 100644
--- a/internal/gitaly/service/remote/find_remote_root_ref.go
+++ b/internal/gitaly/service/remote/find_remote_root_ref.go
@@ -10,8 +10,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"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"
 )
 
 const headPrefix = "HEAD branch: "
@@ -54,7 +52,7 @@ func (s *server) findRemoteRootRef(ctx context.Context, request *gitalypb.FindRe
 		if strings.HasPrefix(line, headPrefix) {
 			rootRef := strings.TrimPrefix(line, headPrefix)
 			if rootRef == "(unknown)" {
-				return "", status.Error(codes.NotFound, "no remote HEAD found")
+				return "", helper.ErrNotFoundf("no remote HEAD found")
 			}
 			return rootRef, nil
 		}
@@ -68,13 +66,13 @@ func (s *server) findRemoteRootRef(ctx context.Context, request *gitalypb.FindRe
 		return "", err
 	}
 
-	return "", status.Error(codes.NotFound, "couldn't query the remote HEAD")
+	return "", helper.ErrNotFoundf("couldn't query the remote HEAD")
 }
 
 // FindRemoteRootRef queries the remote to determine its HEAD
 func (s *server) FindRemoteRootRef(ctx context.Context, in *gitalypb.FindRemoteRootRefRequest) (*gitalypb.FindRemoteRootRefResponse, error) {
 	if in.GetRemoteUrl() == "" {
-		return nil, status.Error(codes.InvalidArgument, "missing remote URL")
+		return nil, helper.ErrInvalidArgumentf("missing remote URL")
 	}
 	if in.Repository == nil {
 		return nil, helper.ErrInvalidArgument(gitalyerrors.ErrEmptyRepository)
diff --git a/internal/gitaly/service/remote/update_remote_mirror.go b/internal/gitaly/service/remote/update_remote_mirror.go
index 343a48f8d..66028d475 100644
--- a/internal/gitaly/service/remote/update_remote_mirror.go
+++ b/internal/gitaly/service/remote/update_remote_mirror.go
@@ -27,7 +27,7 @@ const (
 func (s *server) UpdateRemoteMirror(stream gitalypb.RemoteService_UpdateRemoteMirrorServer) error {
 	firstRequest, err := stream.Recv()
 	if err != nil {
-		return helper.ErrInternalf("receive first request: %v", err)
+		return helper.ErrInternalf("receive first request: %w", err)
 	}
 
 	if err = validateUpdateRemoteMirrorRequest(stream.Context(), firstRequest); err != nil {
@@ -149,7 +149,7 @@ func (s *server) updateRemoteMirror(stream gitalypb.RemoteService_UpdateRemoteMi
 		if firstRequest.GetKeepDivergentRefs() {
 			isAncestor, err := repo.IsAncestor(ctx, git.Revision(remoteTarget), git.Revision(localRef.Target))
 			if err != nil && !errors.Is(err, localrepo.InvalidCommitError(remoteTarget)) {
-				return fmt.Errorf("is ancestor: %w", err)
+				return fmt.Errorf("is %q an ancestor of %q: %w", remoteTarget, localRef.Target, err)
 			}
 
 			if !isAncestor {
@@ -179,7 +179,7 @@ func (s *server) updateRemoteMirror(stream gitalypb.RemoteService_UpdateRemoteMi
 	for remoteRef, remoteCommitOID := range toDelete {
 		isAncestor, err := repo.IsAncestor(ctx, git.Revision(remoteCommitOID), git.Revision(defaultBranch))
 		if err != nil && !errors.Is(err, localrepo.InvalidCommitError(remoteCommitOID)) {
-			return fmt.Errorf("is ancestor: %w", err)
+			return fmt.Errorf("is %q an ancestor of %q: %w", remoteCommitOID, defaultBranch, err)
 		}
 
 		if isAncestor {
@@ -269,11 +269,11 @@ func validateUpdateRemoteMirrorRequest(ctx context.Context, req *gitalypb.Update
 	}
 
 	if req.GetRemote() == nil {
-		return fmt.Errorf("missing Remote")
+		return errors.New("missing Remote")
 	}
 
 	if req.GetRemote().GetUrl() == "" {
-		return fmt.Errorf("remote is missing URL")
+		return errors.New("remote is missing URL")
 	}
 
 	return nil