Merge branch 'pks-user-squash-structured-errors' into 'master'

operations: Implement structured errors for UserSquash

See merge request gitlab-org/gitaly!4374

7 files changed, 177 insertions, 16 deletions

diff --git a/cmd/gitaly-git2go/rebase.go b/cmd/gitaly-git2go/rebase.go
index 8d6d44010..0ca365ff4 100644
--- a/cmd/gitaly-git2go/rebase.go
+++ b/cmd/gitaly-git2go/rebase.go
@@ -149,6 +149,22 @@ func (cmd *rebaseSubcommand) rebase(ctx context.Context, request *git2go.RebaseC
 		}
 
 		if err := rebase.Commit(op.Id, nil, &committer, commit.Message()); err != nil {
+			if git.IsErrorCode(err, git.ErrorCodeUnmerged) {
+				index, err := rebase.InmemoryIndex()
+				if err != nil {
+					return "", fmt.Errorf("getting conflicting index: %w", err)
+				}
+
+				conflictingFiles, err := getConflictingFiles(index)
+				if err != nil {
+					return "", fmt.Errorf("getting conflicting files: %w", err)
+				}
+
+				return "", fmt.Errorf("commit %q: %w", op.Id.String(), git2go.ConflictingFilesError{
+					ConflictingFiles: conflictingFiles,
+				})
+			}
+
 			// If the commit has already been applied on the target branch then we can
 			// skip it if we were told to.
 			if request.SkipEmptyCommits && git.IsErrorCode(err, git.ErrorCodeApplied) {
diff --git a/cmd/gitaly-git2go/rebase_test.go b/cmd/gitaly-git2go/rebase_test.go
index 8aa7e013f..5c6e84299 100644
--- a/cmd/gitaly-git2go/rebase_test.go
+++ b/cmd/gitaly-git2go/rebase_test.go
@@ -156,7 +156,7 @@ func TestRebase_rebase(t *testing.T) {
 		{
 			desc:        "Rebase with conflict",
 			branch:      "rebase-encoding-failure-trigger",
-			expectedErr: "rebase: commit \"eb8f5fb9523b868cef583e09d4bf70b99d2dd404\": conflicts have not been resolved",
+			expectedErr: "rebase: commit \"eb8f5fb9523b868cef583e09d4bf70b99d2dd404\": there are conflicting files",
 		},
 		{
 			desc:        "Orphaned branch",
diff --git a/go.mod b/go.mod
index 9d4e391d5..5a506cea6 100644
--- a/go.mod
+++ b/go.mod
@@ -27,7 +27,7 @@ require (
 	github.com/jackc/pgtype v1.9.1
 	github.com/jackc/pgx/v4 v4.14.1
 	github.com/kelseyhightower/envconfig v1.3.0
-	github.com/libgit2/git2go/v33 v33.0.6
+	github.com/libgit2/git2go/v33 v33.0.9
 	github.com/olekukonko/tablewriter v0.0.2
 	github.com/opencontainers/runtime-spec v1.0.2
 	github.com/opentracing/opentracing-go v1.2.0
diff --git a/go.sum b/go.sum
index af61cd398..bd266ef1e 100644
--- a/go.sum
+++ b/go.sum
@@ -632,8 +632,8 @@ github.com/lib/pq v1.10.2/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o=
 github.com/libgit2/git2go v0.0.0-20190104134018-ecaeb7a21d47 h1:HDt7WT3kpXSHq4mlOuLzgXH9LeOK1qlhyFdKIAzxxeM=
 github.com/libgit2/git2go v0.0.0-20190104134018-ecaeb7a21d47/go.mod h1:4bKN42efkbNYMZlvDfxGDxzl066GhpvIircZDsm8Y+Y=
 github.com/libgit2/git2go/v31 v31.4.12/go.mod h1:c/rkJcBcUFx6wHaT++UwNpKvIsmPNqCeQ/vzO4DrEec=
-github.com/libgit2/git2go/v33 v33.0.6 h1:F//bA3/pgSTVq2hLNahhnof9NxyCzFF/c3MB6lb93Qo=
-github.com/libgit2/git2go/v33 v33.0.6/go.mod h1:KdpqkU+6+++4oHna/MIOgx4GCQ92IPCdpVRMRI80J+4=
+github.com/libgit2/git2go/v33 v33.0.9 h1:4ch2DJed6IhJO28BEohkUoGvxLsRzUjxljoNFJ6/O78=
+github.com/libgit2/git2go/v33 v33.0.9/go.mod h1:KdpqkU+6+++4oHna/MIOgx4GCQ92IPCdpVRMRI80J+4=
 github.com/lightstep/lightstep-tracer-common/golang/gogo v0.0.0-20190605223551-bc2310a04743/go.mod h1:qklhhLq1aX+mtWk9cPHPzaBjWImj5ULL6C7HFJtXQMM=
 github.com/lightstep/lightstep-tracer-common/golang/gogo v0.0.0-20200305213919-a88bf8de3718/go.mod h1:qklhhLq1aX+mtWk9cPHPzaBjWImj5ULL6C7HFJtXQMM=
 github.com/lightstep/lightstep-tracer-common/golang/gogo v0.0.0-20210210170715-a8dfcb80d3a7 h1:YjW+hUb8Fh2S58z4av4t/0cBMK/Q0aP48RocCFsC8yI=
diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go
index 9e778f55d..4a942a15e 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.
@@ -146,6 +183,35 @@ 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.ErrInternalf("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
+			}
+
+			return "", helper.ErrInternalf("rebasing commits: %w", err)
+		}
+
 		return "", fmt.Errorf("rebasing end onto start commit: %w", gitError{
 			Err:    err,
 			ErrMsg: err.Error(),
@@ -185,6 +251,10 @@ 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(),
diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go
index b758ba4aa..668503535 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"
@@ -484,8 +486,11 @@ func TestUserSquash_validation(t *testing.T) {
 }
 
 func TestUserSquash_conflicts(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.UserSquashImprovedErrorHandling).Run(t, testUserSquashConflicts)
+}
+
+func testUserSquashConflicts(t *testing.T, ctx context.Context) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
@@ -512,11 +517,27 @@ func TestUserSquash_conflicts(t *testing.T) {
 		StartSha:      theirs.String(),
 		EndSha:        ours.String(),
 	})
-	require.NoError(t, err)
 
-	testhelper.ProtoEqual(t, &gitalypb.UserSquashResponse{
-		GitError: fmt.Sprintf("rebase: commit %q: conflicts have not been resolved", ours),
-	}, response)
+	if featureflag.UserSquashImprovedErrorHandling.IsEnabled(ctx) {
+		testhelper.RequireGrpcError(t, errWithDetails(t,
+			helper.ErrInternalf("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)
+	}
 }
 
 func TestUserSquash_ancestry(t *testing.T) {
@@ -554,8 +575,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 +599,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 +635,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)