Merge branch 'jc-cherry-pick-list-conflicting-files' into 'master'

operations: Convert UserCherryPick to return structured errors

See merge request gitlab-org/gitaly!4585

5 files changed, 225 insertions, 30 deletions

diff --git a/cmd/gitaly-git2go-v15/cherry_pick.go b/cmd/gitaly-git2go-v15/cherry_pick.go
index d94748363..0b991a812 100644
--- a/cmd/gitaly-git2go-v15/cherry_pick.go
+++ b/cmd/gitaly-git2go-v15/cherry_pick.go
@@ -13,6 +13,7 @@ import (
 	git "github.com/libgit2/git2go/v33"
 	"gitlab.com/gitlab-org/gitaly/v15/cmd/gitaly-git2go-v15/git2goutil"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git2go"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 )
 
 type cherryPickSubcommand struct{}
@@ -93,6 +94,17 @@ func (cmd *cherryPickSubcommand) cherryPick(ctx context.Context, r *git2go.Cherr
 	}
 
 	if index.HasConflicts() {
+		if featureflag.CherryPickStructuredErrors.IsEnabled(ctx) {
+			conflictingFiles, err := getConflictingFiles(index)
+			if err != nil {
+				return "", fmt.Errorf("getting conflicting files: %w", err)
+			}
+
+			return "", git2go.ConflictingFilesError{
+				ConflictingFiles: conflictingFiles,
+			}
+		}
+
 		return "", git2go.HasConflictsError{}
 	}
 
diff --git a/cmd/gitaly-git2go-v15/cherry_pick_test.go b/cmd/gitaly-git2go-v15/cherry_pick_test.go
index 4e4478317..cb4b3329f 100644
--- a/cmd/gitaly-git2go-v15/cherry_pick_test.go
+++ b/cmd/gitaly-git2go-v15/cherry_pick_test.go
@@ -4,7 +4,7 @@
 package main
 
 import (
-	"errors"
+	"context"
 	"testing"
 	"time"
 
@@ -14,6 +14,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/cmd/gitaly-git2go-v15/git2goutil"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git2go"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 )
@@ -116,8 +117,8 @@ func TestCherryPick(t *testing.T) {
 			commit: []gittest.TreeEntry{
 				{Path: "file", Content: "foobar", Mode: "100644"},
 			},
-			expectedErr:    git2go.HasConflictsError{},
-			expectedErrMsg: "cherry-pick: could not apply due to conflicts",
+			expectedErr:    git2go.ConflictingFilesError{},
+			expectedErrMsg: "cherry-pick: there are conflicting files",
 		},
 		{
 			desc: "empty cherry-pick fails",
@@ -161,7 +162,11 @@ func TestCherryPick(t *testing.T) {
 		}
 
 		t.Run(tc.desc, func(t *testing.T) {
-			ctx := testhelper.Context(t)
+			ctx := featureflag.ContextWithFeatureFlag(
+				testhelper.Context(t),
+				featureflag.CherryPickStructuredErrors,
+				true,
+			)
 
 			committer := git.Signature{
 				Name:  "Baz",
@@ -183,7 +188,7 @@ func TestCherryPick(t *testing.T) {
 				require.EqualError(t, err, tc.expectedErrMsg)
 
 				if tc.expectedErr != nil {
-					require.True(t, errors.Is(err, tc.expectedErr))
+					require.ErrorAs(t, err, &tc.expectedErr)
 				}
 				return
 			}
@@ -218,3 +223,67 @@ func TestCherryPick(t *testing.T) {
 		})
 	}
 }
+
+func TestCherryPickStructuredErrors(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.CherryPickStructuredErrors).Run(t,
+		testCherryPickStructuredErrors,
+	)
+}
+
+func testCherryPickStructuredErrors(t *testing.T, ctx context.Context) {
+	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
+
+	testcfg.BuildGitalyGit2Go(t, cfg)
+	executor := buildExecutor(t, cfg)
+
+	base := gittest.WriteCommit(
+		t,
+		cfg,
+		repoPath,
+		gittest.WithParents(),
+		gittest.WithTreeEntries(gittest.TreeEntry{
+			Path: "file", Content: "foo", Mode: "100644",
+		}))
+
+	ours := gittest.WriteCommit(
+		t,
+		cfg,
+		repoPath,
+		gittest.WithParents(base),
+		gittest.WithTreeEntries(
+			gittest.TreeEntry{Path: "file", Content: "fooqux", Mode: "100644"},
+		)).String()
+
+	commit := gittest.WriteCommit(
+		t,
+		cfg,
+		repoPath,
+		gittest.WithParents(base),
+		gittest.WithTreeEntries(
+			gittest.TreeEntry{Path: "file", Content: "foobar", Mode: "100644"},
+		)).String()
+
+	committer := git.Signature{
+		Name:  "Baz",
+		Email: "baz@example.com",
+		When:  time.Date(2021, 1, 17, 14, 45, 51, 0, time.FixedZone("", +2*60*60)),
+	}
+
+	_, err := executor.CherryPick(ctx, repo, git2go.CherryPickCommand{
+		Repository:    repoPath,
+		CommitterName: committer.Name,
+		CommitterMail: committer.Email,
+		CommitterDate: committer.When,
+		Message:       "Foo",
+		Ours:          ours,
+		Commit:        commit,
+	})
+
+	if featureflag.CherryPickStructuredErrors.IsEnabled(ctx) {
+		require.EqualError(t, err, "cherry-pick: there are conflicting files")
+		require.ErrorAs(t, err, &git2go.ConflictingFilesError{})
+	} else {
+		require.EqualError(t, err, "cherry-pick: could not apply due to conflicts")
+		require.ErrorAs(t, err, &git2go.HasConflictsError{})
+	}
+}
diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go
index f3dd635bc..0c379e671 100644
--- a/internal/gitaly/service/operations/cherry_pick.go
+++ b/internal/gitaly/service/operations/cherry_pick.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
@@ -62,17 +63,44 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 		Mainline:      mainline,
 	})
 	if err != nil {
+		var conflictErr git2go.ConflictingFilesError
+		var emptyErr git2go.EmptyError
 		switch {
 		case errors.As(err, &git2go.HasConflictsError{}):
 			return &gitalypb.UserCherryPickResponse{
 				CreateTreeError:     err.Error(),
 				CreateTreeErrorCode: gitalypb.UserCherryPickResponse_CONFLICT,
 			}, nil
-		case errors.As(err, &git2go.EmptyError{}):
-			return &gitalypb.UserCherryPickResponse{
-				CreateTreeError:     err.Error(),
-				CreateTreeErrorCode: gitalypb.UserCherryPickResponse_EMPTY,
-			}, nil
+		case errors.As(err, &conflictErr):
+			conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
+			for _, conflictingFile := range conflictErr.ConflictingFiles {
+				conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
+			}
+
+			detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+				helper.ErrFailedPreconditionf("cherry pick: %w", err),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_CherryPickConflict{
+						CherryPickConflict: &gitalypb.MergeConflictError{
+							ConflictingFiles: conflictingFiles,
+						},
+					},
+				})
+			if errGeneratingDetailedErr != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+			return nil, detailedErr
+		case errors.As(err, &emptyErr):
+			detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+				helper.ErrFailedPrecondition(err),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_ChangesAlreadyApplied{},
+				})
+			if errGeneratingDetailedErr != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+
+			return nil, detailedErr
 		case errors.Is(err, git2go.ErrInvalidArgument):
 			return nil, helper.ErrInvalidArgument(err)
 		default:
