operations: Return error from UserSquash when resolving revisions fails

We do not return an error in case resolving either the start or end
revision of a UserSquash call fails. This makes us blind for the error
conditions in this RPC, which is in turn creating problems in Praefect
setups where we have to rely on errors in order to decide whether we
have to create replication jobs or not.

Return structured errors in case the error condition triggers such that
callers can tell what kind of error they have been hitting.

Changelog: changed

3 files changed, 100 insertions, 7 deletions

diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go
index 9e778f55d..270fdd5d3 100644
--- a/internal/gitaly/service/operations/squash.go
+++ b/internal/gitaly/service/operations/squash.go
@@ -11,6 +11,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -110,6 +111,24 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 	// all parents of the start commit.
 	startCommit, err := repo.ResolveRevision(ctx, git.Revision(req.GetStartSha()+"^{commit}"))
 	if err != nil {
+		if featureflag.UserSquashImprovedErrorHandling.IsEnabled(ctx) {
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrInvalidArgumentf("resolving start revision: %w", err),
+				&gitalypb.UserSquashError{
+					Error: &gitalypb.UserSquashError_ResolveRevision{
+						ResolveRevision: &gitalypb.ResolveRevisionError{
+							Revision: []byte(req.GetStartSha()),
+						},
+					},
+				},
+			)
+			if err != nil {
+				return "", helper.ErrInternalf("error details: %w", err)
+			}
+
+			return "", detailedErr
+		}
+
 		return "", fmt.Errorf("cannot resolve start commit: %w", gitError{
 			// This error is simply for backwards compatibility. We should just
 			// return the plain error eventually.
@@ -121,6 +140,24 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 	// And we need to take the tree of the end commit. This tree already is the result
 	endCommit, err := repo.ResolveRevision(ctx, git.Revision(req.GetEndSha()+"^{commit}"))
 	if err != nil {
+		if featureflag.UserSquashImprovedErrorHandling.IsEnabled(ctx) {
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrInvalidArgumentf("resolving end revision: %w", err),
+				&gitalypb.UserSquashError{
+					Error: &gitalypb.UserSquashError_ResolveRevision{
+						ResolveRevision: &gitalypb.ResolveRevisionError{
+							Revision: []byte(req.GetEndSha()),
+						},
+					},
+				},
+			)
+			if err != nil {
+				return "", helper.ErrInternalf("error details: %w", err)
+			}
+
+			return "", detailedErr
+		}
+
 		return "", fmt.Errorf("cannot resolve end commit's tree: %w", gitError{
 			// This error is simply for backwards compatibility. We should just
 			// return the plain error eventually.
diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go
index b758ba4aa..fae48985a 100644
--- a/internal/gitaly/service/operations/squash_test.go
+++ b/internal/gitaly/service/operations/squash_test.go
@@ -1,6 +1,7 @@
 package operations
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -14,6 +15,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -554,8 +556,11 @@ func TestUserSquash_ancestry(t *testing.T) {
 }
 
 func TestUserSquash_gitError(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.UserSquashImprovedErrorHandling).Run(t, testUserSquashGitError)
+}
+
+func testUserSquashGitError(t *testing.T, ctx context.Context) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
@@ -575,9 +580,31 @@ func TestUserSquash_gitError(t *testing.T) {
 				StartSha:      "doesntexisting",
 				EndSha:        endSha,
 			},
-			expectedResponse: &gitalypb.UserSquashResponse{
-				GitError: "fatal: ambiguous argument 'doesntexisting...54cec5282aa9f21856362fe321c800c236a61615': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n",
-			},
+			expectedErr: func() error {
+				if featureflag.UserSquashImprovedErrorHandling.IsEnabled(ctx) {
+					return errWithDetails(t,
+						helper.ErrInvalidArgumentf("resolving start revision: reference not found"),
+						&gitalypb.UserSquashError{
+							Error: &gitalypb.UserSquashError_ResolveRevision{
+								ResolveRevision: &gitalypb.ResolveRevisionError{
+									Revision: []byte("doesntexisting"),
+								},
+							},
+						},
+					)
+				}
+
+				return nil
+			}(),
+			expectedResponse: func() *gitalypb.UserSquashResponse {
+				if featureflag.UserSquashImprovedErrorHandling.IsEnabled(ctx) {
+					return nil
+				}
+
+				return &gitalypb.UserSquashResponse{
+					GitError: "fatal: ambiguous argument 'doesntexisting...54cec5282aa9f21856362fe321c800c236a61615': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n",
+				}
+			}(),
 		},
 		{
 			desc: "not existing end SHA",
@@ -589,9 +616,31 @@ func TestUserSquash_gitError(t *testing.T) {
 				StartSha:      startSha,
 				EndSha:        "doesntexisting",
 			},
-			expectedResponse: &gitalypb.UserSquashResponse{
-				GitError: "fatal: ambiguous argument 'b83d6e391c22777fca1ed3012fce84f633d7fed0...doesntexisting': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n",
-			},
+			expectedErr: func() error {
+				if featureflag.UserSquashImprovedErrorHandling.IsEnabled(ctx) {
+					return errWithDetails(t,
+						helper.ErrInvalidArgumentf("resolving end revision: reference not found"),
+						&gitalypb.UserSquashError{
+							Error: &gitalypb.UserSquashError_ResolveRevision{
+								ResolveRevision: &gitalypb.ResolveRevisionError{
+									Revision: []byte("doesntexisting"),
+								},
+							},
+						},
+					)
+				}
+
+				return nil
+			}(),
+			expectedResponse: func() *gitalypb.UserSquashResponse {
+				if featureflag.UserSquashImprovedErrorHandling.IsEnabled(ctx) {
+					return nil
+				}
+
+				return &gitalypb.UserSquashResponse{
+					GitError: "fatal: ambiguous argument 'b83d6e391c22777fca1ed3012fce84f633d7fed0...doesntexisting': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n",
+				}
+			}(),
 		},
 		{
 			desc: "user has no name set",
diff --git a/internal/metadata/featureflag/ff_user_squash_improved_error_handling.go b/internal/metadata/featureflag/ff_user_squash_improved_error_handling.go
new file mode 100644
index 000000000..e749e6f84
--- /dev/null
+++ b/internal/metadata/featureflag/ff_user_squash_improved_error_handling.go
@@ -0,0 +1,7 @@
+package featureflag
+
+// UserSquashImprovedErrorHandling enables proper error handling in the UserSquash 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 UserSquashImprovedErrorHandling = NewFeatureFlag("user_squash_improved_error_handling", false)