operations: Return structured errors in UserRevert

3 files changed, 169 insertions, 40 deletions

diff --git a/internal/featureflag/ff_return_structed_errors_in_revert.go b/internal/featureflag/ff_return_structed_errors_in_revert.go
new file mode 100644
index 000000000..52efdae02
--- /dev/null
+++ b/internal/featureflag/ff_return_structed_errors_in_revert.go
@@ -0,0 +1,13 @@
+package featureflag
+
+// ReturnStructuredErrorsInUserRevert enables return structured errors in UserRevert.
+// Modify the RPC UserRevert to return structured errors instead of
+// inline errors. Modify the handling of the following four
+// errors: 'Conflict', 'Changes Already Applied', 'Branch diverged',
+// and 'CustomHookError'. Returns the corresponding structured error.
+var ReturnStructuredErrorsInUserRevert = NewFeatureFlag(
+	"return_structured_errors_in_revert",
+	"v16.8.0",
+	"https://gitlab.com/gitlab-org/gitaly/-/issues/5752",
+	false,
+)
diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go
index 90d03a7dd..72e45dc8f 100644
--- a/internal/gitaly/service/operations/revert.go
+++ b/internal/gitaly/service/operations/revert.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"time"
 
+	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/remoterepo"
@@ -87,22 +88,46 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest
 	if err != nil {
 		var conflictErr *localrepo.MergeTreeConflictError
 		if errors.As(err, &conflictErr) {
-			return &gitalypb.UserRevertResponse{
-				// it's better that this error matches the git2go for now
-				CreateTreeError:     "revert: could not apply due to conflicts",
-				CreateTreeErrorCode: gitalypb.UserRevertResponse_CONFLICT,
-			}, nil
+			if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+				conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFileInfo))
+				for _, conflictingFileInfo := range conflictErr.ConflictingFileInfo {
+					conflictingFiles = append(conflictingFiles, []byte(conflictingFileInfo.FileName))
+				}
+				return nil, structerr.NewFailedPrecondition("revert: there are conflicting files").WithDetail(
+					&gitalypb.UserRevertError{
+						Error: &gitalypb.UserRevertError_MergeConflict{
+							MergeConflict: &gitalypb.MergeConflictError{
+								ConflictingFiles: conflictingFiles,
+							},
+						},
+					})
+			} else {
+				return &gitalypb.UserRevertResponse{
+					// it's better that this error matches the git2go for now
+					CreateTreeError:     "revert: could not apply due to conflicts",
+					CreateTreeErrorCode: gitalypb.UserRevertResponse_CONFLICT,
+				}, nil
+			}
 		}
 
 		return nil, structerr.NewInternal("merge-tree: %w", err)
 	}
 
 	if oursCommit.TreeId == treeOID.String() {
-		return &gitalypb.UserRevertResponse{
-			// it's better that this error matches the git2go for now
-			CreateTreeError:     "revert: could not apply because the result was empty",
-			CreateTreeErrorCode: gitalypb.UserRevertResponse_EMPTY,
-		}, nil
+		if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+			return nil, structerr.NewFailedPrecondition("revert: could not apply because the result was empty").WithDetail(
+				&gitalypb.UserRevertError{
+					Error: &gitalypb.UserRevertError_ChangesAlreadyApplied{
+						ChangesAlreadyApplied: &gitalypb.ChangesAlreadyAppliedError{},
+					},
+				})
+		} else {
+			return &gitalypb.UserRevertResponse{
+				// it's better that this error matches the git2go for now
+				CreateTreeError:     "revert: could not apply because the result was empty",
+				CreateTreeErrorCode: gitalypb.UserRevertResponse_EMPTY,
+			}, nil
+		}
 	}
 
 	newrev, err = quarantineRepo.WriteCommit(
@@ -166,18 +191,39 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest
 			return nil, structerr.NewInternal("checking for ancestry: %w", err)
 		}
 		if !ancestor {
-			return &gitalypb.UserRevertResponse{
-				CommitError: "Branch diverged",
-			}, nil
+			if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+				return nil, structerr.NewFailedPrecondition("revert: branch diverged").WithDetail(
+					&gitalypb.UserRevertError{
+						Error: &gitalypb.UserRevertError_NotAncestor{
+							NotAncestor: &gitalypb.NotAncestorError{
+								ParentRevision: []byte(oldrev.Revision()),
+								ChildRevision:  []byte(newrev.Revision()),
+							},
+						},
+					})
+			} else {
+				return &gitalypb.UserRevertResponse{
+					CommitError: "Branch diverged",
+				}, nil
+			}
 		}
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, newrev, oldrev); err != nil {
 		var customHookErr updateref.CustomHookError
 		if errors.As(err, &customHookErr) {
-			return &gitalypb.UserRevertResponse{
-				PreReceiveError: customHookErr.Error(),
-			}, nil
+			if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+				return nil, structerr.NewPermissionDenied("revert: custom hook error").WithDetail(
+					&gitalypb.UserRevertError{
+						Error: &gitalypb.UserRevertError_CustomHook{
+							CustomHook: customHookErr.Proto(),
+						},
+					})
+			} else {
+				return &gitalypb.UserRevertResponse{
+					PreReceiveError: customHookErr.Error(),
+				}, nil
+			}
 		}
 
 		return nil, fmt.Errorf("update reference with hooks: %w", err)
diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go
index 225669662..754bf0748 100644
--- a/internal/gitaly/service/operations/revert_test.go
+++ b/internal/gitaly/service/operations/revert_test.go
@@ -19,6 +19,8 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
@@ -427,6 +429,7 @@ func TestServer_UserRevert_quarantine(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
+		featureflag.ReturnStructuredErrorsInUserRevert,
 	).Run(t, testServerUserRevertQuarantine)
 }
 
@@ -468,9 +471,22 @@ func testServerUserRevertQuarantine(t *testing.T, ctx context.Context) {
 		Message:    []byte("Reverting " + revertedCommit.Id),
 		Timestamp:  &timestamppb.Timestamp{Seconds: 12345},
 	})
-	require.NoError(t, err)
-	require.NotNil(t, response)
-	require.NotEmpty(t, response.PreReceiveError)
+	if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+		expectedError := structerr.NewPermissionDenied("revert: custom hook error").WithDetail(
+			&gitalypb.UserRevertError{
+				Error: &gitalypb.UserRevertError_CustomHook{
+					CustomHook: &gitalypb.CustomHookError{
+						HookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE,
+					},
+				},
+			})
+		testhelper.RequireGrpcError(t, expectedError, err)
+	} else {
+		require.NoError(t, err)
+		require.NotNil(t, response)
+		require.NotEmpty(t, response.PreReceiveError)
+
+	}
 
 	objectHash, err := repo.ObjectHash(ctx)
 	require.NoError(t, err)
@@ -599,6 +615,7 @@ func TestServer_UserRevert_stableID(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
+		featureflag.ReturnStructuredErrorsInUserRevert,
 	).Run(t, testServerUserRevertStableID)
 }
 
@@ -652,8 +669,10 @@ func testServerUserRevertStableID(t *testing.T, ctx context.Context) {
 			"sha256": "28b57208e72bc2317143571997b9cfc444a51b52a43dde1c0282633a2b60de71",
 		}),
 	}, response.BranchUpdate)
-	require.Empty(t, response.CreateTreeError)
-	require.Empty(t, response.CreateTreeErrorCode)
+	if !featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+		require.Empty(t, response.CreateTreeError)
+		require.Empty(t, response.CreateTreeErrorCode)
+	}
 
 	// headCommit is pointed commit after revert
 	headCommit, err := repo.ReadCommit(ctx, git.Revision(git.DefaultBranch))
@@ -695,6 +714,7 @@ func TestServer_UserRevert_successfulIntoEmptyRepo(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
+		featureflag.ReturnStructuredErrorsInUserRevert,
 	).Run(t, testServerUserRevertSuccessfulIntoEmptyRepo)
 }
 