@@ -101,17 +129,42 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 			return nil, err
 		}
 		if !ancestor {
-			return &gitalypb.UserCherryPickResponse{
-				CommitError: "Branch diverged",
-			}, nil
+			detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+				helper.ErrFailedPrecondition(errors.New("cherry-pick: branch diverged")),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_TargetBranchDiverged{
+						TargetBranchDiverged: &gitalypb.NotAncestorError{
+							ParentRevision: []byte(oldrev.Revision()),
+							ChildRevision:  []byte(newrev),
+						},
+					},
+				})
+
+			if errGeneratingDetailedErr != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+
+			return nil, detailedErr
 		}
 	}
 
 	if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, newrev, oldrev); err != nil {
 		if errors.As(err, &updateref.CustomHookError{}) {
-			return &gitalypb.UserCherryPickResponse{
-				PreReceiveError: err.Error(),
-			}, nil
+			detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails(
+				helper.ErrFailedPrecondition(errors.New("access check failed")),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_AccessCheck{
+						AccessCheck: &gitalypb.AccessCheckError{
+							ErrorMessage: strings.TrimSuffix(err.Error(), "\n"),
+						},
+					},
+				})
+
+			if errGeneratingDetailedErr != nil {
+				return nil, helper.ErrInternalf("error details: %w", err)
+			}
+
+			return nil, detailedErr
 		}
 
 		return nil, fmt.Errorf("update reference with hooks: %w", err)
diff --git a/internal/gitaly/service/operations/cherry_pick_test.go b/internal/gitaly/service/operations/cherry_pick_test.go
index aebb68a9d..c6878cac2 100644
--- a/internal/gitaly/service/operations/cherry_pick_test.go
+++ b/internal/gitaly/service/operations/cherry_pick_test.go
@@ -1,18 +1,24 @@
 package operations
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"path/filepath"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
@@ -367,8 +373,19 @@ func TestServer_UserCherryPick_failedWithPreReceiveError(t *testing.T) {
 			gittest.WriteCustomHook(t, repoPath, hookName, hookContent)
 
 			response, err := client.UserCherryPick(ctx, request)
-			require.NoError(t, err)
-			require.Contains(t, response.PreReceiveError, "GL_ID="+gittest.TestUser.GlId)
+			require.Nil(t, response)
+			testhelper.RequireGrpcError(t, errWithDetails(t,
+				helper.ErrFailedPrecondition(
+					errors.New("access check failed"),
+				),
+				&gitalypb.UserCherryPickError{
+					Error: &gitalypb.UserCherryPickError_AccessCheck{
+						AccessCheck: &gitalypb.AccessCheckError{
+							ErrorMessage: "GL_ID=user-123",
+						},
+					},
+				},
+			), err)
 		})
 	}
 }
