operations: Convert UserCherryPick to return structured errors

When an error happens (merge conflict, changes already applied, pre
receveive, or target branch diverged), instead of returning the error
embedded in the response, return a structured error with some relevant
detail and context regarding what led to the error.

Changelog: changed

2 files changed, 134 insertions, 25 deletions

diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go
index f3dd635bc..0c379e671 100644
--- a/internal/gitaly/service/operations/cherry_pick.go
+++ b/internal/gitaly/service/operations/cherry_pick.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
@@ -62,17 +63,44 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 		Mainline:      mainline,
 	})
 	if err != nil {
+		var conflictErr git2go.ConflictingFilesError
+		var emptyErr git2go.EmptyError
 		switch {
 		case errors.As(err, &git2go.HasConflictsError{}):
 			return &gitalypb.UserCherryPickResponse{
 				CreateTreeError:     err.Error(),
 				CreateTreeErrorCode: gitalypb.UserCherryPickResponse_CONFLICT,
 			}, nil
-		case errors.As(err, &git2go.EmptyError{}):
-			return &gitalypb.UserCherryPickResponse{
-				CreateTreeError:     err.Error(),
-				CreateTreeErrorCode: gitalypb.UserCherryPickResponse_EMPTY,
-			}, nil
+		case errors.As(err, &conflictErr):
+			conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
+			for _, conflictingFile := range conflictErr.ConflictingFiles {
+				conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
+			}
+
+			detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+				helper.ErrFailedPreconditionf("cherry pick: %w", err),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_CherryPickConflict{
+						CherryPickConflict: &gitalypb.MergeConflictError{
+							ConflictingFiles: conflictingFiles,
+						},
+					},
+				})
+			if errGeneratingDetailedErr != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+			return nil, detailedErr
+		case errors.As(err, &emptyErr):
+			detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+				helper.ErrFailedPrecondition(err),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_ChangesAlreadyApplied{},
+				})
+			if errGeneratingDetailedErr != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+
+			return nil, detailedErr
 		case errors.Is(err, git2go.ErrInvalidArgument):
 			return nil, helper.ErrInvalidArgument(err)
 		default:
@@ -101,17 +129,42 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 			return nil, err
 		}
 		if !ancestor {
-			return &gitalypb.UserCherryPickResponse{
-				CommitError: "Branch diverged",
-			}, nil
+			detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+				helper.ErrFailedPrecondition(errors.New("cherry-pick: branch diverged")),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_TargetBranchDiverged{
+						TargetBranchDiverged: &gitalypb.NotAncestorError{
+							ParentRevision: []byte(oldrev.Revision()),
+							ChildRevision:  []byte(newrev),
+						},
+					},
+				})
+
+			if errGeneratingDetailedErr != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+
+			return nil, detailedErr
 		}
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, newrev, oldrev); err != nil {
 		if errors.As(err, &updateref.CustomHookError{}) {
-			return &gitalypb.UserCherryPickResponse{
-				PreReceiveError: err.Error(),
-			}, nil
+			detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+				helper.ErrFailedPrecondition(errors.New("access check failed")),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_AccessCheck{
+						AccessCheck: &gitalypb.AccessCheckError{
+							ErrorMessage: strings.TrimSuffix(err.Error(), "\n"),
+						},
+					},
+				})
+
+			if errGeneratingDetailedErr != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+
+			return nil, detailedErr
 		}
 
 		return nil, fmt.Errorf("update reference with hooks: %w", err)
diff --git a/internal/gitaly/service/operations/cherry_pick_test.go b/internal/gitaly/service/operations/cherry_pick_test.go
index aebb68a9d..c6878cac2 100644
--- a/internal/gitaly/service/operations/cherry_pick_test.go
+++ b/internal/gitaly/service/operations/cherry_pick_test.go
@@ -1,18 +1,24 @@
 package operations
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"path/filepath"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
+	"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/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
@@ -367,8 +373,19 @@ func TestServer_UserCherryPick_failedWithPreReceiveError(t *testing.T) {
 			gittest.WriteCustomHook(t, repoPath, hookName, hookContent)
 
 			response, err := client.UserCherryPick(ctx, request)
-			require.NoError(t, err)
-			require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId)
+			require.Nil(t, response)
+			testhelper.RequireGrpcError(t, errWithDetails(t,
+				helper.ErrFailedPrecondition(
+					errors.New("access check failed"),
+				),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_AccessCheck{
+						AccessCheck: &gitalypb.AccessCheckError{
+							ErrorMessage: "GL_ID=user-123",
+						},
+					},
+				},
+			), err)
 		})
 	}
 }