@@ -752,8 +772,10 @@ func testServerUserRevertSuccessfulIntoEmptyRepo(t *testing.T, ctx context.Conte
 	}
 
 	require.Equal(t, expectedBranchUpdate, response.BranchUpdate)
-	require.Empty(t, response.CreateTreeError)
-	require.Empty(t, response.CreateTreeErrorCode)
+	if !featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+		require.Empty(t, response.CreateTreeError)
+		require.Empty(t, response.CreateTreeErrorCode)
+	}
 	require.Equal(t, request.Message, headCommit.Subject)
 	require.Equal(t, revertedCommit.Id, headCommit.ParentIds[0])
 	gittest.RequireTree(t, cfg, repoPath, response.BranchUpdate.CommitId,
@@ -767,6 +789,7 @@ func TestServer_UserRevert_successfulGitHooks(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
+		featureflag.ReturnStructuredErrorsInUserRevert,
 	).Run(t, testServerUserRevertSuccessfulGitHooks)
 }
 
@@ -806,7 +829,9 @@ func testServerUserRevertSuccessfulGitHooks(t *testing.T, ctx context.Context) {
 
 	response, err := client.UserRevert(ctx, request)
 	require.NoError(t, err)
-	require.Empty(t, response.PreReceiveError)
+	if !featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+		require.Empty(t, response.PreReceiveError)
+	}
 	headCommit, err := repo.ReadCommit(ctx, git.Revision(destinationBranch))
 	require.NoError(t, err)
 	gittest.RequireTree(t, cfg, repoPath, headCommit.Id, nil)
@@ -822,6 +847,7 @@ func TestServer_UserRevert_failedDueToPreReceiveError(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
+		featureflag.ReturnStructuredErrorsInUserRevert,
 	).Run(t, testServerUserRevertFailedDueToPreReceiveError)
 }
 
@@ -859,9 +885,19 @@ func testServerUserRevertFailedDueToPreReceiveError(t *testing.T, ctx context.Co
 		t.Run(hookName, func(t *testing.T) {
 			gittest.WriteCustomHook(t, repoPath, hookName, hookContent)
 
-			response, err := client.UserRevert(ctx, request)
-			require.NoError(t, err)
-			require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId)
+			if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+				_, err := client.UserRevert(ctx, request)
+				actualStatus, _ := status.FromError(err)
+				require.Equal(t, actualStatus.Code(), codes.PermissionDenied)
+				require.Equal(t, actualStatus.Message(), "revert: custom hook error")
+				revertError, ok := actualStatus.Details()[0].(*gitalypb.UserRevertError)
+				require.True(t, ok)
+				require.Contains(t, revertError.GetCustomHook().String(), "GL_ID="+gittest.TestUser.GlId)
+			} else {
+				response, err := client.UserRevert(ctx, request)
+				require.NoError(t, err)
+				require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId)
+			}
 		})
 	}
 }
@@ -871,6 +907,7 @@ func TestServer_UserRevert_failedDueToCreateTreeErrorConflict(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
+		featureflag.ReturnStructuredErrorsInUserRevert,
 	).Run(t, testServerUserRevertFailedDueToCreateTreeErrorConflict)
 }
 
@@ -917,10 +954,20 @@ func testServerUserRevertFailedDueToCreateTreeErrorConflict(t *testing.T, ctx co
 		Message:    []byte("Reverting " + revertedCommit.Id),
 	}
 
