operations: Convert UserCherryPick to return structured errorsjc-cherry-pick-list-conflicting-files

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..9506bc915 100644
--- a/internal/gitaly/service/operations/cherry_pick.go
+++ b/internal/gitaly/service/operations/cherry_pick.go
@@ -62,17 +62,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 +128,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.ErrFailedPreconditionf("cherry pick: %w", err),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_AccessCheck{
+						AccessCheck: &gitalypb.AccessCheckError{
+							ErrorMessage: err.Error(),
+						},
+					},
+				})
+
+			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..1c213f451 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,20 @@ 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(
+					//nolint: revive
+					errors.New("cherry pick: GL_ID=user-123\n"),
+				),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_AccessCheck{
+						AccessCheck: &gitalypb.AccessCheckError{
+							ErrorMessage: "GL_ID=user-123\n",
+						},
+					},
+				},
+			), err)
 		})
 	}
 }
@@ -397,9 +415,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 +452,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 +496,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 +601,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(), "cherry pick: executing custom hooks")
 
 	hookOutput := testhelper.MustReadFile(t, outputPath)
 	oid, err := git.NewObjectIDFromHex(text.ChompBytes(hookOutput))