@@ -397,9 +414,15 @@ func TestServer_UserCherryPick_failedWithCreateTreeError(t *testing.T) {
 	}
 
 	response, err := client.UserCherryPick(ctx, request)
-	require.NoError(t, err)
-	require.NotEmpty(t, response.CreateTreeError)
-	require.Equal(t, gitalypb.UserCherryPickResponse_EMPTY, response.CreateTreeErrorCode)
+	require.Nil(t, response)
+	testhelper.RequireGrpcError(t, errWithDetails(t,
+		helper.ErrFailedPrecondition(
+			errors.New("cherry-pick: could not apply because the result was empty"),
+		),
+		&gitalypb.UserCherryPickError{
+			Error: &gitalypb.UserCherryPickError_ChangesAlreadyApplied{},
+		},
+	), err)
 }
 
 func TestServer_UserCherryPick_failedWithCommitError(t *testing.T) {
@@ -428,14 +451,30 @@ func TestServer_UserCherryPick_failedWithCommitError(t *testing.T) {
 	}
 
 	response, err := client.UserCherryPick(ctx, request)
-	require.NoError(t, err)
-	require.Equal(t, "Branch diverged", response.CommitError)
+	require.Nil(t, response)
+	s, ok := status.FromError(err)
+	require.True(t, ok)
+
+	details := s.Details()
+	require.Len(t, details, 1)
+	detailedErr, ok := details[0].(*gitalypb.UserCherryPickError)
+	require.True(t, ok)
+
+	targetBranchDivergedErr, ok := detailedErr.Error.(*gitalypb.UserCherryPickError_TargetBranchDiverged)
+	require.True(t, ok)
+	assert.Equal(t, []byte("8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab"), targetBranchDivergedErr.TargetBranchDiverged.ParentRevision)
 }
 
-func TestServer_UserCherryPick_failedWithConflict(t *testing.T) {
+func TestServerUserCherryPickRailedWithConflict(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
+	testhelper.NewFeatureSets(featureflag.CherryPickStructuredErrors).Run(
+		t,
+		testServerUserCherryPickRailedWithConflict,
+	)
+}
+
+func testServerUserCherryPickRailedWithConflict(t *testing.T, ctx context.Context) {
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -456,9 +495,25 @@ func TestServer_UserCherryPick_failedWithConflict(t *testing.T) {
 	}
 
 	response, err := client.UserCherryPick(ctx, request)
-	require.NoError(t, err)
-	require.NotEmpty(t, response.CreateTreeError)
-	require.Equal(t, gitalypb.UserCherryPickResponse_CONFLICT, response.CreateTreeErrorCode)
+	if featureflag.CherryPickStructuredErrors.IsEnabled(ctx) {
+		require.Nil(t, response)
+		s, ok := status.FromError(err)
+		require.True(t, ok)
+
+		details := s.Details()
+		require.Len(t, details, 1)
+		detailedErr, ok := details[0].(*gitalypb.UserCherryPickError)
+		require.True(t, ok)
+
+		conflictErr, ok := detailedErr.Error.(*gitalypb.UserCherryPickError_CherryPickConflict)
+		require.True(t, ok)
+		require.Len(t, conflictErr.CherryPickConflict.ConflictingFiles, 1)
+		assert.Equal(t, []byte("NEW_FILE.md"), conflictErr.CherryPickConflict.ConflictingFiles[0])
+	} else {
+		require.NoError(t, err)
+		require.NotEmpty(t, response.CreateTreeError)
+		require.Equal(t, gitalypb.UserCherryPickResponse_CONFLICT, response.CreateTreeErrorCode)
+	}
 }
 
 func TestServer_UserCherryPick_successfulWithGivenCommits(t *testing.T) {
@@ -545,8 +600,9 @@ func TestServer_UserCherryPick_quarantine(t *testing.T) {
 	}
 
 	response, err := client.UserCherryPick(ctx, request)
-	require.NoError(t, err)
-	require.NotNil(t, response)
+	require.Nil(t, response)
+	require.NotNil(t, err)
+	assert.Contains(t, err.Error(), "access check failed")
 
 	hookOutput := testhelper.MustReadFile(t, outputPath)
 	oid, err := git.NewObjectIDFromHex(text.ChompBytes(hookOutput))