Merge branch 'cherry_pick_clean_up' into 'master'

Make UserCherryPick return NotFound when commit cannot be found

Closes #4207

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4989

Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: James Fargher <jfargher@gitlab.com>

6 files changed, 61 insertions, 20 deletions

diff --git a/cmd/gitaly-git2go/cherry_pick_test.go b/cmd/gitaly-git2go/cherry_pick_test.go
index 25d1c843f..93308da28 100644
--- a/cmd/gitaly-git2go/cherry_pick_test.go
+++ b/cmd/gitaly-git2go/cherry_pick_test.go
@@ -84,7 +84,7 @@ func TestCherryPick(t *testing.T) {
 		commit           []gittest.TreeEntry
 		expected         map[string]string
 		expectedCommitID string
-		expectedErr      error
+		expectedErrAs    interface{}
 		expectedErrMsg   string
 	}{
 		{
@@ -114,7 +114,7 @@ func TestCherryPick(t *testing.T) {
 			commit: []gittest.TreeEntry{
 				{Path: "file", Content: "foobar", Mode: "100644"},
 			},
-			expectedErr:    git2go.ConflictingFilesError{},
+			expectedErrAs:  &git2go.ConflictingFilesError{},
 			expectedErrMsg: "cherry-pick: there are conflicting files",
 		},
 		{
@@ -128,19 +128,21 @@ func TestCherryPick(t *testing.T) {
 			commit: []gittest.TreeEntry{
 				{Path: "file", Content: "fooqux", Mode: "100644"},
 			},
-			expectedErr:    git2go.EmptyError{},
+			expectedErrAs:  &git2go.EmptyError{},
 			expectedErrMsg: "cherry-pick: could not apply because the result was empty",
 		},
 		{
 			desc:           "fails on nonexistent ours commit",
-			expectedErrMsg: "cherry-pick: ours commit lookup: lookup commit \"nonexistent\": revspec 'nonexistent' not found",
+			expectedErrAs:  &git2go.CommitNotFoundError{},
+			expectedErrMsg: "cherry-pick: ours commit lookup: commit not found: \"nonexistent\"",
 		},
 		{
 			desc: "fails on nonexistent cherry-pick commit",
 			ours: []gittest.TreeEntry{
 				{Path: "file", Content: "fooqux", Mode: "100644"},
 			},
-			expectedErrMsg: "cherry-pick: commit lookup: lookup commit \"nonexistent\": revspec 'nonexistent' not found",
+			expectedErrAs:  &git2go.CommitNotFoundError{},
+			expectedErrMsg: "cherry-pick: commit lookup: commit not found: \"nonexistent\"",
 		},
 	}
 	for _, tc := range testcases {
@@ -180,8 +182,8 @@ func TestCherryPick(t *testing.T) {
 			if tc.expectedErrMsg != "" {
 				require.EqualError(t, err, tc.expectedErrMsg)
 
-				if tc.expectedErr != nil {
-					require.ErrorAs(t, err, &tc.expectedErr)
+				if tc.expectedErrAs != nil {
+					require.ErrorAs(t, err, tc.expectedErrAs)
 				}
 				return
 			}
diff --git a/cmd/gitaly-git2go/revert_test.go b/cmd/gitaly-git2go/revert_test.go
index 0c96988ee..b4f3fbe8c 100644
--- a/cmd/gitaly-git2go/revert_test.go
+++ b/cmd/gitaly-git2go/revert_test.go
@@ -155,7 +155,7 @@ func TestRevert_trees(t *testing.T) {
 
 				return "nonexistent", revertOid.String()
 			},
-			expectedErr: "revert: ours commit lookup: lookup commit \"nonexistent\": revspec 'nonexistent' not found",
+			expectedErr: "revert: ours commit lookup: commit not found: \"nonexistent\"",
 		},
 		{
 			desc: "nonexistent revert fails",
@@ -166,7 +166,7 @@ func TestRevert_trees(t *testing.T) {
 
 				return oursOid.String(), "nonexistent"
 			},
-			expectedErr: "revert: revert commit lookup: lookup commit \"nonexistent\": revspec 'nonexistent' not found",
+			expectedErr: "revert: revert commit lookup: commit not found: \"nonexistent\"",
 		},
 	}
 	for _, tc := range testcases {
diff --git a/cmd/gitaly-git2go/util.go b/cmd/gitaly-git2go/util.go
index a6ecd2a33..187ce0708 100644
--- a/cmd/gitaly-git2go/util.go
+++ b/cmd/gitaly-git2go/util.go
@@ -6,11 +6,15 @@ import (
 	"fmt"
 
 	git "github.com/libgit2/git2go/v34"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git2go"
 )
 
 func lookupCommit(repo *git.Repository, ref string) (*git.Commit, error) {
 	object, err := repo.RevparseSingle(ref)
-	if err != nil {
+	switch {
+	case git.IsErrorCode(err, git.ErrorCodeNotFound):
+		return nil, git2go.CommitNotFoundError{Revision: ref}
+	case err != nil:
 		return nil, fmt.Errorf("lookup commit %q: %w", ref, err)
 	}
 
diff --git a/internal/git2go/serialization.go b/internal/git2go/serialization.go
index b4967ca25..0f4949c5f 100644
--- a/internal/git2go/serialization.go
+++ b/internal/git2go/serialization.go
@@ -3,6 +3,7 @@ package git2go
 import (
 	"encoding/gob"
 	"errors"
+	"fmt"
 	"reflect"
 )
 
@@ -29,6 +30,7 @@ var registeredTypes = map[reflect.Type]struct{}{
 	reflect.TypeOf(EmptyError{}):             {},
 	reflect.TypeOf(IndexError("")):           {},
 	reflect.TypeOf(ConflictError{}):          {},
+	reflect.TypeOf(CommitNotFoundError{}):    {},
 }
 
 // Result is the serialized result.
@@ -78,6 +80,16 @@ func (err EmptyError) Error() string {
 	return "could not apply because the result was empty"
 }
 
+// CommitNotFoundError indicates that the given commit rev could not be found.
+type CommitNotFoundError struct {
+	// Revision used to lookup the commit
+	Revision string
+}
+
+func (err CommitNotFoundError) Error() string {
+	return fmt.Sprintf("commit not found: %q", err.Revision)
+}
+
 // SerializableError returns an error that is Gob serializable.
 // Registered types are serialized directly. Unregistered types
 // are transformed in to an opaque error using their error message.
diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go
index 981dc9030..c81e540c3 100644
--- a/internal/gitaly/service/operations/cherry_pick.go
+++ b/internal/gitaly/service/operations/cherry_pick.go
@@ -16,7 +16,8 @@ import (
 	"google.golang.org/grpc/status"
 )
 
-//nolint:revive // This is unintentionally missing documentation.
+// UserCherryPick tries to perform a cherry-pick of a given commit onto a
+// branch. See the protobuf documentation for details.
 func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPickRequest) (*gitalypb.UserCherryPickResponse, error) {
 	if err := validateCherryPickOrRevertRequest(req); err != nil {
 		return nil, status.Errorf(codes.InvalidArgument, "UserCherryPick: %v", err)
@@ -100,6 +101,8 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 			}
 
 			return nil, detailedErr
+		case errors.As(err, &git2go.CommitNotFoundError{}):
+			return nil, helper.ErrNotFound(err)
 		case errors.Is(err, git2go.ErrInvalidArgument):
 			return nil, helper.ErrInvalidArgument(err)
 		default:
diff --git a/internal/gitaly/service/operations/cherry_pick_test.go b/internal/gitaly/service/operations/cherry_pick_test.go
index 075b095bb..7391b2fd8 100644
--- a/internal/gitaly/service/operations/cherry_pick_test.go
+++ b/internal/gitaly/service/operations/cherry_pick_test.go
@@ -280,7 +280,7 @@ func TestServer_UserCherryPick_failedValidations(t *testing.T) {
 	t.Parallel()
 	ctx := testhelper.Context(t)
 
-	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -288,11 +288,13 @@ func TestServer_UserCherryPick_failedValidations(t *testing.T) {
 	require.NoError(t, err)
 
 	destinationBranch := "cherry-picking-dst"
+	gittest.Exec(t, cfg, "-C", repoPath, "branch", destinationBranch, "master")
 
 	testCases := []struct {
-		desc    string
-		request *gitalypb.UserCherryPickRequest
-		code    codes.Code
+		desc            string
+		request         *gitalypb.UserCherryPickRequest
+		expectedErrCode codes.Code
+		expectedErrMsg  string
 	}{
 		{
 			desc: "empty user",
@@ -303,7 +305,8 @@ func TestServer_UserCherryPick_failedValidations(t *testing.T) {
 				BranchName: []byte(destinationBranch),
 				Message:    []byte("Cherry-picking " + cherryPickedCommit.Id),
 			},
-			code: codes.InvalidArgument,
+			expectedErrCode: codes.InvalidArgument,
+			expectedErrMsg:  "rpc error: code = InvalidArgument desc = UserCherryPick: empty User",
 		},
 		{
 			desc: "empty commit",
@@ -314,7 +317,8 @@ func TestServer_UserCherryPick_failedValidations(t *testing.T) {
 				BranchName: []byte(destinationBranch),
 				Message:    []byte("Cherry-picking " + cherryPickedCommit.Id),
 			},
-			code: codes.InvalidArgument,
+			expectedErrCode: codes.InvalidArgument,
+			expectedErrMsg:  "rpc error: code = InvalidArgument desc = UserCherryPick: empty Commit",
 		},
 		{
 			desc: "empty branch name",
@@ -325,7 +329,8 @@ func TestServer_UserCherryPick_failedValidations(t *testing.T) {
 				BranchName: nil,
 				Message:    []byte("Cherry-picking " + cherryPickedCommit.Id),
 			},
-			code: codes.InvalidArgument,
+			expectedErrCode: codes.InvalidArgument,
+			expectedErrMsg:  "rpc error: code = InvalidArgument desc = UserCherryPick: empty BranchName",
 		},
 		{
 			desc: "empty message",
@@ -336,14 +341,29 @@ func TestServer_UserCherryPick_failedValidations(t *testing.T) {
 				BranchName: []byte(destinationBranch),
 				Message:    nil,
 			},
-			code: codes.InvalidArgument,
+			expectedErrCode: codes.InvalidArgument,
+			expectedErrMsg:  "rpc error: code = InvalidArgument desc = UserCherryPick: empty Message",
+		},
+		{
+			desc: "commit not found",
+			request: &gitalypb.UserCherryPickRequest{
+				Repository: repoProto,
+				User:       gittest.TestUser,
+				Commit:     &gitalypb.GitCommit{Id: "will-not-be-found"},
+				BranchName: []byte(destinationBranch),
+				Message:    []byte("Cherry-picking not found"),
+			},
+			expectedErrCode: codes.NotFound,
+			expectedErrMsg:  "rpc error: code = NotFound desc = cherry-pick: commit lookup: commit not found: \"will-not-be-found\"",
 		},
 	}
 
 	for _, testCase := range testCases {
 		t.Run(testCase.desc, func(t *testing.T) {
 			_, err := client.UserCherryPick(ctx, testCase.request)
-			testhelper.RequireGrpcCode(t, err, testCase.code)
+
+			testhelper.RequireGrpcCode(t, err, testCase.expectedErrCode)
+			require.EqualError(t, err, testCase.expectedErrMsg)
 		})
 	}
 }