Merge branch '5482-reference-conflicts-are-treated-as-internal-errors' into 'master'

Treat reference conflicts as InvalidArgument errors

Closes #5482

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

Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>

6 files changed, 65 insertions, 21 deletions

diff --git a/internal/gitaly/service/operations/merge_to_ref.go b/internal/gitaly/service/operations/merge_to_ref.go
index c065dca3e..b3d623650 100644
--- a/internal/gitaly/service/operations/merge_to_ref.go
+++ b/internal/gitaly/service/operations/merge_to_ref.go
@@ -82,6 +82,8 @@ func (s *Server) UserMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 		}
 
 		oldTargetOID = oid
+	} else if errors.Is(err, git.ErrReferenceAmbiguous) {
+		return nil, structerr.NewInvalidArgument("target reference is ambiguous: %w", err)
 	} else if errors.Is(err, git.ErrReferenceNotFound) {
 		oldTargetOID = objectHash.ZeroOID
 	} else {
diff --git a/internal/gitaly/service/operations/merge_to_ref_test.go b/internal/gitaly/service/operations/merge_to_ref_test.go
index 25db51ec2..28863e136 100644
--- a/internal/gitaly/service/operations/merge_to_ref_test.go
+++ b/internal/gitaly/service/operations/merge_to_ref_test.go
@@ -10,7 +10,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
@@ -181,39 +180,59 @@ func testUserMergeToRefConflicts(t *testing.T, ctx context.Context) {
 	))
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
+	gittest.Exec(t, cfg, "-C", repoPath, "branch", "parent-ref", right.String())
+
 	t.Run("disallow conflicts to be merged", func(t *testing.T) {
-		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, left, right, "disallowed-conflicts")
+		request := &gitalypb.UserMergeToRefRequest{
+			Repository:     repoProto,
+			User:           gittest.TestUser,
+			TargetRef:      []byte("refs/merge-requests/x/written"),
+			SourceSha:      left.String(),
+			Message:        []byte("message1"),
+			FirstParentRef: []byte("refs/heads/parent-ref"),
+		}
 
 		_, err := client.UserMergeToRef(ctx, request)
 		testhelper.RequireGrpcError(t, structerr.NewFailedPrecondition("Failed to create merge commit for source_sha %s and target_sha %s at refs/merge-requests/x/written", left, right), err)
-	})
 
-	targetRef := git.Revision("refs/merge-requests/foo")
+		hasRevision, err := repo.HasRevision(ctx, "refs/merge-requests/written")
+		require.NoError(t, err)
+		require.False(t, hasRevision, "branch should not have been created")
+	})
 
 	t.Run("failing merge does not update target reference if skipping precursor update-ref", func(t *testing.T) {
-		request := buildUserMergeToRefRequest(t, cfg, repoProto, repoPath, left, right, t.Name())
-		request.TargetRef = []byte(targetRef)
+		request := &gitalypb.UserMergeToRefRequest{
+			Repository:     repoProto,
+			User:           gittest.TestUser,
+			TargetRef:      []byte("refs/merge-requests/foo"),
+			SourceSha:      left.String(),
+			Message:        []byte("message1"),
+			FirstParentRef: []byte("refs/heads/parent-ref"),
+		}
 
 		_, err := client.UserMergeToRef(ctx, request)
-		testhelper.RequireGrpcCode(t, err, codes.FailedPrecondition)
+		testhelper.RequireGrpcError(t, structerr.NewFailedPrecondition("Failed to create merge commit for source_sha %s and target_sha %s at refs/merge-requests/foo", left, right), err)
 
-		hasRevision, err := repo.HasRevision(ctx, targetRef)
+		hasRevision, err := repo.HasRevision(ctx, "refs/merge-requests/foo")
 		require.NoError(t, err)
 		require.False(t, hasRevision, "branch should not have been created")
 	})
-}
 
