repository: Structured errors for UserRebaseConfirmablejc-user-rebase-confirmable-structured-errors

6 files changed, 87 insertions, 2 deletions

diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index ccf1bd69e..db133eeb3 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -1239,6 +1239,7 @@ func TestUserMergeToRef_ignoreHooksRequest(t *testing.T) {
 
 			resp, err := client.UserMergeToRef(ctx, request)
 			require.NoError(t, err)
+			//nolint:staticcheck
 			require.Empty(t, resp.PreReceiveError)
 		})
 	}
diff --git a/internal/gitaly/service/operations/rebase.go b/internal/gitaly/service/operations/rebase.go
index 30c4438cb..03671280c 100644
--- a/internal/gitaly/service/operations/rebase.go
+++ b/internal/gitaly/service/operations/rebase.go
@@ -9,6 +9,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -67,6 +68,34 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba
 		SkipEmptyCommits: true,
 	})
 	if err != nil {
+		if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) {
+			var conflictErr git2go.ConflictingFilesError
+			if errors.As(err, &conflictErr) {
+				conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
+				for _, conflictingFile := range conflictErr.ConflictingFiles {
+					conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
+				}
+
+				detailedErr, err := helper.ErrWithDetails(
+					helper.ErrInternalf("rebasing commits: %w", err),
+					&gitalypb.UserRebaseConfirmableError{
+						Error: &gitalypb.UserRebaseConfirmableError_RebaseConflict{
+							RebaseConflict: &gitalypb.MergeConflictError{
+								ConflictingFiles: conflictingFiles,
+							},
+						},
+					},
+				)
+				if err != nil {
+					return helper.ErrInternalf("error details: %w", err)
+				}
+
+				return detailedErr
+			}
+
+			return helper.ErrInternalf("rebasing commits: %w", err)
+		}
+
 		return stream.Send(&gitalypb.UserRebaseConfirmableResponse{
 			GitError: err.Error(),
 		})
@@ -100,6 +129,21 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba
 		header.GitPushOptions...); err != nil {
 		switch {
 		case errors.As(err, &updateref.HookError{}):
+			if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) {
+				detailedErr, err := helper.ErrWithDetails(
+					helper.ErrInternalf("rebasing commits: %w", err),
+					&gitalypb.UserRebaseConfirmableError{
+						Error: &gitalypb.UserRebaseConfirmableError_PreReceive{
+							PreReceive: err.Error(),
+						},
+					},
+				)
+				if err != nil {
+					return helper.ErrInternalf("error details: %w", err)
+				}
+
+				return detailedErr
+			}
 			return stream.Send(&gitalypb.UserRebaseConfirmableResponse{
 				PreReceiveError: err.Error(),
 			})
diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go
index 1d388ded7..57ed35b63 100644
--- a/internal/gitaly/service/operations/rebase_test.go
+++ b/internal/gitaly/service/operations/rebase_test.go
@@ -1,6 +1,7 @@
 package operations
 
 import (
+	"context"
 	"fmt"
 	"io"
 	"path/filepath"
@@ -15,6 +16,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
@@ -561,9 +563,13 @@ func TestFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T) {
 	}
 }
 
-func TestFailedUserRebaseConfirmableDueToGitError(t *testing.T) {
+func TestUserRebaseConfirmable_gitError(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.UserRebaseConfirmableImprovedErrorHandling).
+		Run(t, testFailedUserRebaseConfirmableDueToGitError)
+}
+
+func testFailedUserRebaseConfirmableDueToGitError(t *testing.T, ctx context.Context) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go
index 9b57b63b2..62240417b 100644
--- a/internal/gitaly/service/operations/revert.go
+++ b/internal/gitaly/service/operations/revert.go
@@ -11,6 +11,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -152,6 +153,24 @@ func (s *Server) fetchStartRevision(
 
 	startRevision, err := remoteRepo.ResolveRevision(ctx, git.Revision(fmt.Sprintf("%s^{commit}", startBranchName)))
 	if err != nil {
+		if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) {
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrInvalidArgumentf("resolving branch on remote: %w", err),
+				&gitalypb.UserRebaseConfirmableError{
+					Error: &gitalypb.UserRebaseConfirmableError_ResolveRevision{
+						ResolveRevision: &gitalypb.ResolveRevisionError{
+							Revision: startBranchName,
+						},
+					},
+				},
+			)
+			if err != nil {
+				return "", helper.ErrInternalf("error details: %w", err)
+			}
+
+			return "", detailedErr
+		}
+
 		return "", helper.ErrInvalidArgumentf("resolve start ref: %w", err)
 	}
 
diff --git a/internal/metadata/featureflag/ff_user_rebase_confirmable_improved_error_handling.go b/internal/metadata/featureflag/ff_user_rebase_confirmable_improved_error_handling.go
new file mode 100644
index 000000000..b66a0e944
--- /dev/null
+++ b/internal/metadata/featureflag/ff_user_rebase_confirmable_improved_error_handling.go
@@ -0,0 +1,7 @@
+package featureflag
+
+// UserRebaseConfirmableImprovedErrorHandling enables proper error handling in the UserRebaseConfirmable RPC. When this
+// flag is disabled many error cases were returning successfully with an error message embedded in
+// the response. With this flag enabled, this is converted to return real gRPC errors with
+// structured errors.
+var UserRebaseConfirmableImprovedErrorHandling = NewFeatureFlag("user_rebase_confirmable_improved_error_handling", false)
diff --git a/ruby/proto/gitaly/operations_pb.rb b/ruby/proto/gitaly/operations_pb.rb
index e5a38e03c..c1d4cc2f8 100644
--- a/ruby/proto/gitaly/operations_pb.rb
+++ b/ruby/proto/gitaly/operations_pb.rb
@@ -238,6 +238,13 @@ Google::Protobuf::DescriptorPool.generated_pool.build do
       optional :squash_sha, :string, 1
       optional :git_error, :string, 3
     end
+    add_message "gitaly.UserRebaseConfirmableError" do
+      oneof :error do
+        optional :resolve_revision, :message, 1, "gitaly.ResolveRevisionError"
+        optional :rebase_conflict, :message, 2, "gitaly.MergeConflictError"
+        optional :pre_receive, :string, 3
+      end
+    end
     add_message "gitaly.UserSquashError" do
       oneof :error do
         optional :resolve_revision, :message, 1, "gitaly.ResolveRevisionError"
@@ -312,6 +319,7 @@ module Gitaly
   UserRebaseConfirmableResponse = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("gitaly.UserRebaseConfirmableResponse").msgclass
   UserSquashRequest = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("gitaly.UserSquashRequest").msgclass
   UserSquashResponse = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("gitaly.UserSquashResponse").msgclass
+  UserRebaseConfirmableError = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("gitaly.UserRebaseConfirmableError").msgclass
   UserSquashError = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("gitaly.UserSquashError").msgclass
   UserApplyPatchRequest = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("gitaly.UserApplyPatchRequest").msgclass
   UserApplyPatchRequest::Header = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("gitaly.UserApplyPatchRequest.Header").msgclass