operations: Always use structured errors for UserSquash

With d88d29185 (operations: Return error from UserSquash when resolving
revisions fails, 2022-03-01), we have converted UserSquash to return
structured errors in some cases where it previously returned no errors
at all. Instead, the RPC used to embed the error in the response struct
via the `GitError` field. This has made us blind for actual issues which
happen in production, and furthermore it trips Praefect's logic which
decides whether we need to create repliction jobs or not.

Rails has been adapted already in c7bcaadf193 (gitaly_client: Handle
detailed errors for UserSquash, 2022-03-02), and the changes have been
rolled out to production. While we required one follow-up fix to the new
error codes we returned in 6c63998ad (operations: Fix wrong error code
when UserSquash conflicts, 2022-03-11), the rollout has otherwise been
successful.

Remove the feature flag altogether and deprecated the `GitError` field,
which isn't used anymore.

Changelog: changed

3 files changed, 88 insertions, 164 deletions

diff --git a/internal/gitaly/service/operations/apply_patch.go b/internal/gitaly/service/operations/apply_patch.go
index 6c58d6bf0..f87cec969 100644
--- a/internal/gitaly/service/operations/apply_patch.go
+++ b/internal/gitaly/service/operations/apply_patch.go
@@ -22,6 +22,17 @@ import (
 
 var errNoDefaultBranch = errors.New("no default branch")
 
+type gitError struct {
+	// ErrMsg error message from 'git' executable if any.
+	ErrMsg string
+	// Err is an error that happened during rebase process.
+	Err error
+}
+
+func (er gitError) Error() string {
+	return er.ErrMsg + ": " + er.Err.Error()
+}
+
 //nolint: revive,stylecheck // This is unintentionally missing documentation.
 func (s *Server) UserApplyPatch(stream gitalypb.OperationService_UserApplyPatchServer) error {
 	firstRequest, err := stream.Recv()
diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go
index 096dfc68b..1129d2658 100644
--- a/internal/gitaly/service/operations/squash.go
+++ b/internal/gitaly/service/operations/squash.go
@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 
-	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/quarantine"
@@ -21,7 +20,6 @@ import (
 
 const (
 	gitlabWorktreesSubDir = "gitlab-worktree"
-	ambiguousArgumentFmt  = "fatal: ambiguous argument '%s...%s': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n"
 )
 
 // UserSquash collapses a range of commits identified via a start and end revision into a single
@@ -33,15 +31,6 @@ func (s *Server) UserSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 
 	sha, err := s.userSquash(ctx, req)
 	if err != nil {
-		var gitErr gitError
-		if errors.As(err, &gitErr) {
-			if gitErr.ErrMsg != "" {
-				// we log an actual error as it would be lost otherwise (it is not sent back to the client)
-				ctxlogrus.Extract(ctx).WithError(err).Error("user squash")
-				return &gitalypb.UserSquashResponse{GitError: gitErr.ErrMsg}, nil
-			}
-		}
-
 		return nil, helper.ErrInternal(err)
 	}
 
@@ -92,17 +81,6 @@ func validateUserSquashRequest(req *gitalypb.UserSquashRequest) error {
 	return nil
 }
 
-type gitError struct {
-	// ErrMsg error message from 'git' executable if any.
-	ErrMsg string
-	// Err is an error that happened during rebase process.
-	Err error
-}
-
-func (er gitError) Error() string {
-	return er.ErrMsg + ": " + er.Err.Error()
-}
-
 func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest) (string, error) {
 	repo := s.localrepo(req.GetRepository())
 	repoPath, err := repo.Path()
@@ -131,59 +109,41 @@ 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()),
-						},
+		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
+			},
+		)
+		if err != nil {
+			return "", helper.ErrInternalf("error details: %w", err)
 		}
 
-		return "", fmt.Errorf("cannot resolve start commit: %w", gitError{
-			// This error is simply for backwards compatibility. We should just
-			// return the plain error eventually.
-			Err:    err,
-			ErrMsg: fmt.Sprintf(ambiguousArgumentFmt, req.GetStartSha(), req.GetEndSha()),
-		})
+		return "", detailedErr
 	}
 
 	// 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()),