-func buildUserMergeToRefRequest(tb testing.TB, cfg config.Cfg, repo *gitalypb.Repository, repoPath string, sourceSha, targetSha git.ObjectID, mergeBranchName string) *gitalypb.UserMergeToRefRequest {
-	gittest.Exec(tb, cfg, "-C", repoPath, "branch", mergeBranchName, targetSha.String())
+	t.Run("target reference is ambigous", func(t *testing.T) {
+		gittest.WriteRef(t, cfg, repoPath, "refs/merge-requests/x/written-before", right)
 
-	return &gitalypb.UserMergeToRefRequest{
-		Repository:     repo,
-		User:           gittest.TestUser,
-		TargetRef:      []byte("refs/merge-requests/x/written"),
-		SourceSha:      sourceSha.String(),
-		Message:        []byte("message1"),
-		FirstParentRef: []byte("refs/heads/" + mergeBranchName),
-	}
+		request := &gitalypb.UserMergeToRefRequest{
+			Repository:     repoProto,
+			User:           gittest.TestUser,
+			TargetRef:      []byte("refs/merge-requests/x/written*"),
+			SourceSha:      left.String(),
+			Message:        []byte("message1"),
+			FirstParentRef: []byte("refs/heads/parent-ref"),
+		}
+
+		_, err := client.UserMergeToRef(ctx, request)
+		testhelper.RequireGrpcError(t, structerr.NewInvalidArgument(`target reference is ambiguous: reference is ambiguous: conflicts with "refs/merge-requests/x/written-before"`), err)
+	})
 }
 
 func TestUserMergeToRef_stableMergeID(t *testing.T) {
diff --git a/internal/gitaly/service/operations/rebase_to_ref.go b/internal/gitaly/service/operations/rebase_to_ref.go
index a3ae93c7e..2a1c7b817 100644
--- a/internal/gitaly/service/operations/rebase_to_ref.go
+++ b/internal/gitaly/service/operations/rebase_to_ref.go
@@ -67,6 +67,8 @@ func (s *Server) UserRebaseToRef(ctx context.Context, request *gitalypb.UserReba
 		}
 
 		oldTargetOID = oid
+	} else if errors.Is(err, git.ErrReferenceAmbiguous) {
+		return nil, structerr.NewInvalidArgument("target reference is ambiguous: %w", err)
 	} else if errors.Is(err, git.ErrReferenceNotFound) {
 		oldTargetOID = objectHash.ZeroOID
 	} else {
diff --git a/internal/gitaly/service/operations/rebase_to_ref_test.go b/internal/gitaly/service/operations/rebase_to_ref_test.go
index 61ed7acc2..e178b595a 100644
--- a/internal/gitaly/service/operations/rebase_to_ref_test.go
+++ b/internal/gitaly/service/operations/rebase_to_ref_test.go
@@ -182,6 +182,15 @@ func testUserRebaseToRefFailure(t *testing.T, ctx context.Context) {
 			expectedOldOID: validSourceSha, // arbitrary valid SHA
 			expectedError:  structerr.NewFailedPrecondition("could not update %s. Please refresh and try again", validTargetRef),
 		},
+		{
+			desc:           "ambiguous target reference",
+			repo:           repo,
+			user:           gittest.TestUser,
+			targetRef:      []byte("refs/merge-requests/x/m*"),
+			sourceSha:      validSourceSha,
+			firstParentRef: validFirstParentRef,
+			expectedError:  structerr.NewInvalidArgument(`target reference is ambiguous: reference is ambiguous: conflicts with "refs/merge-requests/x/merge"`),
+		},
 	}
 
 	for _, tc := range testCases {
@@ -201,7 +210,7 @@ func testUserRebaseToRefFailure(t *testing.T, ctx context.Context) {
 				ExpectedOldOid: tc.expectedOldOID,
 			}
 			_, err := client.UserRebaseToRef(ctx, request)
-			testhelper.RequireGrpcError(t, err, tc.expectedError)
+			testhelper.RequireGrpcError(t, tc.expectedError, err)
 		})
 	}
 }
diff --git a/internal/gitaly/service/ref/branches.go b/internal/gitaly/service/ref/branches.go
index d67b18cec..8bfa08e72 100644
--- a/internal/gitaly/service/ref/branches.go
+++ b/internal/gitaly/service/ref/branches.go
@@ -26,6 +26,11 @@ func (s *server) FindBranch(ctx context.Context, req *gitalypb.FindBranchRequest
 		if errors.Is(err, git.ErrReferenceNotFound) {
 			return &gitalypb.FindBranchResponse{}, nil
 		}
+
+		if errors.Is(err, git.ErrReferenceAmbiguous) {
+			return nil, structerr.NewInvalidArgument("target reference is ambiguous: %w", err)
+		}
+
 		return nil, err
 	}
 	commit, err := repo.ReadCommit(ctx, git.Revision(branchRef.Target))
diff --git a/internal/gitaly/service/ref/branches_test.go b/internal/gitaly/service/ref/branches_test.go
index 393e44552..318f27708 100644
--- a/internal/gitaly/service/ref/branches_test.go
+++ b/internal/gitaly/service/ref/branches_test.go
@@ -87,7 +87,8 @@ func TestFailedFindBranchRequest(t *testing.T) {
 
 	ctx := testhelper.Context(t)
 	cfg, client := setupRefService(t)
-	repo, _ := gittest.CreateRepository(t, ctx, cfg)
+	repo, repoPath := gittest.CreateRepository(t, ctx, cfg)
+	gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("branch"), gittest.WithMessage("branch"))
 
 	testCases := []struct {
 		desc        string
@@ -106,6 +107,12 @@ func TestFailedFindBranchRequest(t *testing.T) {
 			branchName:  "",
 			expectedErr: status.Error(codes.InvalidArgument, "Branch name cannot be empty"),
 		},
+		{
+			desc:        "ambiguous branch name",
+			repo:        repo,
+			branchName:  "b*",
+			expectedErr: structerr.NewInvalidArgument(`target reference is ambiguous: reference is ambiguous: conflicts with "refs/heads/branch"`),
+		},
 	}
 
 	for _, testCase := range testCases {