Merge branch 'pks-cherry-pick-fix-neglected-feature-flag' into 'master'

operations: Honor feature flag for structured errors in UserCherryPick

See merge request gitlab-org/gitaly!4669

2 files changed, 170 insertions, 96 deletions

diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go
index 0c379e671..b35a078b8 100644
--- a/internal/gitaly/service/operations/cherry_pick.go
+++ b/internal/gitaly/service/operations/cherry_pick.go
@@ -11,6 +11,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -63,44 +64,62 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 		Mainline:      mainline,
 	})
 	if err != nil {
-		var conflictErr git2go.ConflictingFilesError
-		var emptyErr git2go.EmptyError
+		if featureflag.CherryPickStructuredErrors.IsEnabled(ctx) {
+			var conflictErr git2go.ConflictingFilesError
+			var emptyErr git2go.EmptyError
+
+			switch {
+			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:
+				return nil, helper.ErrInternalf("cherry-pick command: %w", err)
+			}
+		}
+
 		switch {
-		case errors.As(err, &git2go.HasConflictsError{}):
+		case errors.As(err, &git2go.HasConflictsError{}) || errors.As(err, &git2go.ConflictingFilesError{}):
 			return &gitalypb.UserCherryPickResponse{
 				CreateTreeError:     err.Error(),
 				CreateTreeErrorCode: gitalypb.UserCherryPickResponse_CONFLICT,
 			}, 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.As(err, &git2go.EmptyError{}):
+			return &gitalypb.UserCherryPickResponse{
+				CreateTreeError:     err.Error(),
+				CreateTreeErrorCode: gitalypb.UserCherryPickResponse_EMPTY,
+			}, nil
 		case errors.Is(err, git2go.ErrInvalidArgument):
 			return nil, helper.ErrInvalidArgument(err)
 		default:
@@ -129,42 +148,58 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 			return nil, err
 		}
 		if !ancestor {
-			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 featureflag.CherryPickStructuredErrors.IsEnabled(ctx) {
+				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)
+				}
 
-			if errGeneratingDetailedErr != nil {
-				return nil, helper.ErrInternalf("error details: %w", err)
+				return nil, detailedErr
 			}
 
-			return nil, detailedErr
+			return &gitalypb.UserCherryPickResponse{
+				CommitError: "Branch diverged",
+			}, nil
 		}
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, newrev, oldrev); err != nil {
-		if errors.As(err, &updateref.CustomHookError{}) {
-			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 featureflag.CherryPickStructuredErrors.IsEnabled(ctx) {
+			if errors.As(err, &updateref.CustomHookError{}) {
+				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)
+				}
 
-			if errGeneratingDetailedErr != nil {
-				return nil, helper.ErrInternalf("error details: %w", err)
+				return nil, detailedErr
 			}
 
-			return nil, detailedErr
+			return nil, fmt.Errorf("update reference with hooks: %w", err)
+		}
+
+		if errors.As(err, &updateref.CustomHookError{}) {
+			return &gitalypb.UserCherryPickResponse{
+				PreReceiveError: err.Error(),
+			}, nil
 		}
 
 		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 c6878cac2..a24711f78 100644
--- a/internal/gitaly/service/operations/cherry_pick_test.go
+++ b/internal/gitaly/service/operations/cherry_pick_test.go
@@ -346,7 +346,12 @@ func TestServer_UserCherryPick_failedValidations(t *testing.T) {
 
 func TestServer_UserCherryPick_failedWithPreReceiveError(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+
+	testhelper.NewFeatureSets(featureflag.CherryPickStructuredErrors).Run(t, testServerUserCherryPickFailedWithPreReceiveError)
+}
+
+func testServerUserCherryPickFailedWithPreReceiveError(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
@@ -373,26 +378,35 @@ func TestServer_UserCherryPick_failedWithPreReceiveError(t *testing.T) {
 			gittest.WriteCustomHook(t, repoPath, hookName, hookContent)
 
 			response, err := client.UserCherryPick(ctx, request)
-			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",
+			if featureflag.CherryPickStructuredErrors.IsEnabled(ctx) {
+				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)
+				), err)
+			} else {
+				require.NoError(t, err)
+				require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId)
+			}
 		})
 	}
 }
 
 func TestServer_UserCherryPick_failedWithCreateTreeError(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.CherryPickStructuredErrors).Run(t, testServerUserCherryPickFailedWithCreateTreeError)
+}
+
+func testServerUserCherryPickFailedWithCreateTreeError(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
@@ -414,20 +428,30 @@ func TestServer_UserCherryPick_failedWithCreateTreeError(t *testing.T) {
 	}
 
 	response, err := client.UserCherryPick(ctx, request)
-	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)
+	if featureflag.CherryPickStructuredErrors.IsEnabled(ctx) {
+		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)
+	} else {
+		require.NoError(t, err)
+		require.NotEmpty(t, response.CreateTreeError)
+		require.Equal(t, gitalypb.UserCherryPickResponse_EMPTY, response.CreateTreeErrorCode)
+	}
 }
 
 func TestServer_UserCherryPick_failedWithCommitError(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.CherryPickStructuredErrors).Run(t, testServerUserCherryPickFailedWithCommitError)
+}
+
+func testServerUserCherryPickFailedWithCommitError(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
@@ -451,18 +475,23 @@ func TestServer_UserCherryPick_failedWithCommitError(t *testing.T) {
 	}
 
 	response, err := client.UserCherryPick(ctx, request)
-	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)
+	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)
+
+		targetBranchDivergedErr, ok := detailedErr.Error.(*gitalypb.UserCherryPickError_TargetBranchDiverged)
+		require.True(t, ok)
+		assert.Equal(t, []byte("8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab"), targetBranchDivergedErr.TargetBranchDiverged.ParentRevision)
+	} else {
+		require.NoError(t, err)
+		require.Equal(t, "Branch diverged", response.CommitError)
+	}
 }
 
 func TestServerUserCherryPickRailedWithConflict(t *testing.T) {
@@ -573,7 +602,12 @@ func TestServer_UserCherryPick_successfulWithGivenCommits(t *testing.T) {
 
 func TestServer_UserCherryPick_quarantine(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
+
+	testhelper.NewFeatureSets(featureflag.CherryPickStructuredErrors).Run(t, testServerUserCherryPickQuarantine)
+}
+
+func testServerUserCherryPickQuarantine(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -600,9 +634,14 @@ func TestServer_UserCherryPick_quarantine(t *testing.T) {
 	}
 
 	response, err := client.UserCherryPick(ctx, request)
-	require.Nil(t, response)
-	require.NotNil(t, err)
-	assert.Contains(t, err.Error(), "access check failed")
+	if featureflag.CherryPickStructuredErrors.IsEnabled(ctx) {
+		require.Nil(t, response)
+		require.NotNil(t, err)
+		assert.Contains(t, err.Error(), "access check failed")
+	} else {
+		require.NoError(t, err)
+		require.NotNil(t, response)
+	}
 
 	hookOutput := testhelper.MustReadFile(t, outputPath)
 	oid, err := git.NewObjectIDFromHex(text.ChompBytes(hookOutput))