-						},
+		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
+			},
+		)
+		if err != nil {
+			return "", helper.ErrInternalf("error details: %w", err)
 		}
 
-		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.
-			Err:    err,
-			ErrMsg: fmt.Sprintf(ambiguousArgumentFmt, req.GetStartSha(), req.GetEndSha()),
-		})
+		return "", detailedErr
 	}
 
 	commitDate, err := dateFromProto(req)
@@ -203,39 +163,32 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 		SkipEmptyCommits: true,
 	})
 	if err != nil {
-		if featureflag.UserSquashImprovedErrorHandling.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.ErrFailedPreconditionf("rebasing commits: %w", err),
-					&gitalypb.UserSquashError{
-						Error: &gitalypb.UserSquashError_RebaseConflict{
-							RebaseConflict: &gitalypb.MergeConflictError{
-								ConflictingFiles: conflictingFiles,
-							},
+		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.ErrFailedPreconditionf("rebasing commits: %w", err),
+				&gitalypb.UserSquashError{
+					Error: &gitalypb.UserSquashError_RebaseConflict{
+						RebaseConflict: &gitalypb.MergeConflictError{
+							ConflictingFiles: conflictingFiles,
 						},
 					},
-				)
-				if err != nil {
-					return "", helper.ErrInternalf("error details: %w", err)
-				}
-
-				return "", detailedErr
+				},
+			)
+			if err != nil {
+				return "", helper.ErrInternalf("error details: %w", err)
 			}
 
-			return "", helper.ErrInternalf("rebasing commits: %w", err)
+			return "", detailedErr
 		}
 
-		return "", fmt.Errorf("rebasing end onto start commit: %w", gitError{
-			Err:    err,
-			ErrMsg: err.Error(),
-		})
+		return "", helper.ErrInternalf("rebasing commits: %w", err)
 	}
 
 	treeID, err := repo.ResolveRevision(ctx, rebasedCommitID.Revision()+"^{tree}")
@@ -271,14 +224,7 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 			treeID.String(),
 		},
 	}, git.WithStdout(&stdout), git.WithStderr(&stderr), git.WithEnv(commitEnv...)); err != nil {
-		if featureflag.UserSquashImprovedErrorHandling.IsEnabled(ctx) {
-			return "", helper.ErrInternalf("creating squashed commit: %w", err)
-		}
-
-		return "", fmt.Errorf("creating commit: %w", gitError{
-			Err:    err,
-			ErrMsg: stderr.String(),
-		})
+		return "", helper.ErrInternalf("creating squashed commit: %w", err)
 	}
 
 	commitID := text.ChompBytes(stdout.Bytes())
diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go
index f20f12fe5..f412fa335 100644
--- a/internal/gitaly/service/operations/squash_test.go
+++ b/internal/gitaly/service/operations/squash_test.go
@@ -78,6 +78,7 @@ func testUserSquashSuccessful(t *testing.T, ctx context.Context) {
 
 			response, err := client.UserSquash(ctx, request)
 			require.NoError(t, err)
+			//nolint:staticcheck
 			require.Empty(t, response.GetGitError())
 
 			commit, err := repo.ReadCommit(ctx, git.Revision(response.SquashSha))
@@ -250,6 +251,7 @@ func testUserSquashStableID(t *testing.T, ctx context.Context) {
 		Timestamp:     &timestamppb.Timestamp{Seconds: 1234512345},
 	})
 	require.NoError(t, err)
+	//nolint:staticcheck
 	require.Empty(t, response.GetGitError())
 
 	commit, err := repo.ReadCommit(ctx, git.Revision(response.SquashSha))