-	response, err := client.UserRevert(ctx, request)
-	require.NoError(t, err)
-	require.NotEmpty(t, response.CreateTreeError)
-	require.Equal(t, gitalypb.UserRevertResponse_CONFLICT, response.CreateTreeErrorCode)
+	if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+		_, err = client.UserRevert(ctx, request)
+		actualStatus, _ := status.FromError(err)
+		require.Equal(t, actualStatus.Code(), codes.FailedPrecondition)
+		require.Equal(t, actualStatus.Message(), "revert: there are conflicting files")
+		revertError, ok := actualStatus.Details()[0].(*gitalypb.UserRevertError)
+		require.True(t, ok)
+		require.NotNil(t, revertError.GetMergeConflict())
+	} else {
+		response, err := client.UserRevert(ctx, request)
+		require.NoError(t, err)
+		require.NotEmpty(t, response.CreateTreeError)
+		require.Equal(t, gitalypb.UserRevertResponse_CONFLICT, response.CreateTreeErrorCode)
+	}
 }
 
 func TestServer_UserRevert_failedDueToCreateTreeErrorEmpty(t *testing.T) {
@@ -928,6 +975,7 @@ func TestServer_UserRevert_failedDueToCreateTreeErrorEmpty(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
+		featureflag.ReturnStructuredErrorsInUserRevert,
 	).Run(t, testServerUserRevertFailedDueToCreateTreeErrorEmpty)
 }
 
@@ -989,13 +1037,25 @@ func testServerUserRevertFailedDueToCreateTreeErrorEmpty(t *testing.T, ctx conte
 
 	response, err := client.UserRevert(ctx, request)
 	require.NoError(t, err)
-	require.Empty(t, response.CreateTreeError)
-	require.Equal(t, gitalypb.UserRevertResponse_NONE, response.CreateTreeErrorCode)
 
-	response, err = client.UserRevert(ctx, request)
-	require.NoError(t, err)
-	require.NotEmpty(t, response.CreateTreeError)
-	require.Equal(t, gitalypb.UserRevertResponse_EMPTY, response.CreateTreeErrorCode)
+	if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+		_, err = client.UserRevert(ctx, request)
+
+		expectedError := structerr.NewFailedPrecondition("revert: could not apply because the result was empty").WithDetail(
+			&gitalypb.UserRevertError{
+				Error: &gitalypb.UserRevertError_ChangesAlreadyApplied{},
+			})
+		testhelper.RequireGrpcError(t, expectedError, err)
+	} else {
+		require.NoError(t, err)
+		require.Empty(t, response.CreateTreeError)
+		require.Equal(t, gitalypb.UserRevertResponse_NONE, response.CreateTreeErrorCode)
+
+		response, err = client.UserRevert(ctx, request)
+		require.NoError(t, err)
+		require.NotEmpty(t, response.CreateTreeError)
+		require.Equal(t, gitalypb.UserRevertResponse_EMPTY, response.CreateTreeErrorCode)
+	}
 }
 
 func TestServer_UserRevert_failedDueToCommitError(t *testing.T) {
@@ -1003,6 +1063,7 @@ func TestServer_UserRevert_failedDueToCommitError(t *testing.T) {
 
 	testhelper.NewFeatureSets(
 		featureflag.GPGSigning,
+		featureflag.ReturnStructuredErrorsInUserRevert,
 	).Run(t, testServerUserRevertFailedDueToCommitError)
 }
 
@@ -1045,8 +1106,17 @@ func testServerUserRevertFailedDueToCommitError(t *testing.T, ctx context.Contex
 		Message:         []byte("Reverting " + revertedCommit.Id),
 		StartBranchName: []byte(sourceBranch),
 	}
-
 	response, err := client.UserRevert(ctx, request)
-	require.NoError(t, err)
-	require.Equal(t, "Branch diverged", response.CommitError)
+
+	if featureflag.ReturnStructuredErrorsInUserRevert.IsEnabled(ctx) {
+		actualStatus, _ := status.FromError(err)
+		require.Equal(t, actualStatus.Code(), codes.FailedPrecondition)
+		require.Equal(t, actualStatus.Message(), "revert: branch diverged")
+		revertError, ok := actualStatus.Details()[0].(*gitalypb.UserRevertError)
+		require.True(t, ok)
+		require.NotNil(t, revertError.GetNotAncestor())
+	} else {
+		require.NoError(t, err)
+		require.Equal(t, "Branch diverged", response.CommitError)
+	}
 }