@@ -397,9 +414,15 @@ func TestServer_UserCherryPick_failedWithCreateTreeError(t *testing.T) {
 	}
 
 	response, err := client.UserCherryPick(ctx, request)
-	require.NoError(t, err)
-	require.NotEmpty(t, response.CreateTreeError)
-	require.Equal(t, gitalypb.UserCherryPickResponse_EMPTY, response.CreateTreeErrorCode)
+	require.Nil(t, response)
+	testhelper.RequireGrpcError(t, errWithDetails(t,
+		helper.ErrFailedPrecondition(
+			errors.New("cherry-pick: could not apply because the result was empty"),
+		),
+		&gitalypb.UserCherryPickError{
+			Error: &gitalypb.UserCherryPickError_ChangesAlreadyApplied{},
+		},
+	), err)
 }
 
 func TestServer_UserCherryPick_failedWithCommitError(t *testing.T) {
@@ -428,14 +451,30 @@ func TestServer_UserCherryPick_failedWithCommitError(t *testing.T) {
 	}
 
 	response, err := client.UserCherryPick(ctx, request)
-	require.NoError(t, err)
-	require.Equal(t, "Branch diverged", response.CommitError)
+	require.Nil(t, response)
+	s, ok := status.FromError(err)
+	require.True(t, ok)
+
+	details := s.Details()
+	require.Len(t, details, 1)
+	detailedErr, ok := details[0].(*gitalypb.UserCherryPickError)
+	require.True(t, ok)
+
+	targetBranchDivergedErr, ok := detailedErr.Error.(*gitalypb.UserCherryPickError_TargetBranchDiverged)
+	require.True(t, ok)
+	assert.Equal(t, []byte("8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab"), targetBranchDivergedErr.TargetBranchDiverged.ParentRevision)
 }
 
-func TestServer_UserCherryPick_failedWithConflict(t *testing.T) {
+func TestServerUserCherryPickRailedWithConflict(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
+	testhelper.NewFeatureSets(featureflag.CherryPickStructuredErrors).Run(
+		t,
+		testServerUserCherryPickRailedWithConflict,
+	)
+}
+
+func testServerUserCherryPickRailedWithConflict(t *testing.T, ctx context.Context) {
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -456,9 +495,25 @@ func TestServer_UserCherryPick_failedWithConflict(t *testing.T) {
 	}
 
 	response, err := client.UserCherryPick(ctx, request)
-	require.NoError(t, err)
-	require.NotEmpty(t, response.CreateTreeError)
-	require.Equal(t, gitalypb.UserCherryPickResponse_CONFLICT, response.CreateTreeErrorCode)
+	if featureflag.CherryPickStructuredErrors.IsEnabled(ctx) {
+		require.Nil(t, response)
+		s, ok := status.FromError(err)
+		require.True(t, ok)
+
+		details := s.Details()
+		require.Len(t, details, 1)
+		detailedErr, ok := details[0].(*gitalypb.UserCherryPickError)
+		require.True(t, ok)
+
+		conflictErr, ok := detailedErr.Error.(*gitalypb.UserCherryPickError_CherryPickConflict)
+		require.True(t, ok)
+		require.Len(t, conflictErr.CherryPickConflict.ConflictingFiles, 1)
+		assert.Equal(t, []byte("NEW_FILE.md"), conflictErr.CherryPickConflict.ConflictingFiles[0])
+	} else {
+		require.NoError(t, err)
+		require.NotEmpty(t, response.CreateTreeError)
+		require.Equal(t, gitalypb.UserCherryPickResponse_CONFLICT, response.CreateTreeErrorCode)
+	}
 }
 
 func TestServer_UserCherryPick_successfulWithGivenCommits(t *testing.T) {
@@ -545,8 +600,9 @@ func TestServer_UserCherryPick_quarantine(t *testing.T) {
 	}
 
 	response, err := client.UserCherryPick(ctx, request)
-	require.NoError(t, err)
-	require.NotNil(t, response)
+	require.Nil(t, response)
+	require.NotNil(t, err)
+	assert.Contains(t, err.Error(), "access check failed")
 
 	hookOutput := testhelper.MustReadFile(t, outputPath)
 	oid, err := git.NewObjectIDFromHex(text.ChompBytes(hookOutput))
diff --git a/internal/metadata/featureflag/ff_cherry_pick_structured_errors.go b/internal/metadata/featureflag/ff_cherry_pick_structured_errors.go
new file mode 100644
index 000000000..65e67b0ed
--- /dev/null
+++ b/internal/metadata/featureflag/ff_cherry_pick_structured_errors.go
@@ -0,0 +1,5 @@
+package featureflag
+
+// CherryPickStructuredErrors enables the UserCherryPick RPC to return
+// structured errors.
+var CherryPickStructuredErrors = NewFeatureFlag("cherry_pick_structured_errors", false)