@@ -314,6 +316,7 @@ func testUserSquashThreeWayMerge(t *testing.T, ctx context.Context) {
 
 	response, err := client.UserSquash(ctx, request)
 	require.NoError(t, err)
+	//nolint:staticcheck
 	require.Empty(t, response.GetGitError())
 
 	commit, err := repo.ReadCommit(ctx, git.Revision(response.SquashSha))
@@ -351,6 +354,7 @@ func testUserSquashSplitIndex(t *testing.T, ctx context.Context) {
 
 	response, err := client.UserSquash(ctx, request)
 	require.NoError(t, err)
+	//nolint:staticcheck
 	require.Empty(t, response.GetGitError())
 	require.False(t, ensureSplitIndexExists(t, cfg, repoPath))
 }
@@ -403,6 +407,7 @@ func testUserSquashRenames(t *testing.T, ctx context.Context) {
 
 	response, err := client.UserSquash(ctx, request)
 	require.NoError(t, err)
+	//nolint:staticcheck
 	require.Empty(t, response.GetGitError())
 
 	commit, err := repo.ReadCommit(ctx, git.Revision(response.SquashSha))
@@ -440,6 +445,7 @@ func testUserSquashMissingFileOnTargetBranch(t *testing.T, ctx context.Context)
 
 	response, err := client.UserSquash(ctx, request)
 	require.NoError(t, err)
+	//nolint:staticcheck
 	require.Empty(t, response.GetGitError())
 }
 
@@ -648,7 +654,6 @@ func TestUserSquash_validation(t *testing.T) {
 
 func TestUserSquash_conflicts(t *testing.T) {
 	testhelper.NewFeatureSets(
-		featureflag.UserSquashImprovedErrorHandling,
 		featureflag.UserSquashQuarantinedVoting,
 	).Run(t, testUserSquashConflicts)
 }
@@ -682,26 +687,19 @@ func testUserSquashConflicts(t *testing.T, ctx context.Context) {
 		EndSha:        ours.String(),
 	})
 
-	if featureflag.UserSquashImprovedErrorHandling.IsEnabled(ctx) {
-		testhelper.RequireGrpcError(t, errWithDetails(t,
-			helper.ErrFailedPreconditionf("rebasing commits: rebase: commit %q: there are conflicting files", ours),
-			&gitalypb.UserSquashError{
-				Error: &gitalypb.UserSquashError_RebaseConflict{
-					RebaseConflict: &gitalypb.MergeConflictError{
-						ConflictingFiles: [][]byte{
-							[]byte("b"),
-						},
+	testhelper.RequireGrpcError(t, errWithDetails(t,
+		helper.ErrFailedPreconditionf("rebasing commits: rebase: commit %q: there are conflicting files", ours),
+		&gitalypb.UserSquashError{
+			Error: &gitalypb.UserSquashError_RebaseConflict{
+				RebaseConflict: &gitalypb.MergeConflictError{
+					ConflictingFiles: [][]byte{
+						[]byte("b"),
 					},
 				},
 			},
-		), err)
-		require.Nil(t, response)
-	} else {
-		require.NoError(t, err)
-		testhelper.ProtoEqual(t, &gitalypb.UserSquashResponse{
-			GitError: fmt.Sprintf("rebase: commit %q: there are conflicting files", ours),
-		}, response)
-	}
+		},
+	), err)
+	require.Nil(t, response)
 }
 
 func TestUserSquash_ancestry(t *testing.T) {
@@ -745,7 +743,6 @@ func testUserSquashAncestry(t *testing.T, ctx context.Context) {
 func TestUserSquash_gitError(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.UserSquashImprovedErrorHandling,
 		featureflag.UserSquashQuarantinedVoting,
 	).Run(t, testUserSquashGitError)
 }
@@ -771,31 +768,16 @@ func testUserSquashGitError(t *testing.T, ctx context.Context) {
 				StartSha:      "doesntexisting",
 				EndSha:        endSha,
 			},
-			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"),
-								},
-							},
+			expectedErr: 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",
@@ -807,31 +789,16 @@ func testUserSquashGitError(t *testing.T, ctx context.Context) {
 				StartSha:      startSha,
 				EndSha:        "doesntexisting",
 			},
-			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"),
-								},
-							},
+			expectedErr: 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",